Page MenuHomePhabricator

Reland "[AArch64] handle -Wa,-march="
AcceptedPublic

Authored by jcai19 on Wed, May 26, 9:56 AM.

Details

Summary

This reverts commit fd11a26d368c5a909fb88548fef2cee7a6c2c931, which was
reverted by 9145a3d4ab7eb05d9fb113b5392e8961df629b88 due to a test
failure on aarch64 backend, e.g.
https://lab.llvm.org/buildbot/#/builders/43/builds/7031. This patch
fixed the test failure.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jcai19 added inline comments.Thu, May 27, 9:28 PM
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
230

Thanks for the clarification! I haven't checked GCC implementation so the comment is likely not accurate. I did make the following observation from below command and its output. It seems all the -Wa.-march values passed (armv8.3-a and armv8.4a) showed up in COLLECT_AS_OPTIONS, as well as in COLLECT_GCC_OPTIONS. On the other hand, only the last value of -march (armv8.2-a) showed up in COLLECT_GCC_OPTIONS, and the -march value was placed before all the -Wa,-march values in COLLECT_GCC_OPTIONS. WDYT?

$ aarch64-linux-gnu-gcc -Wa,-march=armv8.1-a -march=armv8.2-a -march=armv8.3-a -Wa,-march=armv8.4-a -c foo.s -o gcc.o -###
Using built-in specs.
COLLECT_AS_OPTIONS='-march=armv8.1-a' '-march=armv8.4-a'
COLLECT_GCC=aarch64-linux-gnu-gcc
Target: aarch64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 10.2.1-6+build2' --with-bugurl=file:///usr/share/doc/gcc-10/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-10 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libquadmath --disable-libquadmath-support --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --without-target-system-zlib --enable-multiarch --enable-fix-cortex-a53-843419 --disable-werror --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=aarch64-linux-gnu --program-prefix=aarch64-linux-gnu- --includedir=/usr/aarch64-linux-gnu/include --with-build-config=bootstrap-lto-lean --enable-link-mutex
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.2.1 20210110 (Debian 10.2.1-6+build2)
COLLECT_GCC_OPTIONS='-march=armv8.3-a' '-c' '-o' 'gcc.o' '-mlittle-endian' '-mabi=lp64'
/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/as -EL "-march=armv8.3-a" "-mabi=lp64" "-march=armv8.1-a" "-march=armv8.4-a" -o gcc.o foo.s
COMPILER_PATH=/usr/lib/gcc-cross/aarch64-linux-gnu/10/:/usr/lib/gcc-cross/aarch64-linux-gnu/10/:/usr/lib/gcc-cross/aarch64-linux-gnu/:/usr/lib/gcc-cross/aarch64-linux-gnu/10/:/usr/lib/gcc-cross/aarch64-linux-gnu/:/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/
LIBRARY_PATH=/usr/lib/gcc-cross/aarch64-linux-gnu/10/:/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/:/lib/aarch64-linux-gnu/:/lib/../lib/:/usr/lib/aarch64-linux-gnu/:/usr/lib/../lib/:/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-march=armv8.3-a' '-c' '-o' 'gcc.o' '-mlittle-endian' '-mabi=lp64'

Thanks for taking this up! I never got the time for it.

I'd like to see the test check that there are no unused argument warnings when there are multiple values. I missed that with another patch in this area.

Beyond that I'm not sure the logic is right here. You've got -Wa,-march being additive, in contrast to the compiler option which only uses the last value:

$ ./bin/clang --target=aarch64-arm-none-eabi -march=armv8.1-a -march=armv8.2-a /tmp/test.c -###
<...> "-target-cpu" "generic" "-target-feature" "+neon" "-target-feature" "+v8.2a" <...>

Maybe you're working to make some specific build system work but from the clang side we'd want consistent behaviour for Arm and AArch64 options.

