This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Generate pseudo instruction li
ClosedPublic

Authored by pcwang-thead on Oct 27 2021, 11:45 PM.

Details

Summary

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.

Diff Detail

Event Timeline

pcwang-thead requested review of this revision.Oct 27 2021, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 11:45 PM

Updating D112692: [RISCV] Generate pseudo instruction li

Fix testsuite errors.

Fix errors in llvm\test\MC\RISCV\rv64zbs-aliases-valid.s and llvm\test\MC\RISCV\rv64zba-aliases-valid.s

asb added a comment.Oct 28 2021, 7:02 AM

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.

jrtc27 added inline comments.Oct 28 2021, 7:19 AM
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

luke957 resigned from this revision.Oct 28 2021, 7:28 AM
asb added a comment.Oct 28 2021, 7:28 AM

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.

So sorry for my bad herald script.

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.

Removed spaces.

Change addi/mv zero in compiler-rt to li

Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 1:54 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
This comment was removed by pcwang-thead.
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?

jrtc27 added a comment.Nov 1 2021, 5:34 AM

Change addi/mv zero in compiler-rt to li

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)

Change addi/mv zero in compiler-rt to li

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.

asb added a subscriber: compnerd.Nov 2 2021, 7:22 AM

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.

pcwang-thead marked an inline comment as done.
  • Reverts sanitizer changes.
  • Adds a note about how to update tests to commit message.
jrtc27 added a comment.Nov 2 2021, 9:16 PM
  • Adds a note about how to update tests to commit message.

... why?

pcwang-thead edited the summary of this revision. (Show Details)Nov 2 2021, 9:17 PM
jrtc27 added a comment.Nov 2 2021, 9:18 PM

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.

jrtc27 added a comment.Nov 2 2021, 9:21 PM

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.

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.

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.

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.

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.

Thanks for your advice, I haven't noticed this option. :)

asb added a comment.Nov 5 2021, 4:11 AM

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.

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.

Egg on my face - I forgot about that option :) Thanks.

asb accepted this revision.Nov 5 2021, 4:11 AM
This revision is now accepted and ready to land.Nov 5 2021, 4:11 AM
This revision was automatically updated to reflect the committed changes.