This is an archive of the discontinued LLVM Phabricator instance.

[DAG] MatchRotate - Add funnel shift by immediate support
ClosedPublic

Authored by RKSimon on Feb 25 2020, 6:29 AM.

Details

Summary

This patch reuses the existing MatchRotate ROTL/ROTR rotation pattern code to also recognize the more general FSHL/FSHR funnel shift patterns when we have constant shift amounts.

Diff Detail

Event Timeline

RKSimon created this revision.Feb 25 2020, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2020, 6:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lebedev.ri added inline comments.Feb 25 2020, 12:27 PM
llvm/test/CodeGen/X86/avg.ll
2564–2592

All this doesn't look like an improvement,
but i suspect this is an unrelated codegen issue
that is only being exposed by the change.

llvm/test/CodeGen/X86/bitreverse.ll
1114 ↗(On Diff #246436)

Technically this implies more registers are being used here,
not sure what that says about the actual asm change.

llvm/test/CodeGen/X86/known-bits.ll
123

Demandedbits failure? (no longer narrowed into 32-bit)

RKSimon marked 2 inline comments as done.Feb 28 2020, 6:52 AM
RKSimon added inline comments.
llvm/lib/Target/Hexagon/HexagonPatterns.td
1098 ↗(On Diff #246436)

@kparzysz Any comments on this? Should I split off a patch with a fshl specific test case?

RKSimon added inline comments.Mar 4 2020, 9:49 AM
llvm/test/CodeGen/X86/shift-combine.ll
271 ↗(On Diff #246436)

This should be addressed by D75624

The Hexagon changes LGTM.

llvm/lib/Target/Hexagon/HexagonPatterns.td
1098 ↗(On Diff #246436)

You are correct. This should be A2_combine_lh.

RKSimon marked an inline comment as done.Mar 6 2020, 6:45 AM
RKSimon added inline comments.
llvm/lib/Target/Hexagon/HexagonPatterns.td
1098 ↗(On Diff #246436)

Cheers, I'll create a test and commit the fix shortly.

RKSimon updated this revision to Diff 248778.Mar 6 2020, 10:02 AM
RKSimon edited the summary of this revision. (Show Details)

Rebased now that the Hexagon bugfix has landed - thanks @kparzysz

RKSimon updated this revision to Diff 249590.Mar 11 2020, 5:07 AM

rebase - now that D75748 has landed, most of the regressions have been solved.

lebedev.ri accepted this revision.Mar 11 2020, 5:52 AM
lebedev.ri added reviewers: sameconrad, hans.

Looks good to me.
Anyone else?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
6410–6411

/// ... where C1+C2 == bitwidth(x)

llvm/test/CodeGen/X86/known-bits.ll
123

Still applicable

This revision is now accepted and ready to land.Mar 11 2020, 5:52 AM
RKSimon marked an inline comment as done.Mar 11 2020, 8:21 AM
RKSimon added inline comments.
llvm/test/CodeGen/X86/known-bits.ll
123

I'm still looking at the best way to handle overflow ops with SimplifyDemandedBits/ShrinkDemandedOp

spatel accepted this revision.Mar 11 2020, 8:57 AM

LGTM - see inline for a nit.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
6418–6419

Nit: the code structure of these checks seems odd to me.
If we're ok with using the "?:" to choose L/R for rotate, do the same for fsh?

if (IsRotate && (HasROTL || HasROTR))
  Res = DAG.getNode(HasROTL ? ISD::ROTL : ISD::ROTR, DL, VT, LHSShiftArg,
                    HasROTL ? LHSShiftAmt : RHSShiftAmt);
else
  Res = DAG.getNode(HasFSHL ? ISD::FSHL : ISD::FSHR, DL, VT, LHSShiftArg,
                    RHSShiftArg, HasFSHL ? LHSShiftAmt : RHSShiftAmt);
This revision was automatically updated to reflect the committed changes.
RKSimon marked an inline comment as done.Mar 11 2020, 12:16 PM