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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with minor nits.



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1043
+    return true;
+  } else if (match(E, m_Shl(m_Value(Op), m_APInt(AI)))) {
+    C = APInt(AI->getBitWidth(), 1);
----------------
This is optional, but in these cases I find it easier to avoid the `else` and write it as:

```
  if (match(E, m_Mul(m_Value(Op), m_APInt(AI)))) {
    C = *AI;
    return true;
  }

  if (match(E, m_Shl(m_Value(Op), m_APInt(AI)))) {
    C = APInt(AI->getBitWidth(), 1);
    C <<= *AI;
    return true;
  }
```

Same below.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1126
+          C0 == DivOpC && !MulWillOverflow(C0, C1, IsSigned)) {
+        Value *C0_C1 = ConstantInt::get(X->getType()->getContext(), C0 * C1);
+        Value *NewVal;
----------------
LLVM style discourages underscores in variable names.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1128
+        Value *NewVal;
+        if (IsSigned)
+          NewVal = Builder.CreateSRem(X, C0_C1, "srem");
----------------
Minor, but a ternary operation may be more readable here.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1233
+  // X % C0 + (( X / C0 ) % C1) * C0 => X % (C0 * C1)
+  if (Value *V = SimplifyAddWithRemainder(I)) {
+    return replaceInstUsesWith(I, V);
----------------
LLVM style says no braces for single line statements.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976