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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1059
+  // Matches remainder expression Op % C where C is a constant. Returns the
+  // constant value in C and the other operand in Op. Returns the signess of
+  // the remainder operation in IsSigned. Returns true if such a match is
----------------
signess->signedness


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1062
+  // found.
+  auto match_rem = [](Value *E, Value **Op, uint64_t& C, bool& IsSigned) {
+    ConstantInt *CI;
----------------
Can you pass Op as Value *&Op?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1069
+      C = CI->getZExtValue();
+      unsigned Opcode = dyn_cast<Instruction>(E)->getOpcode();
+      if (Opcode == Instruction::And) {
----------------
If you use dyn_cast, you need to check the result isn't null. If it can never fail use 'cast'


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1085
+    if (IsSigned && match(E, m_SDiv(m_Value(*Op), m_ConstantInt(CI)))) {
+      C = CI->getZExtValue();
+      return true;
----------------
What guarantees the ConstantInt only uses 64 bits or less?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1114
+      if (match_div(RemOpv, &DivOpv, DivOpc, IsSigned) && X == DivOpv
+          && C0 == DivOpc) {
+        Value *C0_C1 = ConstantInt::get(X->getType(), C0*C1, IsSigned);
----------------
&& should be at the end of the previous line. If they won't fit, bring "X == DivOpv" down. Or just clang-format thsi.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1122
+        }
+        Instruction *NewInst = dyn_cast<Instruction>(NewVal);
+        if (NewInst->getParent()) {
----------------
Unchecked dyn_cast


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1123
+        Instruction *NewInst = dyn_cast<Instruction>(NewVal);
+        if (NewInst->getParent()) {
+          // It is an existing instruction.
----------------
The IR builder will never give you back an existing instruction. The only optimization it can do is to constant fold, but that wouldn't castable to an Instruction.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976