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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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)))) {
----------------
`m_c_Mul()`
And please make sure that there is a test for both combinations.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1065
+    if (match(E, m_SRem(m_Value(*Op), m_ConstantInt(CI))) ||
+       match(E, m_URem(m_Value(*Op), m_ConstantInt(CI))) ||
+       (match(E, m_And(m_Value(*Op), m_ConstantInt(CI))) &&
----------------
The formatting looks off, maybe use clang-format on the diff (you could use a git post-commit hook)


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1103
+  bool IsSigned;
+  if (((match_rem(LHS, &X, C0, IsSigned) && match_mul(RHS, &MulOpv, MulOpc))
+      || (match_rem(RHS, &X, C0, IsSigned) &&
----------------
You take address of `MulOpv`, and then immediately deference it in `match_rem()`.
Maybe pass it by reference (`Value *&Op`)


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1229
+  // X % C0 + (( X / C0 ) % C1) * C0 => X % (C0 * C1)
+  if (Instruction *Inst = SimplifyAddWithRemainder(I)) {
+    return Inst;
----------------
Here and everywhere - no need for `{}` in such one-line cases.


================
Comment at: test/Transforms/InstCombine/add4.ll:1
+; RUN: opt < %s -instcombine -S | FileCheck %s
+; Test transformation: X % C0 + (( X / C0 ) % C1) * C0 => X % (C0 * C1)
----------------
Please use `llvm/utils/update_test_checks.py`, and ideally split the initial tests (with the output as of trunk) into parent differential, rebase this differential ontop of that one, so the changes are more clear.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976