Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull #226

cielavenir · 2022-10-30T09:47:45Z

No description provided.

…as crc16_t10dif_copy_pmull Signed-off-by: Taiju Yamada <[email protected]>

cielavenir · 2022-10-30T09:50:31Z

~/devel/isa-l ((3c78474...)) $ qemu-aarch64-static ./crc16_t10dif_test
Test crc16_t10dif_test .............................................................Test done: Pass
~/devel/isa-l ((3c78474...)) $ qemu-aarch64-static ./crc16_t10dif_copy_test 
Test crc16_t10dif_copy_test:
....Test done: Pass

gbtucker · 2022-10-31T17:16:06Z

crc/aarch64/crc16_t10dif_copy_pmull.S

@@ -379,13 +379,14 @@ v_br1	.req	v5
 	.size	crc16_t10dif_copy_pmull, .-crc16_t10dif_copy_pmull
 #endif

-ASM_DEF_RODATA


Why do you want them in the .text section now? They were in .rodata before. You show a cross build in the comments but cross build seems to work fine when this is in rodata like other constants.

It worked for gcc but did not work for (apple-)clang.

$ make -f Makefile.unx CC=arm64-apple-darwin-gcc AR=arm64-apple-darwin-ar arch=aarch64 host_cpu=aarch64 DEFINES="-fno-stack-check" lib programs/igzip -j8 mkdir -p bin ---> Building crc/aarch64/crc16_t10dif_pmull.S aarch64 ---> Building crc/aarch64/crc16_t10dif_copy_pmull.S aarch64 crc/aarch64/crc16_t10dif_pmull.S:222:9: error: unknown AArch64 fixup kind! ldr q_fold_const, fold_constant ^ crc/aarch64/crc16_t10dif_copy_pmull.S:231:9: error: unknown AArch64 fixup kind! ldr q_fold_const, fold_constant ^ make: *** [bin/crc16_t10dif_pmull.o] Error 1 make: *** Waiting for unfinished jobs.... make: *** [bin/crc16_t10dif_copy_pmull.o] Error 1

this has been confirmed by #226 (comment)

cielavenir · 2022-10-31T22:38:25Z

@kirbyzhou would you mind taking a look? I mean, does this improve compilation?

kirbyzhou · 2022-11-03T10:09:54Z

@kirbyzhou would you mind taking a look? I mean, does this improve compilation?

I'll try it in a 2 or 3 days. I've been busy recently.

kirbyzhou · 2022-11-17T04:29:10Z

It can be compiled with a lot of warnings:

ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: arm64 function not 4-byte aligned: _gf_vect_dot_prod_neon from erasure_code/aarch64/.libs/gf_vect_dot_prod_neon.o
ld: warning: arm64 function not 4-byte aligned: .Lloop128_init from erasure_code/aarch64/.libs/gf_vect_dot_prod_neon.o
ld: warning: arm64 function not 4-byte aligned: .Lloop128 from erasure_code/aarch64/.libs/gf_vect_dot_prod_neon.o
ld: warning: arm64 function not 4-byte aligned: .Lloop128_vects from erasure_code/aarch64/.libs/gf_vect_dot_prod_neon.o
...
ld: warning: arm64 function not 4-byte aligned: .Lloop16_vects from raid/aarch64/.libs/pq_check_neon.o
ld: warning: arm64 function not 4-byte aligned: .Lloop16_vects_end from raid/aarch64/.libs/pq_check_neon.o
ld: warning: arm64 function not 4-byte aligned: .Lloop16_end from raid/aarch64/.libs/pq_check_neon.o
ld: warning: arm64 function not 4-byte aligned: .Lerror from raid/aarch64/.libs/pq_check_neon.o
copying selected object files to avoid basename conflicts...
  CCLD     programs/igzip

The compression level=1,2,default is broken, only level=0,3 works

