This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Add transforms for `(rem (shl Y, X), (shl Z, X))`
ClosedPublic

Authored by goldstein.w.n on Mar 28 2023, 9:01 PM.

Details

Summary

This is just filling in a missing case from D144225.

We treat (shl Y, X) and (shl Z, X) as (mul Z, 1 << X) and `(mul
Y, 1 << X)` then reuse the same transformations that already exist.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Mar 28 2023, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 9:01 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Mar 28 2023, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 9:01 PM

Rebase (with cleaner pattern match)

sdesmalen added inline comments.May 15 2023, 3:25 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1762

Can you add this new case to the description here?

1766

I wonder if you can do something similar to MatchShiftOrMul, so that we use a similar mechanism

if (MatchShiftOrMul(Op0, X, Y) &&
    MatchShiftOrMul(Op1, X, Z, /*MatchSpecific=*/true))
  ; // Do nothing
else if (MatchShiftConstByVal(Op0, Y, X) &&
         MatchShiftConstByVal(Op1, Z, X, /*MatchSpecific=*/true))
  ShiftX = true;
else
  return nullptr;

Just note that this requires changing the logic in MatchShiftOrMul which currently checks if V is nullptr to use m_Specific only when requested through an operand to the function. This way, you don't need to 'reset' V in between the two calls. I'm actually thinking that this explicit behaviour may be more desirable anyway, because then there is no implicit dependence between successive calls of this function.

1809

Can you add a comment describing the need for this function?

1809

nit: is it worth renaming this to CreateMulOrShift ?

1810

nit: rename to ShiftByX ?

This might be overkill because there are only two options, but for readability purposes I wondered if it was worth adding an enum which describes the two options, e.g. enum RemMatchKind { NoMatch = 0, MulValByConst = 1, ConstShiftedByVal = 2 }; or something.

1818–1820

A number of these comments are now out of date. You could add a comment which describes that:

C << X <=> C * (1 << X)

so that the same logic here still applies when you conceptually substitute (1 << X) by X.
It just requires that the return value must be implemented differently (which is why there is the GetBinOpOut function)

1822

Can you pass in the APInt instead, and do the ConstantInt::get(...) in GetBinOptOut?

goldstein.w.n marked 7 inline comments as done.May 15 2023, 12:57 PM
sdesmalen accepted this revision.May 18 2023, 1:04 AM

Thanks for the changes, LGTM!

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1786

nit: unnecessary curly braces (same above)

This revision is now accepted and ready to land.May 18 2023, 1:04 AM
goldstein.w.n added inline comments.May 18 2023, 9:13 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1786

IIRC, style is if any of the if need a curly, then all should have.

sdesmalen added inline comments.May 19 2023, 5:30 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1786

In this case none of the blocks needs the curly braces if you change the first block to ; // pass ?

goldstein.w.n added inline comments.May 19 2023, 8:10 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1786

In this case none of the blocks needs the curly braces if you change the first block to ; // pass ?

Sure, will do. I'll update when I rebase later. Any chance you will also be able to review D143417?

sdesmalen added inline comments.May 19 2023, 8:32 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1786

Thanks! Are you happy to merge the three accepted patches?

I've currently got quite a few things on my plate so I'm not sure how much time I'll have to review D143417 (especially given its complexity), but I'll try to have a look at it next week.

goldstein.w.n added inline comments.May 19 2023, 9:30 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1786

Sure, I'll merge inbetween reviews. No rush and thank you for the reviews thus far :)

Ping, gentle reminder to land these patches.

This revision was landed with ongoing or failed builds.Jul 6 2023, 12:47 PM
This revision was automatically updated to reflect the committed changes.