Let me restate the Arm behaviour, minus the mcpu stuff that was also in that patch:

  • Only compiler options apply to non assembly files
  • Compiler and assembler options apply to assembly files
  • For assembly files, if you have both kinds of option, you prefer the assembler option(s)
  • Of the options that apply (or are preferred), the last value wins (it's not additive)

Do you disagree with that set of rules?

I wouldn't think that AArch64 would have that different needs, but then again I'm not trying to build existing projects, just make the clang side logical.

Matt added a subscriber: Matt.Tue, Jun 1, 12:19 PM
jcai19 added a comment.Tue, Jun 1, 2:11 PM

Thanks for the comments. Please see my replies inlined.

Thanks for taking this up! I never got the time for it.

I'd like to see the test check that there are no unused argument warnings when there are multiple values. I missed that with another patch in this area.

Beyond that I'm not sure the logic is right here. You've got -Wa,-march being additive, in contrast to the compiler option which only uses the last value:

$ ./bin/clang --target=aarch64-arm-none-eabi -march=armv8.1-a -march=armv8.2-a /tmp/test.c -###
<...> "-target-cpu" "generic" "-target-feature" "+neon" "-target-feature" "+v8.2a" <...>

Maybe you're working to make some specific build system work but from the clang side we'd want consistent behaviour for Arm and AArch64 options.

I understand it's a little bit confusing here, but I was simply trying to match GCC's behavior (please see the example in my last comment) unless I misunderstood its output. I definitely agree having consistent behaviors between Arm and Aarch64 in Clang is more reasonable (in fact that was what I implemented at first) and maybe we should fork from gcc, WDYT?

Let me restate the Arm behaviour, minus the mcpu stuff that was also in that patch:

  • Only compiler options apply to non assembly files
  • Compiler and assembler options apply to assembly files
  • For assembly files, if you have both kinds of option, you prefer the assembler option(s)
  • Of the options that apply (or are preferred), the last value wins (it's not additive)

Do you disagree with that set of rules?

I wouldn't think that AArch64 would have that different needs, but then again I'm not trying to build existing projects, just make the clang side logical.

Thanks for the clarification. I will add more checks.

nickdesaulniers added inline comments.Tue, Jun 1, 2:39 PM
clang/test/Driver/aarch64-target-as-march.s
5

https://reviews.llvm.org/D95872 tests were pretty extensive. We don't need -mcpu tests like https://reviews.llvm.org/D95872 had, but I think it might be interesting to add more tests, particularly one that verifies -Wa,-march is overriding -march.

jcai19 updated this revision to Diff 349150.Tue, Jun 1, 6:17 PM

Limit the change to assembler files, and add more tests.

jcai19 marked an inline comment as done.Tue, Jun 1, 6:20 PM
jcai19 added inline comments.
clang/test/Driver/aarch64-target-as-march.s
5

Sounds good! I've added more tests which covered the above-mentioned case.

I understand it's a little bit confusing here, but I was simply trying to match GCC's behavior (please see the example in my last comment) unless I misunderstood its output. I definitely agree having consistent behaviors between Arm and Aarch64 in Clang is more reasonable (in fact that was what I implemented at first) and maybe we should fork from gcc, WDYT?

If gas handles march by choosing the last value, then we don't need to emit multiple values just to look more like GCC's processed arguments.

Let me check what gas does when you run it directly, I think that's the best way to guide things.

$ cat /tmp/test.s
irg x0, x0
$ aarch64-unknown-linux-gnu-as -march=armv8.5-a+memtag -march=armv8.1-a /tmp/test.s -o /dev/null
/tmp/test.s: Assembler messages:
/tmp/test.s:1: Error: selected processor does not support `irg x0,x0'

GAS is also taking the last value of march. So if we were to follow clang Arm's behaviour, the result would be the same.

Except we don't have to rely on the llvm backend picking the last of "target-feature". Which probably works for architecture versions but for individual extensions I don't think so.

If we make it additive this could happen:

-march=armv8.5-a+memtag -march=armv8.2-a

Becomes: (mte is the backend name for memtag for silly reasons)

-target-feature +v8.5-a -target-feature mte -target-feature +v8.2-a

So now instead of getting v8.2 only, the user now has v8.2+memtag. Only if they do armv8.2-a+nomemtag will they get what they expect (well, what I'd expect). Which means they'd need to know every previous march value.

jcai19 marked an inline comment as done.Wed, Jun 2, 2:42 PM
$ cat /tmp/test.s
irg x0, x0
$ aarch64-unknown-linux-gnu-as -march=armv8.5-a+memtag -march=armv8.1-a /tmp/test.s -o /dev/null
/tmp/test.s: Assembler messages:
/tmp/test.s:1: Error: selected processor does not support `irg x0,x0'

GAS is also taking the last value of march. So if we were to follow clang Arm's behaviour, the result would be the same.

Except we don't have to rely on the llvm backend picking the last of "target-feature". Which probably works for architecture versions but for individual extensions I don't think so.

If we make it additive this could happen:

-march=armv8.5-a+memtag -march=armv8.2-a

Becomes: (mte is the backend name for memtag for silly reasons)

-target-feature +v8.5-a -target-feature mte -target-feature +v8.2-a

So now instead of getting v8.2 only, the user now has v8.2+memtag. Only if they do armv8.2-a+nomemtag will they get what they expect (well, what I'd expect). Which means they'd need to know every previous march value.

Thanks for double checking! I've changed my implementation since I made that comment to take the last value of -Wa,march (and only for assembly files) and added more test cases. It should now follows the Arm behavior.

nickdesaulniers added inline comments.Wed, Jun 2, 3:47 PM
clang/test/Driver/aarch64-target-as-march.s
21–26

perhaps add a test with 8.1 and 8.2 in opposite order that 8.1 is chosen, not 8.2?

jcai19 updated this revision to Diff 349450.Wed, Jun 2, 9:26 PM
jcai19 marked an inline comment as done.

Add a test case.

nickdesaulniers accepted this revision.Thu, Jun 3, 10:38 AM

LGTM; any additional thoughts @DavidSpickett ?

This revision is now accepted and ready to land.Thu, Jun 3, 10:38 AM
DavidSpickett accepted this revision.EditedFri, Jun 4, 1:38 AM

LGTM; any additional thoughts @DavidSpickett ?

A couple of additional tests just to be safe but otherwise LGTM.

Also please include in the commit message the "rules" that we're using here for parsing. Like I did for the arm change.

clang/test/Driver/aarch64-target-as-march.s
30

Add a test with -Wa,-march=armv8.2-a,-march=armv8.1-a (one -Wa with multiple values attached), then repeat the two tests but with -Xassembler instead.

jcai19 updated this revision to Diff 349912.Fri, Jun 4, 10:22 AM

Address the comments and add more tests.

LGTM; any additional thoughts @DavidSpickett ?

A couple of additional tests just to be safe but otherwise LGTM.

Also please include in the commit message the "rules" that we're using here for parsing. Like I did for the arm change.

Thanks all!

clang/test/Driver/aarch64-target-as-march.s
30

I added a test case for -Xassembler -march=armv8.2-a,-march=armv8.1-a, but it seems it does not support multiple values in one invocation?

clang --target=aarch64-linux-gnueabi -Xassembler -march=armv8.2-a,-march=armv8.1-a -c foo.s -o ias.o -###
...
clang-13: error: the clang compiler does not support '-Xassembler -march=armv8.2-a,-march=armv8.1-a'
...

nickdesaulniers requested changes to this revision.Fri, Jun 4, 10:25 AM
nickdesaulniers added inline comments.
clang/test/Driver/aarch64-target-as-march.s
30

I don't think @DavidSpickett meant more UNUSED-WARNING tests, but tests that check the actual chosen value.

This revision now requires changes to proceed.Fri, Jun 4, 10:25 AM
jcai19 updated this revision to Diff 349938.Fri, Jun 4, 12:18 PM
jcai19 marked an inline comment as done.

Update test cases based on comments.

nickdesaulniers accepted this revision.Fri, Jun 4, 12:33 PM

That looks better, thanks @jcai19 . Based on @DavidSpickett 's LGTM I think this is now ready to land.

@DavidSpickett did ask:

Also please include in the commit message the "rules" that we're using here for parsing. Like I did for the arm change.

So you may want to edit the commit description manually in phab if you plan to arc patch this in before merging, otherwise please do so locally before merging to main then pushing.

clang/test/Driver/aarch64-target-as-march.s
30

I added a test case for -Xassembler -march=armv8.2-a,-march=armv8.1-a, but it seems it does not support multiple values in one invocation?

Looking at https://reviews.llvm.org/D95872, your assessment seems correct.

This revision is now accepted and ready to land.Fri, Jun 4, 12:33 PM
jcai19 edited the summary of this revision. (Show Details)Fri, Jun 4, 1:05 PM
jcai19 added a comment.Fri, Jun 4, 1:08 PM

That looks better, thanks @jcai19 . Based on @DavidSpickett 's LGTM I think this is now ready to land.

@DavidSpickett did ask:

Also please include in the commit message the "rules" that we're using here for parsing. Like I did for the arm change.

So you may want to edit the commit description manually in phab if you plan to arc patch this in before merging, otherwise please do so locally before merging to main then pushing.

Thanks for the confirmation! I've edited the commit message to include the rules per discussed. Will wait till Monday in case @DavidSpickett has more comments.

clang/test/Driver/aarch64-target-as-march.s
30

That makes more sense. Sorry for the noise.

This revision was automatically updated to reflect the committed changes.

This breaks tests on arm macs: http://45.33.8.238/macm1/10931/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This breaks tests on arm macs: http://45.33.8.238/macm1/10931/step_7.txt

Please take a look and revert for now if it takes a while to fix.

Thanks, will address the breakage ASAP.

This breaks tests on arm macs: http://45.33.8.238/macm1/10931/step_7.txt

clang: error: the clang compiler does not support '-Wa,--version'

Wouldn't that be https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5 (https://reviews.llvm.org/D99556)?

FWIW the failure goes away locally if I revert this change here, so it's definitely due to this change here.

Things have been red for a while, probably good to revert while you investigate by this point.

jcai19 added a comment.EditedMon, Jun 7, 4:59 PM

FWIW the failure goes away locally if I revert this change here, so it's definitely due to this change here.

Things have been red for a while, probably good to revert while you investigate by this point.

Sorry for the delay but my macbook had some issues so I just finished building LLVM, but I can't seem to reproduce the test failure locally (neither by running the test directly with llvm-lit nor with ninja check-clang). In fact I don't quite understand why this test failed. Like @nickdesaulniers mentioned the build failure should have been fixed in https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5 for a different patch. Do you mind sharing the instructions you used to reproduce the test failure? Thanks.

FWIW the failure goes away locally if I revert this change here, so it's definitely due to this change here.

Things have been red for a while, probably good to revert while you investigate by this point.

Sorry for the delay but my macbook had some issues so I just finished building LLVM, but I can't seem to reproduce the test failure locally (neither by running the test directly with llvm-lit nor with ninja check-clang). In fact I don't quite understand why this test failed. Like @nickdesaulniers mentioned the build failure should have been fixed in https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5 for a different patch. Do you mind sharing the instructions you used to reproduce the test failure? Thanks.

Did you try on a mac with a m1 processor?

FWIW the failure goes away locally if I revert this change here, so it's definitely due to this change here.

Things have been red for a while, probably good to revert while you investigate by this point.

Sorry for the delay but my macbook had some issues so I just finished building LLVM, but I can't seem to reproduce the test failure locally (neither by running the test directly with llvm-lit nor with ninja check-clang). In fact I don't quite understand why this test failed. Like @nickdesaulniers mentioned the build failure should have been fixed in https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5 for a different patch. Do you mind sharing the instructions you used to reproduce the test failure? Thanks.

Did you try on a mac with a m1 processor?

This suggestion doesn't make sense to me. The order of events:

  1. https://reviews.llvm.org/rG3cc3c0f8352ec33ca2f2636f94cb1d85fc57ac16 landed, broke the build
  2. 1 was reverted in https://reviews.llvm.org/rGbf2479c347c8ca88fefdb144d8bae0a7a4231e2a
  3. 1 was relanded in https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5
  4. different patch was landed https://reviews.llvm.org/rGfd11a26d368c5a909fb88548fef2cee7a6c2c931
  5. @thakis reports patch from 4 causes a test failure, but the test failure looks like the test failure from 1, not 4.
  6. 4 is reverted in https://reviews.llvm.org/rG9145a3d4ab7eb05d9fb113b5392e8961df629b88

#5 doesn't make sense to me, and I don't see how the test would turn out differently on an m1 mac vs x86

jcai19 reopened this revision.Mon, Jun 21, 10:09 AM
This revision is now accepted and ready to land.Mon, Jun 21, 10:09 AM
jcai19 updated this revision to Diff 353410.Mon, Jun 21, 10:09 AM
jcai19 edited the summary of this revision. (Show Details)

The original implementation was indeed flawed. The failed test does not specify target triple so on X86 machines the issue won't happen unless I explicitly specify "-target aarch64".
This fixes the problem.

jcai19 retitled this revision from [AArch64] handle -Wa,-march= to Reland "[AArch64] handle -Wa,-march=".Mon, Jun 21, 10:11 AM
jcai19 edited the summary of this revision. (Show Details)
jcai19 retitled this revision from Reland "[AArch64] handle -Wa,-march=" to [AArch64] handle -Wa,-march=.Mon, Jun 21, 10:17 AM
jcai19 edited the summary of this revision. (Show Details)
jcai19 closed this revision.EditedMon, Jun 21, 10:20 AM

It's probably clearer to use a new differential review for relanding. I've created https://reviews.llvm.org/D104656. Sorry for the noises.

jcai19 reopened this revision.Mon, Jun 21, 11:04 AM
This revision is now accepted and ready to land.Mon, Jun 21, 11:04 AM
jcai19 updated this revision to Diff 353427.Mon, Jun 21, 11:05 AM

Use the original flawed code for future reference.

jcai19 closed this revision.Mon, Jun 21, 11:05 AM
jcai19 reopened this revision.Mon, Jun 21, 11:34 AM
This revision is now accepted and ready to land.Mon, Jun 21, 11:34 AM
nickdesaulniers requested changes to this revision.Mon, Jun 21, 11:37 AM

removing LGTM until the reland is posted for review here so we can use the history tab to observe the changes.

This revision now requires changes to proceed.Mon, Jun 21, 11:37 AM
jcai19 updated this revision to Diff 353440.EditedMon, Jun 21, 11:40 AM

After chatting with @nickdesaulniers offline, we have decided to abondoned D104656 in favor of keeping all the updates to this differential review for consistency. Appologies for any inconvenience caused.

jcai19 retitled this revision from [AArch64] handle -Wa,-march= to Reland "[AArch64] handle -Wa,-march=".Mon, Jun 21, 11:41 AM
jcai19 edited the summary of this revision. (Show Details)
jcai19 retitled this revision from Reland "[AArch64] handle -Wa,-march=" to Reland "[AArch64] handle -Wa,-march=".
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
201–205

If this surprised us before, it's likely to surprise us (or someone else) again. In that case, I'd recommend permitting getAArch64ArchFeaturesFromMarch to accept an empty second parameter, and simply return true early. Then callers don't need such a check and comment.

nickdesaulniers accepted this revision.Mon, Jun 21, 1:45 PM

Thanks for following up on a fix!

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
201–205

@jcai19 points out this is how it's done for arm32 as well:
https://reviews.llvm.org/D95872

This revision is now accepted and ready to land.Mon, Jun 21, 1:45 PM