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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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



================
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))
----------------
Add a line to this comment to explain why we add users of the negate to the redo list?


================
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()))) ||
----------------
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)) {



================
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');
----------------
NI -> Mul ?


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


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

https://reviews.llvm.org/D129612