% time ./igzip -3 -c -z  /Users/kirbyzhou/Downloads/centos7-boot.iso  > /dev/null
./igzip -3 -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null  1.79s user 0.09s system 97% cpu 1.914 total

% time ./igzip -2  -c -z  /Users/kirbyzhou/Downloads/centos7-boot.iso  > /dev/null
zsh: segmentation fault  ./igzip -2 -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null
./igzip -2 -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null  0.00s user 0.01s system 36% cpu 0.034 total

% time ./igzip -1  -c -z  /Users/kirbyzhou/Downloads/centos7-boot.iso  > /dev/null
zsh: segmentation fault  ./igzip -1 -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null
./igzip -1 -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null  0.00s user 0.01s system 37% cpu 0.034 total

% time ./igzip -0 -c -z  /Users/kirbyzhou/Downloads/centos7-boot.iso  > /dev/null
./igzip -0 -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null  1.31s user 0.07s system 98% cpu 1.410 total

% time ./igzip  -c -z  /Users/kirbyzhou/Downloads/centos7-boot.iso  > /dev/null 
zsh: segmentation fault  ./igzip -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null
./igzip -c -z /Users/kirbyzhou/Downloads/centos7-boot.iso > /dev/null  0.00s user 0.02s system 43% cpu 0.044 total

The decompression speed is the same as gzip of system and cloudera.

isal-l % time ./igzip -c -d /tmp/x.gz | wc -c 
 531628032
./igzip -c -d /tmp/x.gz  0.24s user 0.14s system 13% cpu 2.774 total
wc -c  2.54s user 0.05s system 93% cpu 2.771 total

macos-system % time gzip -c -d /tmp/x.gz | wc -c 
 531628032
gzip -c -d /tmp/x.gz  0.20s user 0.14s system 12% cpu 2.615 total
wc -c  2.55s user 0.05s system 99% cpu 2.613 total

cloudera % time ./minigzip -c -d /tmp/x.gz | wc -c
 531628032
./minigzip -c -d /tmp/x.gz  0.28s user 0.20s system 18% cpu 2.635 total
wc -c  2.54s user 0.06s system 99% cpu 2.628 total

cielavenir · 2022-11-17T04:37:43Z

It can be compiled with a lot of warnings

Could you tell me the Xcode version used?

cielavenir · 2022-11-17T04:40:25Z

Also, is it true that you used master, not my patch?

kirbyzhou · 2022-11-28T02:21:30Z

Xcode version 14.1

commit 3c784749380f7f167ff7777fd0414f138f9debd5 (HEAD -> fix_mach2)
Author: Taiju Yamada <[email protected]>
Date:   Sun Oct 30 18:46:04 2022 +0900

    Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull
    
    Signed-off-by: Taiju Yamada <[email protected]>

commit fec429e1b952405dc539dea93b44ffa4151490a8 (origin/master, origin/HEAD, master)
Author: Greg Tucker <[email protected]>
Date:   Fri Oct 28 17:05:01 2022 -0700

    build: Add top-level read-only permissions to ci actions
    
    This is recommended by ossf scorecard:
    https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions
    
    Change-Id: I48a36cc6625fa3f1e6babb9edbe81c9522f41a13
    Signed-off-by: Greg Tucker <[email protected]>

cielavenir · 2022-11-28T03:29:53Z

@kirbyzhou could you check whether fec429e compiles on Xcode 14?

Edit: resolved #226 (comment)

akotlar · 2023-06-07T17:11:42Z

Any updates on this? Do we need this for isal support on Apple silicon? Install of the python library seems to be fine through a conda-installed Python 3.11, but I'm not clear whether it's safe to use without this patch.

cielavenir · 2023-06-09T07:29:47Z

@akotlar could you check if 3c78474 and fec429e compiles on mac?

I believe fec429e does not, but I need another witness.

Edit: resolved #226 (comment)

pablodelara · 2024-01-03T15:39:42Z

Any more info here?

