Add an alias of addi [x], zero, imm to generate pseudo
instruction li, which makes assembly mush more readable.
For existed tests, users can update them by running script
llvm/utils/update_llc_test_checks.py.
Details
- Reviewers
asb luke957 - Commits
- rGaf0ecfccae82: [RISCV] Generate pseudo instruction li
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Fix errors in llvm\test\MC\RISCV\rv64zbs-aliases-valid.s and llvm\test\MC\RISCV\rv64zba-aliases-valid.s
Thanks for the patch!
I think it would be better to print this pseudo-instruction as you propose, and it aligns wiht what GCC does. I think the only reason RISC-V LLVM doesn't do this is that it was forgotten about.
The only real downside is of course the change is fairly disruptive in terms of test code. The in-tree tests use update_llc_test_checks wherever possible, and hopefully any downstream users are also doing this.
@jrtc27 - as someone maintaining a downstream branch, any thoughts on this change?
Hm, this feels a bit weird due to li's special treatment, i.e. concerns about whether the disassembly would re-assemble to the same thing. For RVI/RVIC I think that's true, but I don't know if there's a risk that addi and some bitmanip instruction both work in context and we might end up favouring the bitmanip one upon re-assembling the li, though so long as we make sure a lone addi is preferred then that's not an issue (which is what binutils does; it has li aliases for all of c.lui, c.li and addi, with everything else then falling back on their M_LI macro implementation; you don't notice the first one in disassembly though because the lui alias appears before it in the opcodes table), and we already disassemble c.li as li so it's not a new risk. I agree it does also make the disassembly a bit more readable at a glance.
As a downstream it's annoying but automated so not too concerned about that.
llvm/lib/Target/RISCV/RISCVInstrInfo.td | ||
---|---|---|
708 | Either don't bother trying to align it or add spaces to the ones below so it can be properly aligned; currently it looks weird |
As the alias is _only_ matching addi with an X0 and a simm12 I don't think there's an real scenario where it would print a standalone li that would be reassembled to anything other than an addi.
Yeah, I think this is is ok, we just have to be a bit careful to ensure we don't prefer fancy bitmanip things for PseudoLI when a plain ADDI works.
llvm/lib/Target/RISCV/RISCVInstrInfo.td | ||
---|---|---|
708 | OK,I have removed these spaces. |
I have no idea why some tests about sanitizer failed.
Other than that, is there anything I haven't noticed?
This is a completely separate thing, it does not belong here (and I don't agree that it's better, it's just different, they're both idiomatic)
I modified these files just because some sanitizer tests failed on Buildkite, which is a bit weird. And I can't reproduce these errors on my environment.
The result shows that they are irrelevant.
We discussed this last week on the sync-up call and nobody else had concerns about a change like this. @compnerd (IIRC) noted that for changes like this, it's good practice for the commit message to explain how to update (so downstreams can easily do so). I think something like this:
./llvm/utils/update_llc_test_checks.py --llc-binary=build/bin/llc $(grep -r -l -m1 -F '; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py' llvm/test/CodeGen/RISCV).
I agree with @jrtc27 that the sanitizer changes don't belong in this patch.
Oh, I see, Alex's suggestion. I'm not convinced by that, if you're maintaining a significant downstream fork and you don't know how to batch-update tests then I don't know what to say other than you shouldn't be maintaining a significant downstream fork.
FYI, all the update scripts take a -u option so they'll only update tests that are tagged as being autogenerated by that particular script, so you just need to run llvm/utils/update_llc_test_checks.py --llc-binary=build/bin/llc -u on '*.ll' and '*.mir' within the directory.
Either don't bother trying to align it or add spaces to the ones below so it can be properly aligned; currently it looks weird