This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Support arbitrary const shift amount for `lshr (sext i1 ...)`
ClosedPublic

Authored by anton-afanasyev on Oct 7 2021, 11:59 AM.

Details

Summary

Add lshr (sext i1 X to iN), C --> select (X, -1 >> C, 0) case. This expands
C == N-1 case to arbitrary C.

Fixes PR52078

Diff Detail

Event Timeline

anton-afanasyev requested review of this revision.Oct 7 2021, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 11:59 AM
spatel added a comment.Oct 8 2021, 8:40 AM

Can you add the instcombine tests to the file(s) where the existing transform's tests are ("lshr.ll")? It's easier to see the variations that we handle in one look that way.

llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
1069–1070

We don't need the shouldChangeType restriction for the fold to select / zext, do we?
(I'm not sure that we need it for the fold that creates a shift either; that was conservatively added back in D33879.)

1074–1075

For a wide type, this won't work the way you are expecting. Use getAllOnesValue or APInt::getLowBitsSet().

Please add a test like this to verify we don't miscompile:

define i128 @foo(i1 %a) {
  %s = sext i1 %a to i128
  %lshr = lshr i128 %s, 42
  ret i128 %lshr
}

This might need a data layout adjustment to trigger (or the suggestion above to remove shouldChangeType).

llvm/test/Transforms/InstCombine/pr52078.ll
14 ↗(On Diff #377940)

This should already be covered by an existing test, so it shouldn't be necessary.
We do need other tests though: extra use on the sext, vector type, etc.

anton-afanasyev marked 3 inline comments as done.Oct 12 2021, 2:51 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
1069–1070

Actually that's not a restriction since i1 type is desirable so shouldChangeType() check is always true.
But ok, moved that check down for shifts only.

1074–1075

Thanks, fixed and added test.

llvm/test/Transforms/InstCombine/pr52078.ll
14 ↗(On Diff #377940)

Ok, removed and added new tests

anton-afanasyev marked 3 inline comments as done.

Address comments

spatel accepted this revision.Oct 13 2021, 5:28 AM

LGTM - see inline test comments.

llvm/test/Transforms/PhaseOrdering/pr52078.ll
2–3 ↗(On Diff #378939)
  1. Better to create a common prefix if output is always expected to be the same (just the default "CHECK" would do?).
  2. Add a -O2 and/or -O3 run, so we know this behaves as expected with the normal pass pipelines (we still want to adjust AIC in those pipelines in another patch?).
This revision is now accepted and ready to land.Oct 13 2021, 5:28 AM
anton-afanasyev marked an inline comment as done.Oct 14 2021, 6:20 AM
anton-afanasyev added inline comments.
llvm/test/Transforms/PhaseOrdering/pr52078.ll
2–3 ↗(On Diff #378939)
  1. There's still a small difference in variable names (%lshr and %trunc) after different folding ways.
  2. Ok, added -O2, but have to add one more test to show difference here. (And yes, we're still going to adjust pipeline for AIC later).
anton-afanasyev marked an inline comment as done.

Update tests

This revision was landed with ongoing or failed builds.Oct 15 2021, 3:51 AM
This revision was automatically updated to reflect the committed changes.