Change GetUnique() return type from char to init8_t #1269

martin-g · 2024-04-04T11:46:15Z

char is unsigned on aarch64

With this change I was able to fully build SPAdes on Linux ARM64:

mgrigorov in 🌐 euler-arm-22 in spades on  next 
❯ file build/bin/*
build/bin/spades-bwa:                  ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=562164721589d0b609cd56ac36c3186706248059, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-convert-bin-to-fasta: ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=74eda85d3b74b60020a4cd6657de655f3396fcde, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-core:                 ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=b5e24e4001f549108cab10b9745debf2da913be2, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-corrector-core:       ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=ead8c8c5f9a451a02bbb76b6841dc55f38b51fb5, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-gbuilder:             ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=b76e897b78e923d5cdda9c95463810bb42c40dfc, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-gfa-split:            ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=cae67bbe81c35d566da04826af6a04a85546d9fb, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-gmapper:              ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=5b8a3c6077b43c38e41d06c6d8aadc2ecbde9fcc, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-gsimplifier:          ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=7f0b67d6449d6fa4253d429e520358995c4e4f60, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-hammer:               ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=4368c59963ee313387a09cd00033531cbcc5e8e1, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-ionhammer:            ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=432aaf9b887ff8521ee073510fe47ffc379fff1b, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-kmer-estimating:      ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=ef045fa9bee7c8f061d2e133e584bebfaca731f3, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-kmercount:            ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=2f94a2b137de314fe0a12ccf4fbe47ee52afb5a0, for GNU/Linux 3.7.0, with debug_info, not stripped
build/bin/spades-read-filter:          ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, BuildID[sha1]=a7ebfa2a63cbebff0d29f490d570b5581e72b515, for GNU/Linux 3.7.0, with debug_info, not stripped

mgrigorov in 🌐 euler-arm-22 in spades on  next 
❯ ./build/bin/spades-bwa 

Program: bwa (alignment via Burrows-Wheeler transformation)
Version: 0.7.16a-r1185-dirty
Contact: Heng Li <lh3@sanger.ac.uk>

Usage:   bwa <command> [options]

Command: index         index sequences in the FASTA format
         mem           BWA-MEM algorithm
         fastmap       identify super-maximal exact matches
         pemerge       merge overlapping paired ends (EXPERIMENTAL)
         aln           gapped/ungapped alignment
         samse         generate alignment (single ended)
         sampe         generate alignment (paired ended)
         bwasw         BWA-SW for long queries

         shm           manage indices in shared memory
         fa2pac        convert FASTA to PAC format
         pac2bwt       generate BWT from PAC
         bwtupdate     update .bwt to the new format
         bwt2sa        generate SA from BWT and Occ

Note: To use BWA, you need to first index the genome with `bwa index'.
      There are three alignment algorithms in BWA: `mem', `bwasw', and
      `aln/samse/sampe'. If you are not sure which to use, try `bwa mem'
      first. Please `man ./bwa.1' for the manual.

If it does not cause regression for the supported platforms I'd be thankful if it is merged!

`char` is unsigned on aarch64 Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

asl · 2024-04-04T16:12:42Z

src/common/kmer_index/extension_index/inout_mask.hpp

@@ -63,8 +63,8 @@ class InOutMask {
        return unique[mask];
    }

-    static constexpr char GetUnique(uint8_t mask) {
-        constexpr char next[] =
+    static constexpr int8_t GetUnique(uint8_t mask) {


Thanks for the change! However, I think we need to go the other way. Here is the full story:

The return value is assumed to be 0 - 3 (representing A, C, G, T correspondingly) for input of 0, 1, 2, 4 (converting single-bit mask to a nucleotide)

-1's represent the impossible values, but still they should be as initializers.

So, the return type should really be char as it is used later on in GetUniqueOutgoing / GetUniqueIncoming.

I would probably use 0xFF instead of -1. Does this work for you on aarch64-linux? To make things explicit we can even do something like:

{ UINT8_C(0xFF), 0, 1, UINT8_C(0xFF), 2, UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), 3, UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF) };

Yes! That works too!
Let me update the PR!

I updated it but I have the feeling that more changes are needed at the call sites of GetUniqueOutgoing / GetUniqueIncoming.

Callsites should be fine. The contract is that one should never call GetUniqueOutgoing / GetUniqueIncoming if they could return 0xFF.

`char` type is unsigned on aarch64, so negative values cannot be used

asl · 2024-04-04T21:59:51Z

src/common/kmer_index/extension_index/inout_mask.hpp

@@ -63,10 +63,10 @@ class InOutMask {
        return unique[mask];
    }

-    static constexpr char GetUnique(uint8_t mask) {
+     static constexpr char GetUnique(uint8_t mask) {


Suggested change

static constexpr char GetUnique(uint8_t mask) {

static constexpr char GetUnique(uint8_t mask) {

(extra whitespace here)

asl · 2024-04-05T06:13:35Z

Well, it seems it fails build non systems with unsigned char type. Let me think a bit about better solution.

martin-g · 2024-04-05T06:17:09Z

How about using 127 instead of 255 ? AFAIU any other value than the meaningful ones (1, 2, 3 and 4) should be OK ?!

…

On Fri, 5 Apr 2024 at 9:13, Anton Korobeynikov ***@***.***> wrote: Well, it seems it fails build non systems with unsigned char type. Let me think a bit about better solution. — Reply to this email directly, view it on GitHub <#1269 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABYUQWWQ5YBFUGK2H53BWDY3Y6KJAVCNFSM6AAAAABFXDKZOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZZGAZDGNJSHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

asl · 2024-04-05T06:18:35Z

How about using 127 instead of 255 ? AFAIU any other value than the meaningful ones (1, 2, 3 and 4) should be OK

Yes, but let me think a bit more. Maybe we'd solve the problem the other way :)

asl · 2024-04-08T09:26:25Z

I pushed a fix in d28746b

Will you please check on aarch64/linux?

martin-g · 2024-04-08T10:05:08Z

I pushed a fix in d28746b

Will you please check on aarch64/linux?

Confirmed that it works fine on Linux ARM64! Thank you!
Closing this PR!

Do you have any ETA when next will be released ?

asl · 2024-04-08T10:54:47Z

Do you have any ETA when next will be released ?

Well... as soon as we will finish the documentation revamp. And finalize the release CI

asl · 2024-06-05T01:42:34Z

Just in case, SPAdes 4.0 was released: https://github.com/ablab/spades/releases/tag/v4.0.0

martin-g · 2024-06-05T01:46:17Z

Thanks!
I've updated the Bioconda recipe - bioconda/bioconda-recipes#46731

asl · 2024-06-05T01:50:02Z

Thanks! You may want to enable NCBI SRA input there. Also, it would make sense to update URL links as cab.spbu.ru is not working anymore

martin-g · 2024-06-05T07:01:30Z

You may want to enable NCBI SRA input there.

Sure! What exactly do you suggest to do ?

asl · 2024-06-05T07:02:30Z

You may want to enable NCBI SRA input there.

Sure! What exactly do you suggest to do ?

https://ablab.github.io/spades/installation.html#enabling-ncbi-sra-input-file-support

martin-g · 2024-06-05T07:06:08Z

bioconda/bioconda-recipes#48281

Change GetUnique() return type from char to init8_t

6bbf2c5

`char` is unsigned on aarch64 Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

asl reviewed Apr 4, 2024

View reviewed changes

Use 0xFF instead of -1 for the unique mask

c9e4223

`char` type is unsigned on aarch64, so negative values cannot be used

asl reviewed Apr 4, 2024

View reviewed changes

martin-g closed this Apr 8, 2024

martin-g deleted the aarch64-compatibility branch April 8, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change GetUnique() return type from char to init8_t #1269

Change GetUnique() return type from char to init8_t #1269

martin-g commented Apr 4, 2024

asl Apr 4, 2024 •

edited

Loading

martin-g Apr 4, 2024

martin-g Apr 4, 2024

asl Apr 4, 2024

asl Apr 4, 2024

asl Apr 4, 2024

asl commented Apr 5, 2024

martin-g commented Apr 5, 2024 via email

asl commented Apr 5, 2024

asl commented Apr 8, 2024

martin-g commented Apr 8, 2024

asl commented Apr 8, 2024

asl commented Jun 5, 2024

martin-g commented Jun 5, 2024

asl commented Jun 5, 2024

martin-g commented Jun 5, 2024

asl commented Jun 5, 2024

martin-g commented Jun 5, 2024

	static constexpr char GetUnique(uint8_t mask) {
	static constexpr char GetUnique(uint8_t mask) {

Change GetUnique() return type from char to init8_t #1269

Change GetUnique() return type from char to init8_t #1269

Conversation

martin-g commented Apr 4, 2024

asl Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

martin-g Apr 4, 2024

Choose a reason for hiding this comment

martin-g Apr 4, 2024

Choose a reason for hiding this comment

asl Apr 4, 2024

Choose a reason for hiding this comment

asl Apr 4, 2024

Choose a reason for hiding this comment

asl Apr 4, 2024

Choose a reason for hiding this comment

asl commented Apr 5, 2024

martin-g commented Apr 5, 2024 via email

asl commented Apr 5, 2024

asl commented Apr 8, 2024

martin-g commented Apr 8, 2024

asl commented Apr 8, 2024

asl commented Jun 5, 2024

martin-g commented Jun 5, 2024

asl commented Jun 5, 2024

martin-g commented Jun 5, 2024

asl commented Jun 5, 2024

martin-g commented Jun 5, 2024

asl Apr 4, 2024 •

edited

Loading