[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.

Some style nits -- will review the content of the patch once these are fixed.  You might want to skim through these: https://llvm.org/docs/CodingStandards.html http://llvm.org/docs/ProgrammersManual.html



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1045
+  // match is found.
+  auto match_mul = [](Value *E, Value **Op, uint64_t& C) {
+    ConstantInt *CI;
----------------
LLVM naming style is CamelCase so this should be `MatchMul`.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1047
+    ConstantInt *CI;
+    if (match(E, m_Mul(m_Value(*Op), m_ConstantInt(CI))) ||
+        match(E, m_Mul(m_ConstantInt(CI), m_Value(*Op)))) {
----------------
lebedev.ri wrote:
> `m_c_Mul()`
> And please make sure that there is a test for both combinations.
I'm not sure if `m_c_Mul` is necessary here -- we should be canonicalizing multiplications such that the constant operand, if there is any, is always the RHS.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1052
+    } else if (match(E, m_Shl(m_Value(*Op), m_ConstantInt(CI)))) {
+      C = 1 << CI->getZExtValue();
+      return true;
----------------
`getZExtValue` will crash if the constant does not fit in 64 bits.  It is probably better to keep constants as `APInt` throughout.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1066
+       match(E, m_URem(m_Value(*Op), m_ConstantInt(CI))) ||
+       (match(E, m_And(m_Value(*Op), m_ConstantInt(CI))) &&
+        isPowerOf2_32(CI->getZExtValue() + 1))) {
----------------
Use `m_Power2`.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1069
+      C = CI->getZExtValue();
+      unsigned Opcode = dyn_cast<Instruction>(E)->getOpcode();
+      if (Opcode == Instruction::And) {
----------------
craig.topper wrote:
> If you use dyn_cast, you need to check the result isn't null. If it can never fail use 'cast'
`dyn_cast<T>(V)->foo()` is never necessary.  If `V` is always of type `T` then use `cast<>` not `dyn_cast<>`.  If `V` may not be of type `T` then `dyn_cast<>` may return `nullptr` and cause UB (so you need to check the value returned by `dyn_cast<>`).

More details at http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1108
+    uint64_t C1;
+    bool IsSigned_2;
+    if (match_rem(MulOpv, &RemOpv, C1, IsSigned_2) &&
----------------
`IsSigned_2` needs a more descriptive name.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976