[PATCH] D143014: Add constant combines for `(urem/srem (mul X, Y), (mul X, Z))`
Noah Goldstein via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 3 13:18:42 PST 2023
goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added a comment.
Sorry for the delay, didn't see your replies till just now :(
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1723
+
+ bool IsSRem = I.getOpcode() == Instruction::SRem;
+
----------------
sdesmalen wrote:
> nit: Please move this closer to its use. Also, is it worth calling it `IsSigned`?
> nit: Please move this closer to its use. Also, is it worth calling it `IsSigned`?
Moved closer, it was original `IsSigned` but matt asked for change to `IsSRem`. Personally I'm agnostic.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1738
+ APInt RemYZ = IsSRem ? APIntY.srem(APIntZ) : APIntY.urem(APIntZ);
+ // (rem (mul nuw/nsw X, Y), (mul X, Z))
+ // if (rem Y, Z) == 0
----------------
sdesmalen wrote:
> There is an implied condition that if `RemYZ = 0`, then `Y >= Z`, consequently `mul X, Z` cannot overflow if `mul X, Y` cannot overflow.
>
>
> I think the code would be easier to follow if you do something like this:
>
> bool LHSNoWrap = IsSRem ? BO0HasNSW : BO0HasNUW;
> bool RHSNoWrap = IsSRem ? BO1HasNSW : BO1HasNUW;
> if (Y >= Z && LHSNoWrap) {
> // Handle case for RemYZ == 0 and Y >= Z
> } else if (Y < Z && RHSNoWrap) {
> // Handle case for RemYZ == Y
> }
> There is an implied condition that if `RemYZ = 0`, then `Y >= Z`, consequently `mul X, Z` cannot overflow if `mul X, Y` cannot overflow.
>
the `Y >= Z` -> implied flags doesn't seem to verify in all cases i.e:
https://alive2.llvm.org/ce/z/bc8BXG
vs
https://alive2.llvm.org/ce/z/uERkKH
or
https://alive2.llvm.org/ce/z/BUuMfn
vs
https://alive2.llvm.org/ce/z/X6ZEtQ
I don't doubt that there may be more cases we can add, but would prefer to leave that as a todo.
>
> I think the code would be easier to follow if you do something like this:
>
> bool LHSNoWrap = IsSRem ? BO0HasNSW : BO0HasNUW;
> bool RHSNoWrap = IsSRem ? BO1HasNSW : BO1HasNUW;
> if (Y >= Z && LHSNoWrap) {
> // Handle case for RemYZ == 0 and Y >= Z
> } else if (Y < Z && RHSNoWrap) {
> // Handle case for RemYZ == Y
> }
Done.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1761
+
+ // (rem (mul nuw/nsw X, Y), (mul {nsw} X, Z))
+ // if Y >= Z
----------------
sdesmalen wrote:
> if `Y >= Z` and `mul X, Y` doesn't overflow, then `mul X, Z` also can't overflow, so the test for B01HasNSW can be removed?
> if `Y >= Z` and `mul X, Y` doesn't overflow, then `mul X, Z` also can't overflow, so the test for B01HasNSW can be removed?
Doesn't verify: https://alive2.llvm.org/ce/z/_yqMCk
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1705
+ ConstantInt *ConstY = GetOperandAsConstantInt(Y);
+ ConstantInt *ConstZ = GetOperandAsConstantInt(Z);
+
----------------
sdesmalen wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > Why doesn't this use `match(Y, m_APInt(APIntY))` etc? As far as I can tell you don't use the ConstantInt itself, and m_APInt already handles splats.
> > > Why doesn't this use `match(Y, m_APInt(APIntY))` etc? As far as I can tell you don't use the ConstantInt itself, and m_APInt already handles splats.
> >
> > That would work for this patch (can update if thats preference), but last in the series (D143417) generalizes this to non-constants so would have to return to this code either way.
> Since we're reviewing this patch, and not D143417, I would prefer you to use `m_APInt` here. D143417 can change it back if that's what it needs to do.
DOne, likewise for the shift patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143014/new/
https://reviews.llvm.org/D143014
More information about the llvm-commits
mailing list