[PATCH] D148210: [InstCombine] Reassociate (C * X) * Y in foldICmpMulConstant

[Jun Zhang via Phabricator via llvm-commits]

junaire marked 2 inline comments as done.
junaire added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2105
+  if (Value *NewMul = tryReassociateMulConstant(Pred, Mul, Builder))
+    replaceInstUsesWith(*Mul, NewMul);
+
----------------
junaire wrote:
> bcl5980 wrote:
> > junaire wrote:
> > > bcl5980 wrote:
> > > > should be 
> > > > ```
> > > > return new ICmpInst(Pred, NewMul, Mul->getOperand(1));
> > > > ```
> > > Why return it? It doesn't make sense to me. I thought what we want to do is reassociate `(X * C) * Y` to `(X * Y) * C` so the later code can recognize the pattern and fold it. Thus we reuse the existing logic.
> > After return you still can reuse existing. I'm not sure if currect code will break something or not but I don't think we have similar usage in instcombine.
> Hi @bcl5980, thanks for your suggestion, I appreciate it. However, after I tried to replace `replaceInstUsesWith(*Mul, NewMul);` to `return new ICmpInst(Pred, NewMul, Mul->getOperand(1));`, the folds in my tests stop working. I took another deep look and I think your suggestion should be right, let me try to investigate why... (any more comments are welcome
So that's `new ICmpInst(Pred, NewMul, ConstantInt::get(Mul->getType(), C));`, not `return new ICmpInst(Pred, NewMul, Mul->getOperand(1));`. Fixed, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148210/new/

https://reviews.llvm.org/D148210