Page MenuHomePhabricator

[DAG] Fold (srl (shl x, c1), c2) -> and(shl/srl(x, c3), m)
ClosedPublic

Authored by RKSimon on May 17 2022, 2:18 PM.

Details

Summary

Similar to the existing (shl (srl x, c1), c2) fold

Part of the work to fix the regressions in D77804

Diff Detail

Event Timeline

RKSimon created this revision.May 17 2022, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 2:18 PM
RKSimon requested review of this revision.May 17 2022, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 2:18 PM
foad added a comment.Jun 13 2022, 2:08 AM

Surely the (srl (shl x, c1), c2) form is better for targets which aren't good at doing an AND-with-large-immediate-value. Is this supposed to be a canonicalization, which targets will undo later as appropriate?

Surely the (srl (shl x, c1), c2) form is better for targets which aren't good at doing an AND-with-large-immediate-value. Is this supposed to be a canonicalization, which targets will undo later as appropriate?

We have the TLI shouldFoldConstantShiftPairToMask callback to allow targets to control this.

craig.topper added inline comments.Jun 14 2022, 10:37 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9442

Why is the last SDLoc from N0?

RKSimon added inline comments.Jun 14 2022, 10:48 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9442

It was copy+pasted from the (shl (srl x, c1), c2) variant

craig.topper added inline comments.Jun 14 2022, 10:58 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9442

Is that correct? Can you swap the order of debug locs? The N0 debug loc is now after the N location in the DAG

RKSimon added inline comments.Jun 14 2022, 11:02 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9442

I'll take a look

RKSimon added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9442
RKSimon updated this revision to Diff 437103.Jun 15 2022, 3:24 AM

Fix SDLoc mismatch

RKSimon updated this revision to Diff 437916.Jun 17 2022, 8:18 AM
RKSimon retitled this revision from [DAG] Fold (srl (shl x, c1), c2) -> and(shl/srl(x, c3), m) (WIP) to [DAG] Fold (srl (shl x, c1), c2) -> and(shl/srl(x, c3), m).
RKSimon edited the summary of this revision. (Show Details)

Extend AArch64TargetLowering::shouldFoldConstantShiftPairToMask so that we don't lose the srl(shl(x,c1),c2) pair if it would likely break a UBFX pattern.

The AArch64 change sounds OK to me.

Thank @dmgreen - does anyone have more comments?

spatel accepted this revision.Jun 19 2022, 12:21 PM

LGTM - noted a close call in x86 tests, but that can be addressed if we find real regressions.

llvm/test/CodeGen/X86/shift-mask.ll
597

This and the next diff might be worse depending on uarch, so that suggests a possible refinement for the x86 TLI hook for 64-bit integer.

This revision is now accepted and ready to land.Jun 19 2022, 12:21 PM
This revision was landed with ongoing or failed builds.Jun 20 2022, 12:37 AM
This revision was automatically updated to reflect the committed changes.