Page MenuHomePhabricator

[RISCV] Optimize or/xor with immediate in Zbs extension
ClosedPublic

Authored by benshi001 on May 13 2021, 5:59 AM.

Details

Summary

This patch optimize or/xor with immediate to a pair of BSETI/BINVI
if the immediate has exactly two set bits.

Diff Detail

Event Timeline

benshi001 created this revision.May 13 2021, 5:59 AM
benshi001 requested review of this revision.May 13 2021, 5:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 5:59 AM
benshi001 updated this revision to Diff 345141.May 13 2021, 7:46 AM

We need to optimize integer materialization, both in general and to take advantage of bitmanip. I suppose that the baseline test would be less bad if we had done that, but that this optimization would still be worth it?

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1293 ↗(On Diff #345141)

The code already seems self-explanatory. Remove?

1299 ↗(On Diff #345141)

"The immediate must have exactly two bits set."

llvm/lib/Target/RISCV/RISCVInstrInfoB.td
90

"only two set bits" -> "two set bits", otherwise it seems it's "up to two bits"

benshi001 updated this revision to Diff 345337.May 13 2021, 7:42 PM
benshi001 added a reviewer: luismarques.
benshi001 marked 3 inline comments as done.

Can this be moved into RISCVTargetLowering::targetShrinkDemandedConstant? aarch64 has a great optimizeLogicalImm which may give some inspiration.

benshi001 added a comment.EditedMay 14 2021, 8:07 AM

Can this be moved into RISCVTargetLowering::targetShrinkDemandedConstant? aarch64 has a great optimizeLogicalImm which may give some inspiration.

Thanks. I have investigated AArch64's targetShrinkDemandedConstant and optimizeLogicalImm, but do not think my optimization is suitable to be there.

AArch64's optimizeLogicalImm does optimize the immediate operand but does not change the operator.

While my optimization changes the operator from or/xor to bseti/binvi.

We need to optimize integer materialization, both in general and to take advantage of bitmanip. I suppose that the baseline test would be less bad if we had done that, but that this optimization would still be worth it?

Thanks for your suggestion. I will check if integer materialization can be optimized by bitmanip. But I still think my optimization is worthy to do. Snice it saves an unnecessary or/xor instr.

Please write a description of the patch so that it will go in the git log when it's committed. It would be good if you could mention Zbs in the title.

llvm/lib/Target/RISCV/RISCVInstrInfoB.td
91

Why does this need to be a ComplexPattern? Can't we do it with an ImmLeaf?

93

Btis->Bits

99

Btis->Bits

craig.topper added inline comments.May 14 2021, 9:24 AM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
91

Maybe we can't check one use from ImmLeaf?

craig.topper added inline comments.May 14 2021, 9:39 AM
llvm/lib/Target/RISCV/RISCVInstrInfoB.td
91

But we can use PatLeaf<(imm), [{ }]> I think that's the way to go. I'll post a patch to do the same to AddiPair.

benshi001 updated this revision to Diff 345600.May 14 2021, 7:24 PM
benshi001 edited the summary of this revision. (Show Details)
benshi001 marked 5 inline comments as done.

Please write a description of the patch so that it will go in the git log when it's committed. It would be good if you could mention Zbs in the title.

Thanks for your comments. I have write a brief description.

benshi001 retitled this revision from [RISCV] Optimize or/xor with immediate to [RISCV] Optimize or/xor with immediate in Zbs extension.May 15 2021, 12:08 AM
craig.topper accepted this revision.Sun, May 16, 12:14 PM

LGTM. Do you plan to do this for BCLR as well?

Can we use ORI+BSETI if there is one bit set in bits 63:11 and multiple bits in bits 10:0?

This revision is now accepted and ready to land.Sun, May 16, 12:14 PM

LGTM. Do you plan to do this for BCLR as well?

Can we use ORI+BSETI if there is one bit set in bits 63:11 and multiple bits in bits 10:0?

Thanks for your suggestion. I will do them in other patches. BTW, could you please also approve https://reviews.llvm.org/D102396 ?

Current patch depends on it.

This revision was landed with ongoing or failed builds.Sun, May 16, 8:00 PM
This revision was automatically updated to reflect the committed changes.
benshi001 added a comment.EditedSun, May 16, 8:09 PM

sorry, I pushed a wrong revision, it seems my description "This patch optimize or/xor with immediate to a pair of BSETI/BINVI
if the immediate has exactly two set bits."

is not included in my commit message.

I will pay attention to that in my future patches/commits.

And the pushed code is the exactly what Craig has approved.