[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 inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1035
 
+// Simplifies X % C0 + (( X / C0 ) % C1) * C0 to X % (C0 * C1), where (C0) * C1)
+// does not overflow.
----------------
s/`(C0) * C1)`/`(C0 * C1)`/


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1041
+  // match is found.
+  auto MatchMul = [](Value *E, Value *&Op, APInt &C) {
+    const APInt *CI;
----------------
We're not getting much leverage from making these lambdas -- how about making them `static` helper functions?  That'll reduce the indentation and make `SimplifyAddWithRemainder` a bit smaller.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1042
+  auto MatchMul = [](Value *E, Value *&Op, APInt &C) {
+    const APInt *CI;
+    if (match(E, m_Mul(m_Value(Op), m_APInt(CI)))) {
----------------
Let's call this something other than `CI` since `CI` usually means `ConstantInt`.

Same comment applies below.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1062
+        match(E, m_URem(m_Value(Op), m_APInt(CI))) ||
+        (match(E, m_And(m_Value(Op), m_APInt(CI))) && (*CI + 1).isPowerOf2())) {
+      C = *CI;
----------------
Instead of `(*CI + 1).isPowerOf2()` you can do `CI->isAllOnesValue()` to avoid the addition and also read a bit cleaner.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1081
+      return true;
+    } else if (!IsSigned && (match(E, m_UDiv(m_Value(Op), m_APInt(CI))) ||
+                             match(E, m_LShr(m_Value(Op), m_APInt(CI))))) {
----------------
Can this match a `ConstantExpr` (which isn't an `Instruction` and will fail the `cast<>` below)?  Or have we checked before that `E` is an `Instruction`?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1095
+  // Returns whether C0 * C1 with the given signedness overflows.
+  auto MulOverflow = [](APInt &C0, APInt &C1, bool IsSigned) {
+    bool overflow;
----------------
Let's call this `MulWillOverflow`.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1108
+  bool IsSigned;
+  // Match I = X % C0 + MulOpV * MulOpC
+  if (((MatchRem(LHS, X, C0, IsSigned) && MatchMul(RHS, MulOpV, MulOpC)) ||
----------------
Since you're checking this anyway, how about:

```
// Match I = X % C0 + MulOpV * C0
```



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1114
+    APInt C1;
+    bool Rem2_IsSigned;
+    // Match MulOpC = RemOpV % C1
----------------
LLVM style will be `Rem2IsSigned` or `InnerRemIsSigned` (since `2` isn't very discriminating).


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1115
+    bool Rem2_IsSigned;
+    // Match MulOpC = RemOpV % C1
+    if (MatchRem(MulOpV, RemOpV, C1, Rem2_IsSigned) &&
----------------
Should be `MulOpV` in the comment.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1120
+      APInt DivOpC;
+      // Match RemOpV = DivOpV / DivOpC
+      if (MatchDiv(RemOpV, DivOpV, DivOpC, IsSigned) && X == DivOpV &&
----------------
Instead of commenting `RemOpV = DivOpV / DivOpC` how about commenting `RemOpV = X / C0`?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1123
+          C0 == DivOpC && !MulOverflow(C0, C1, IsSigned)) {
+        Value *C0_C1 = ConstantInt::get(X->getType()->getContext(), C0 * C1);
+        Value *NewVal;
----------------
How about s/`C0_C1`/`NewDivisor` ?  Generally LLVM coding style does not allow underscores in variable names.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1130
+        // NewVal can't be a constant because I is not a constant.
+        return replaceInstUsesWith(I, cast<Instruction>(NewVal));
+      }
----------------
The `cast<>` to `Instruction` is probably not necessary -- `replaceInstUsesWith` takes a `Value` as its second argument.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1231
+  // X % C0 + (( X / C0 ) % C1) * C0 => X % (C0 * C1)
+  if (Instruction *Inst = SimplifyAddWithRemainder(I)) {
+    return Inst;
----------------
LLVM style avoids braces around single line statements like this.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1232
+  if (Instruction *Inst = SimplifyAddWithRemainder(I)) {
+    return Inst;
+  }
----------------
Might be worth being consistent with the previous clause where `replaceInstUsesWith` is called here, not in the callee.


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:629
 
+  /// \brief Tries to simplify add operations using the definition of remainder.
+  ///
----------------
No need for `\brief` in new doxygen comments -- we now build the docs with autobrief which means the first sentence is automatically considered `\brief`.


================
Comment at: test/Transforms/InstCombine/add4.ll:92
+  %tmp3 = mul i32 %tmp2, 299
+  %tmp4 = add nuw nsw i32 %tmp, %tmp3
+  ret i32 %tmp4
----------------
You should probably add a test case with where the `add` does not have `nuw` or `nsw`, just to show that these flags are not necessary.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976