Page MenuHomePhabricator

[RISCV] optimize addition with a pair of (addi imm)
ClosedPublic

Authored by benshi001 on Jun 20 2020, 9:39 AM.

Details

Summary

For an addition with an immediate in specific ranges, a pair of
addi-addi can be generated instead of the ordinary lui-addi-add serial.

Diff Detail

Event Timeline

benshi001 created this revision.Jun 20 2020, 9:39 AM

for (add i32 %a, 4094), it can be simplfied to
addi a0, a0, 2047
addi a0, a0, 2047.

The original assembly is
lui a1, 1
addi a1, a1, -2
add a0, a0, a1

However, this optimization can only be performed if the value generated by lui-addi has no other use.

My information,

name = Ben Shi
email = powerman1st@163.com

benshi001 updated this revision to Diff 272280.Jun 20 2020, 5:41 PM
benshi001 updated this revision to Diff 272300.Jun 21 2020, 3:45 AM
benshi001 edited the summary of this revision. (Show Details)
luismarques added inline comments.Jun 23 2020, 12:58 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
85

Given that you can have an ADDI of -2048, that should be -4096, not -4095. (Please update the comment, the code and the test results accordingly).

87–88

This is a matter of style so it's not very important, but I would lean towards implementing this check with a comparison logic that matches the comment about the required range, as that would be make it easier to check at a glance.

92–93

Nitpick: make it the other way around, so that the second addi is the "smaller" one. That generated code seems more intuitive to me that way :-)

llvm/test/CodeGen/RISCV/add-imm.ll
9–25

When you have i32 and the i64 tests they should both have riscv32 and riscv64 test CHECKs. You can have operations on i64s in riscv32, they just doesn't correspond to a single native instruction. But in this case I don't see much advantage in having test variants for both types, as this optimization should be orthogonal to that. I suggest keeping one i32+i64 test (to show that it works with both types), and trimming the remaining tests down to just the i32 variant.

benshi001 updated this revision to Diff 273379.Jun 25 2020, 8:25 AM
benshi001 marked 4 inline comments as done.

All done. Thanks for your suggestion. ^_^

Why "Harbormaster failed remote builds in B61745: Diff 273379!" ???

The "make check-all" fully passed with the new version of patch.

Could you please help me review it? I have not got any suggestion for one week :)

luismarques accepted this revision.Jul 2 2020, 7:40 AM

The patch seems correct to me. I'm not quite sure this is the best place to do the this transformation (guidance on that from other people would be welcome), but I don't have a better solution to propose right now. I'm going to approve this patch, but I suggest you wait a couple more days to give other people an opportunity to comment on that issue.
Regarding the tests, as a nitpicking issue, I would only suggest giving the test functions slightly more semantic names (maybe something like negative_bound_reject, etc.).

This revision is now accepted and ready to land.Jul 2 2020, 7:40 AM
benshi001 updated this revision to Diff 275546.Jul 5 2020, 5:35 AM

Thanks. I have improved the function names according to your suggestion!

MaskRay added inline comments.Jul 7 2020, 3:49 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
80

The two comments can be combined and placed under case ISD::ADD:

Optimize (add r, imm) to (addi (addi r, imm0) imm1) if applicable.

83

if (!ConstOp->hasOneUse()) is not tested.

92

Add const if applicable.

benshi001 updated this revision to Diff 276293.Jul 7 2020, 6:10 PM
benshi001 retitled this revision from [RISCV] Optimize addition with an immediate to [RISCV] optimize addition with a pair of (addi imm).
benshi001 marked 2 inline comments as done.
MaskRay accepted this revision.Jul 7 2020, 6:54 PM

Looks great!

This revision was automatically updated to reflect the committed changes.