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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Driver/ToolChains/Arch/AArch64.cpp | ||
---|---|---|
227 | This comment is confusing. -march is a driver option and the GCC driver will internally convert it into the equivalent of -Wa,-march before passing the total command line to GNU as. |
clang/lib/Driver/ToolChains/Arch/AArch64.cpp | ||
---|---|---|
227 | 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 -### |
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.
Thanks for the comments. Please see my replies inlined.
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.
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. |
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.
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.
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? |
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. |
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/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. |
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 |
Looking at https://reviews.llvm.org/D95872, your assessment seems correct. |
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 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.
clang: error: the clang compiler does not support '-Wa,--version'
Wouldn't that be https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5 (https://reviews.llvm.org/D99556)?
First bad build is http://45.33.8.238/macm1/10923/summary.html which has https://github.com/llvm/llvm-project/compare/1465e7770bca...43929ccc1296 as regression range
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.
This suggestion doesn't make sense to me. The order of events:
- https://reviews.llvm.org/rG3cc3c0f8352ec33ca2f2636f94cb1d85fc57ac16 landed, broke the build
- 1 was reverted in https://reviews.llvm.org/rGbf2479c347c8ca88fefdb144d8bae0a7a4231e2a
- 1 was relanded in https://reviews.llvm.org/rG76d9bc72784d88f4dd57b9939e52c73739438af5
- different patch was landed https://reviews.llvm.org/rGfd11a26d368c5a909fb88548fef2cee7a6c2c931
- @thakis reports patch from 4 causes a test failure, but the test failure looks like the test failure from 1, not 4.
- 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
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.
It's probably clearer to use a new differential review for relanding. I've created https://reviews.llvm.org/D104656. Sorry for the noises.
removing LGTM until the reland is posted for review here so we can use the history tab to observe the changes.
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.
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. |
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: |
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.