tests: fix loongarch test for new LLVM 23 codegen #151134
Conversation
It looks like the behavior only changed for loongarch32, so maybe the better path would be to split the test into 32 and 64 bit files, and then the 32 bit one could use revisions for LLVM versions, which would obviate the need for the goofy regular expression dancing. I'm not really sure what the best outcome is for this test, so I'm very open to suggestions.
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
the
llvm-main
|
cc @heiher @xen0n as target maintainers (https://doc.rust-lang.org/nightly/rustc/platform-support/loongarch-none.html and loongarch-linux). I don't have a strong opinion myself and unless there's opinions otherwise from the target maintainers will r+ in a few days/week. |
|
Hi, thanks for the heads-up and fix! The LA32 ABI only recently got stabilized in the latest LoongArch ABI specs v2.50, so going forward the new behavior is expected. (The In LLVM upstream, we usually don't separate LA32 and LA64 tests, but again one doesn't have to consider co-existence of multiple LLVM versions there, so IMO you can structure the tests however you want as long as it's easy to maintain. Personally, I think smaller arches benefit significantly more from maintainability than everything else. |
| // loongarch32: pca{{(lau|ddu)}}12i $t0, %got_pc{{(add)?}}_hi20(extern_static) | ||
| // loongarch32: ld.w $t0, $t0, %got_pc{{(add)?}}_lo12({{(extern_static|\.Lpcadd_hi1)}}) |
There was a problem hiding this comment.
Thanks for the fix! The two LoongArch32 variants generate different instruction sequences here: la32r emits pcaddu12i + ld.w, while la32s emits pcalau12i + ld.w. While a regex wildcard would cut down on test code line, it also makes the expected output much harder to understand at a glance. A clearer option would be to split into loongarch32r and loongarch32s (using -Ctarget-feature=+32s).
| // loongarch32: pca{{(lau|ddu)}}12i $t0, %got_pc{{(add)?}}_hi20(extern_static) | |
| // loongarch32: ld.w $t0, $t0, %got_pc{{(add)?}}_lo12({{(extern_static|\.Lpcadd_hi1)}}) | |
| // loongarch32s: pcalau12i $t0, %got_pc_hi20(extern_static) | |
| // loongarch32s: ld.w $t0, $t0, %got_pc_lo12(extern_static) | |
| // loongarch32r: pcaddu12i $t0, %got_pcadd_hi20(extern_static) | |
| // loongarch32r: ld.w $t0, $t0, %got_pcadd_lo12(.Lpcadd_hi1) |
There was a problem hiding this comment.
It looks like these changes were backported to LLVM 22, so I've incorporated this in #150722. I think that matches what you suggested here.
|
r=me if the suggestion makes sense + is squashed into the PR |
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in #151134. Depends on: * [x] #151410 * [ ] #150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
Update to LLVM 22 Scheduled release date: Feb 24 1.94 becomes stable: Mar 5 Changes: * Update to rc2, with one patch to work around our outdated illumos sysroot (rust-lang/llvm-project@41256ab). * Update the host toolchain as well, otherwise we lose cross-language LTO, in particular for jemalloc. * Adjust one loongarch assembly test. The split into r and s variants is based on the suggestion in #151134. Depends on: * [x] #151410 * [ ] #150756 * [x] llvm/llvm-project#175190 * [x] llvm/llvm-project#175912 * [x] llvm/llvm-project#175965 * [x] llvm/llvm-project#176195 * [x] llvm/llvm-project#157073 * [x] llvm/llvm-project#176421 * [x] llvm/llvm-project#176925 * [x] llvm/llvm-project#177187
|
☔ The latest upstream changes (presumably #150722) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thanks @nikic! I'll close this. |
rustbot
removed
the
S-waiting-on-review
6 participants
It looks like the behavior only changed for loongarch32, so maybe the better path would be to split the test into 32 and 64 bit files, and then the 32 bit one could use revisions for LLVM versions, which would obviate the need for the goofy regular expression dancing. I'm not really sure what the best outcome is for this test, so I'm very open to suggestions.
@rustbot label llvm-main