[PATCH] D129612: [Reassociate] Cleanup minor missed optimizations

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

In D129612#3648882 <https://reviews.llvm.org/D129612#3648882>, @spatel wrote:

> Is it possible to create an integer (mul and sub) test that also has this problem?

I expect so.  I'll look into creating one.



================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:580-581
 
       // If this is a multiply expression, turn any internal negations into
       // multiplies by -1 so they can be reassociated.
       if (Instruction *Tmp = dyn_cast<Instruction>(Op))
----------------
spatel wrote:
> Add a line to this comment to explain why we add users of the negate to the redo list?
Will do.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:582
       // multiplies by -1 so they can be reassociated.
       if (Instruction *Tmp = dyn_cast<Instruction>(Op))
         if ((Opcode == Instruction::Mul && match(Tmp, m_Neg(m_Value()))) ||
----------------
spatel wrote:
> I know this is existing code, but for readability, I'd rename this "Neg" instead of "Tmp" and remove the partially-braced nested-ifs:
>   Instruction *Neg;
>   if ((Opcode == Instruction::Mul && match(Op, m_Neg(m_Value()))) ||
>       (Opcode == Instruction::FMul && match(Op, m_FNeg(m_Value()))) &&
>       match(Op, m_Instruction(Neg)) {
> 
Sounds good.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:587
                      << "MORPH LEAF: " << *Op << " (" << Weight << ") TO ");
-          Tmp = LowerNegateToMultiply(Tmp);
-          LLVM_DEBUG(dbgs() << *Tmp << '\n');
-          Worklist.push_back(std::make_pair(Tmp, Weight));
+          Instruction *NI = LowerNegateToMultiply(Tmp);
+          LLVM_DEBUG(dbgs() << *NI << '\n');
----------------
spatel wrote:
> NI -> Mul ?
Will do.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:591
+          for (User *U : NI->users()) {
+            if (BinaryOperator *UTmp = dyn_cast<BinaryOperator>(U))
+              ToRedo.insert(UTmp);
----------------
spatel wrote:
> UTmp -> UserBO
Will do.


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

https://reviews.llvm.org/D129612