Page MenuHomePhabricator

[SelectionDAG] Optimize expansion for rotates/funnel shifts
ClosedPublic

Authored by RKSimon on Oct 25 2021, 5:45 AM.

Details

Summary

(Branched from D108058 as getting this completed should help unlock some other WIP patches).

If the type of a funnel shift needs to be expanded, expand it to two funnel shifts instead of regular shifts. For constant shifts, this doesn't make much difference, but for variable shifts it allows a more optimal lowering.

Also use the optimized funnel shift lowering for rotates.

Original Patch: @efriedma (Eli Friedman)

Diff Detail

Event Timeline

RKSimon created this revision.Oct 25 2021, 5:45 AM
RKSimon requested review of this revision.Oct 25 2021, 5:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 5:45 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Oct 25 2021, 9:41 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
4411

Does passing "false" to this really solve the original problem I raised? That makes getShiftAmountTy use pointer type, but pointer type on some targets is i16 which wouldn't be enough to represent a shift amount for an i65356. Should this instead use the getShiftAmountTyForConstant helper? The name of that function is outdated.

RKSimon added inline comments.Oct 25 2021, 10:13 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
4411

Sorry I missed that comment on the other phab

craig.topper added inline comments.Oct 25 2021, 10:22 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
4411

I'm working on a patch to fold getShiftAmountTyForConstant into TLI.getShiftAmountTy and then we'll never have to thing about this again.

RKSimon updated this revision to Diff 382054.Oct 25 2021, 10:49 AM

Use getShiftAmountTyForConstant

RKSimon marked an inline comment as done.Oct 25 2021, 10:49 AM

Code change LGTM. X86 and RISCV tests LGTM.

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
4402

Add a full stop at the end of this comment to match the other comments in this function.

@nemanjai The PPC codegen is no longer branchless, how much of an issue is that?

Arm parts obviously look great

RKSimon updated this revision to Diff 382248.Oct 26 2021, 3:19 AM

rebase (for real this time....)

FWIW, the PPC changes look fine.

nemanjai added inline comments.Oct 26 2021, 5:10 AM
llvm/test/CodeGen/PowerPC/funnel-shift-rot.ll
95–96

These OR's with zero seem odd, but they are essentially just register copies so there isn't an issue with performance there.

Thanks everyone - anyone willing to officially accept this?

craig.topper added inline comments.Oct 26 2021, 12:10 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
4411

Drop the false. I don't think it is needed.

RKSimon updated this revision to Diff 382423.Oct 26 2021, 12:18 PM

Use default getShiftAmountTy arguments

RKSimon updated this revision to Diff 382434.Oct 26 2021, 12:56 PM

using default legal type args caused some x86 instruction reordering

RKSimon updated this revision to Diff 382435.Oct 26 2021, 12:58 PM

I finally remembered to add the missing '.' to the end of the comment

ping - any more comments?

This revision is now accepted and ready to land.Tue, Nov 2, 2:50 AM