[PATCH] D45976: [InstCombine] Simplify Add with remainder expressions as operands.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Mostly nits except for the comment with the Alive proofs (didn't get to the meat of the patch yet).



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1035
 
+// Simplifies X % C0 + (( X / C0 ) % C1) * C0 to X % (C0 * C1).
+Instruction *InstCombiner::SimplifyAddWithRemainder(BinaryOperator &I) {
----------------
Are these correct interpretations of this transform:

https://rise4fun.com/Alive/j4G (signed)
https://rise4fun.com/Alive/RQB (unsigned)

?  If so, Alive seems to think they're incorrect.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1037
+Instruction *InstCombiner::SimplifyAddWithRemainder(BinaryOperator &I) {
+  if (I.getOpcode() != Instruction::Add)
+    return nullptr;
----------------
Can this condition ever be false?  You're calling this only from `visitAdd` right?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1045
+    ConstantInt *CI;
+    if (match(E, m_Mul(m_Value(Op), m_ConstantInt(CI)))) {
+      C = CI->getValue();
----------------
Use `m_APInt` instead of manually calling `CI->getValue()`.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1049
+    } else if (match(E, m_Shl(m_Value(Op), m_ConstantInt(CI)))) {
+      C = APInt(CI->getBitWidth(), 1);
+      C <<= CI->getValue();
----------------
Is this lifetime of this `APInt` correct?  Won't it go out of scope once `MatchMul` returns?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1064
+        match(E, m_URem(m_Value(Op), m_ConstantInt(CI))) ||
+        (match(E, m_And(m_Value(Op), m_ConstantInt(CI))) &&
+         (CI->getValue() + 1).isPowerOf2())) {
----------------
Use `m_APInt` here too.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1070
+        C++;
+      IsSigned = (Opcode == Instruction::SRem);
+      return true;
----------------
Braces around `Opcode == Instruction::SRem` are not necessary.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1087
+                match(E, m_LShr(m_Value(Op), m_ConstantInt(CI))))) {
+      C = CI->getValue();
+      if (cast<Instruction>(E)->getOpcode() == Instruction::LShr) {
----------------
Probably better to do this as a separate change, but it might be nice to push all of this logic (division as `udiv` or as an `lshr`) into a matcher.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1098
+  Value *LHS = I.getOperand(0), *RHS = I.getOperand(1);
+  Value *X, *MulOpv;
+  APInt C0, MulOpc;
----------------
LLVM style would be `MulOpV` and `MulOpC`, but you can also say `MulLHS` and `MulRHS` -- you can tell which is constant and which isn't by the C++ type.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1101
+  bool IsSigned;
+  if (((MatchRem(LHS, X, C0, IsSigned) && MatchMul(RHS, MulOpv, MulOpc)) ||
+       (MatchRem(RHS, X, C0, IsSigned) && MatchMul(LHS, MulOpv, MulOpc))) &&
----------------
I don't think you need the `(` around the call to `MatchRem`.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1101
+  bool IsSigned;
+  if (((MatchRem(LHS, X, C0, IsSigned) && MatchMul(RHS, MulOpv, MulOpc)) ||
+       (MatchRem(RHS, X, C0, IsSigned) && MatchMul(LHS, MulOpv, MulOpc))) &&
----------------
sanjoy wrote:
> I don't think you need the `(` around the call to `MatchRem`.
I think this sort of thing will be more readable if you give examples of what you're matching using the C++ names right before the `if`:

```
// Match I = (X  % C0) + (MulOpv * MulOpc)
```

etc.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1107
+    bool Rem2_IsSigned;
+    if (MatchRem(MulOpv, RemOpv, C1, Rem2_IsSigned) &&
+        IsSigned == Rem2_IsSigned) {
----------------
Same here:

```
// Match MulOpv = RemOpv % C1
```

though here the benefit is less because the code is simpler.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1120
+        Instruction *NewInst = cast<Instruction>(NewVal);
+        return replaceInstUsesWith(I, NewInst);
+      }
----------------
Can just be `replaceInstUsesWith(I, cast<Instruction>(NewVal))`.  However, please state why `NewVal` can not be a constant.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976