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

[Bixia Zheng via Phabricator via llvm-commits]

bixia added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1035
 
+// Simplifies X % C0 + (( X / C0 ) % C1) * C0 to X % (C0 * C1).
+Instruction *InstCombiner::SimplifyAddWithRemainder(BinaryOperator &I) {
----------------
sanjoy wrote:
> Are these correct interpretations of this transform:
> 
> https://rise4fun.com/Alive/j4G (signed)
> https://rise4fun.com/Alive/RQB (unsigned)
> 
> ?  If so, Alive seems to think they're incorrect.
Added check to make sure C0*C1 does not overflow.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1037
+Instruction *InstCombiner::SimplifyAddWithRemainder(BinaryOperator &I) {
+  if (I.getOpcode() != Instruction::Add)
+    return nullptr;
----------------
sanjoy wrote:
> Can this condition ever be false?  You're calling this only from `visitAdd` right?
Removed.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1049
+    } else if (match(E, m_Shl(m_Value(Op), m_ConstantInt(CI)))) {
+      C = APInt(CI->getBitWidth(), 1);
+      C <<= CI->getValue();
----------------
sanjoy wrote:
> Is this lifetime of this `APInt` correct?  Won't it go out of scope once `MatchMul` returns?
The temp constructed by APInt(...) is copied to C.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1120
+        Instruction *NewInst = cast<Instruction>(NewVal);
+        return replaceInstUsesWith(I, NewInst);
+      }
----------------
sanjoy wrote:
> Can just be `replaceInstUsesWith(I, cast<Instruction>(NewVal))`.  However, please state why `NewVal` can not be a constant.
I put a comment that says NewVal can't be a constant because I is not a constant.


Repository:
  rL LLVM

https://reviews.llvm.org/D45976