cielavenir · 2024-01-05T00:10:43Z

I need someone who can check #226 (comment) , which takes less than 30min.

Otherwise I need to buy a M1 mac as github-hosted M1 runner is not free and cannot be used for experiments.

Edit: resolved #226 (comment)

akotlar · 2024-01-05T15:16:15Z

Sorry missed this notification. I’ll double check thanks!

pablodelara · 2024-01-09T15:07:25Z

@akotlar thanks for looking into it. Let us know asap, thanks!

pablodelara · 2024-01-15T11:23:31Z

We are releasing this week, so I'll defer this to next release, unless it is clear this is fixing the issue and not breaking any other ARM architectures.

jimharris · 2024-02-09T20:38:41Z

SPDK ran into this general problem as well, see spdk/spdk#3247.

We pushed a fix to our isa-l submodule fork which got me unblocked for now. But it would be great to have this fixed in isa-l upstream.

pablodelara · 2024-02-12T09:12:55Z

@liuqinfei could you check this is OK with you?

liuqinfei · 2024-02-12T10:32:54Z

Ok, i am on holiday until this Saturday. When i am back, i will verify this pr as soon as possible.

…

---- Replied Message ---- | From | Pablo de ***@***.***> | | Date | 02/12/2024 17:13 | | To | ***@***.***> | | Cc | ***@***.***>***@***.***> | | Subject | Re: [intel/isa-l] Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull (PR #226) | @liuqinfei could you check this is OK with you? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

liuqinfei · 2024-02-17T07:13:00Z

I have verified the latest master branch with this PR on kunpeng Aarch64 platform. All of the func test cases are passed.

[root@node1 isa-l]# cat /etc/os-release
NAME="openEuler"
VERSION="22.03 (LTS-SP1)"
ID="openEuler"
VERSION_ID="22.03"
PRETTY_NAME="openEuler 22.03 (LTS-SP1)"
ANSI_COLOR="0;31"

[root@node1 isa-l]# uname -r
5.10.0-136.12.0.86.oe2203sp1.aarch64

[root@node1 isa-l]# make check
make --no-print-directory check-am
make --no-print-directory erasure_code/gf_vect_mul_test erasure_code/erasure_code_test erasure_code/gf_inverse_test erasure_code/erasure_code_update_test raid/xor_gen_test raid/pq_gen_test raid/xor_check_test raid/pq_check_test crc/crc16_t10dif_test crc/crc16_t10dif_copy_test crc/crc64_funcs_test crc/crc32_funcs_test igzip/igzip_rand_test igzip/igzip_wrapper_hdr_test igzip/checksum32_funcs_test mem/mem_zero_detect_test
make --no-print-directory check-TESTS
PASS: erasure_code/gf_vect_mul_test
PASS: erasure_code/erasure_code_test
PASS: erasure_code/gf_inverse_test
PASS: erasure_code/erasure_code_update_test
PASS: raid/xor_gen_test
PASS: raid/pq_gen_test
PASS: raid/xor_check_test
PASS: raid/pq_check_test
PASS: crc/crc16_t10dif_test
PASS: crc/crc16_t10dif_copy_test
PASS: crc/crc64_funcs_test
PASS: crc/crc32_funcs_test
PASS: igzip/igzip_rand_test
PASS: igzip/igzip_wrapper_hdr_test
PASS: igzip/checksum32_funcs_test
PASS: mem/mem_zero_detect_test

Testsuite summary for libisal 2.31.0

TOTAL: 16

PASS: 16

SKIP: 0

XFAIL: 0

FAIL: 0

XPASS: 0

ERROR: 0

============================================================================

liuqinfei · 2024-02-20T17:04:38Z

Hi, @jimharris I read your PR3247 for spdk. The issue is interesting. It seems that the behavior ldr data in .rodata section should be forbidden with (apple-)clang. For this case, GCC works but (apple-)clang fails. So, it seems that it is better to fix the (apple-)clang. Right?

cielavenir · 2024-03-01T05:25:08Z

So, with macos-14 runner, I can run CI finally. Though I needed to add libtool explicitly cf #277

fec429e -based, https://github.com/cielavenir/isa-l/actions/runs/8106178923/job/22155682092 build fails
3c78474 -based, https://github.com/cielavenir/isa-l/actions/runs/8106198849/job/22155728690 build passes (fails test though)

So this pull request is valid at least (unless clang should be fixed as Liuqinfei says)

However igzip_rand_test segfaults (other 15 tests pass though), which should be investigated. But I think the investigation is super-difficult without local arm64 mac.

cielavenir · 2024-03-01T05:48:00Z

@yuhaoth
#164 (comment)

x18 problem should be fix . I will raise an issue later.

This looks forgotten. Could you take a look?

~/devel/isa-l (master) $ ag declare_generic_reg.*18
igzip/aarch64/igzip_deflate_finish_aarch64.S
100:	declare_generic_reg	m_bit_count,	18,w

igzip/aarch64/igzip_deflate_body_aarch64.S
98:	declare_generic_reg	m_bit_count,	18,w

igzip/aarch64/igzip_decode_huffman_code_block_aarch64.S
276:	declare_generic_reg	start_out,	18,x

igzip/aarch64/isal_deflate_icf_finish_hash_hist.S
120:	declare_generic_reg	file_start,		18,x

igzip/aarch64/isal_deflate_icf_body_hash_hist.S
110:	declare_generic_reg	level_buf,	18,x

I remember I tried but in igzip_decode_huffman_code_block_aarch64.S all x0 to x28 are occupied. I'm pretty not sure if x18 can be simply switched to x29.

cielavenir · 2024-03-01T17:21:04Z

@yuhaoth seems like x29 is safe. And actually, thanks to github M1-mac runner, I was avoid x18 by my own. With M1-mac github runner CI fixed, I published #278

@kirbyzhou , 278 should not have segmentation fault anymore.

cielavenir · 2024-03-04T13:36:23Z

closing to avoid confusion

pablodelara · 2024-03-06T15:49:34Z

Reopening this PR, then #278 can be rebased. @liuqinfei, are you OK with this fix?

liuqinfei · 2024-03-06T16:21:28Z

Reopening this PR, then #278 can be rebased. @liuqinfei, are you OK with this fix?

As i verified, this patch works well on arm64. If no deep study on clang, it's ok to accept this.

pablodelara · 2024-03-07T10:18:37Z

PR merged in the repo now, thanks!

chenrui333 · 2024-04-09T22:05:18Z

The patch works for me, any chance of cutting a new release? cc @pablodelara

relates to Homebrew/homebrew-core#168488

cielavenir · 2024-04-10T16:31:16Z

@chenrui333 actually you should include #278 whole content

Fix mach compilation again; fold_constant has to be the same section …

3c78474

…as crc16_t10dif_copy_pmull Signed-off-by: Taiju Yamada <[email protected]>

cielavenir mentioned this pull request Oct 30, 2022

Added Apple Silicon Mac support #164

Closed

gbtucker reviewed Oct 31, 2022

View reviewed changes

cielavenir closed this Nov 17, 2022

cielavenir reopened this Nov 17, 2022

akotlar mentioned this pull request Jun 7, 2023

isal incompatible with apple silicon bystrogenomics/bystro#116

Closed

cho-m mentioned this pull request Jul 7, 2023

xrootd: update to 5.6.1 Homebrew/homebrew-core#136074

Closed

6 tasks

rhpvorderman mentioned this pull request Jan 31, 2024

ARM OSX build fails #276

Closed

cielavenir mentioned this pull request Mar 1, 2024

Add macos-14 (arm64) CI (and made it pass finally) #278

Closed

cielavenir closed this Mar 4, 2024

pablodelara reopened this Mar 6, 2024

pablodelara closed this Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull #226

Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull #226

cielavenir commented Oct 30, 2022

cielavenir commented Oct 30, 2022

gbtucker Oct 31, 2022

cielavenir Oct 31, 2022

cielavenir Oct 31, 2022

cielavenir Mar 6, 2024

cielavenir commented Oct 31, 2022 •

edited

Loading

kirbyzhou commented Nov 3, 2022

kirbyzhou commented Nov 17, 2022

cielavenir commented Nov 17, 2022

cielavenir commented Nov 17, 2022

kirbyzhou commented Nov 28, 2022

cielavenir commented Nov 28, 2022 •

edited

Loading

akotlar commented Jun 7, 2023

cielavenir commented Jun 9, 2023 •

edited

Loading

pablodelara commented Jan 3, 2024

cielavenir commented Jan 5, 2024 •

edited

Loading

akotlar commented Jan 5, 2024

pablodelara commented Jan 9, 2024

pablodelara commented Jan 15, 2024

jimharris commented Feb 9, 2024

pablodelara commented Feb 12, 2024

liuqinfei commented Feb 12, 2024 via email

liuqinfei commented Feb 17, 2024

liuqinfei commented Feb 20, 2024

cielavenir commented Mar 1, 2024

cielavenir commented Mar 1, 2024

cielavenir commented Mar 1, 2024

cielavenir commented Mar 4, 2024

pablodelara commented Mar 6, 2024

liuqinfei commented Mar 6, 2024

pablodelara commented Mar 7, 2024

chenrui333 commented Apr 9, 2024

cielavenir commented Apr 10, 2024

Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull #226

Fix mach compilation again; fold_constant has to be the same section as crc16_t10dif_copy_pmull #226

Conversation

cielavenir commented Oct 30, 2022

cielavenir commented Oct 30, 2022

gbtucker Oct 31, 2022

Choose a reason for hiding this comment

cielavenir Oct 31, 2022

Choose a reason for hiding this comment

cielavenir Oct 31, 2022

Choose a reason for hiding this comment

cielavenir Mar 6, 2024

Choose a reason for hiding this comment

cielavenir commented Oct 31, 2022 • edited Loading

kirbyzhou commented Nov 3, 2022

kirbyzhou commented Nov 17, 2022

cielavenir commented Nov 17, 2022

cielavenir commented Nov 17, 2022

kirbyzhou commented Nov 28, 2022

cielavenir commented Nov 28, 2022 • edited Loading

akotlar commented Jun 7, 2023

cielavenir commented Jun 9, 2023 • edited Loading

pablodelara commented Jan 3, 2024

cielavenir commented Jan 5, 2024 • edited Loading

akotlar commented Jan 5, 2024

pablodelara commented Jan 9, 2024

pablodelara commented Jan 15, 2024

jimharris commented Feb 9, 2024

pablodelara commented Feb 12, 2024

liuqinfei commented Feb 12, 2024 via email

liuqinfei commented Feb 17, 2024

Testsuite summary for libisal 2.31.0

TOTAL: 16

PASS: 16

SKIP: 0

XFAIL: 0

FAIL: 0

XPASS: 0

ERROR: 0

liuqinfei commented Feb 20, 2024

cielavenir commented Mar 1, 2024

cielavenir commented Mar 1, 2024

cielavenir commented Mar 1, 2024

cielavenir commented Mar 4, 2024

pablodelara commented Mar 6, 2024

liuqinfei commented Mar 6, 2024

pablodelara commented Mar 7, 2024

chenrui333 commented Apr 9, 2024

cielavenir commented Apr 10, 2024

cielavenir commented Oct 31, 2022 •

edited

Loading

cielavenir commented Nov 28, 2022 •

edited

Loading

cielavenir commented Jun 9, 2023 •

edited

Loading

cielavenir commented Jan 5, 2024 •

edited

Loading