[PATCH] D12345: [Reassociate]: Add intermediate subtract instructions created while negating to be redone later for more reassociate opportunities

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:613
@@ -610,3 +612,3 @@
         // No uses outside the expression, try morphing it.
-      } else if (It != Leaves.end()) {
+      } else {
         // Already in the leaf map.
----------------
I assume this condition was removed because it will always be true, correct?  If so, please commit this in isolation.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:1931
@@ -1928,1 +1930,3 @@
 
+// Remove the dead instructions and if it's operands are trivially dead
+// Add them to Ins as well to remove them.
----------------
How about:

// Remove dead instructions and if any operands are trivially dead add them to
// Insts so they will be removed as well.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:1934
@@ +1933,3 @@
+void Reassociate::RecursivelyEraseDeadInsts(
+    Instruction *I, SetVector<AssertingVH<Instruction>> &Ins) {
+  assert(isInstructionTriviallyDead(I) && "Trivially dead instructions only!");
----------------
Ins -> Insts

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:1936
@@ +1935,3 @@
+  assert(isInstructionTriviallyDead(I) && "Trivially dead instructions only!");
+  SetVector<Value *> Ops(I->op_begin(), I->op_end());
+  ValueRankMap.erase(I);
----------------
Can we use a for loop to loop over all the Instruction operands, rather than pushing/poping each operand onto/off of a SetVector?

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2280
@@ +2279,3 @@
+    // Make a copy of all the instructions to be redone so we can remove dead
+    // instructions
+    SetVector<AssertingVH<Instruction>> ToRedo(RedoInsts);
----------------
Please add a period.  Comments should be written with proper capitalization, punctuation, etc.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2282
@@ +2281,3 @@
+    SetVector<AssertingVH<Instruction>> ToRedo(RedoInsts);
+    // First run through and remove all the dead instructions.
+    // As the order of traversal of these instructions are not guaranteed,
----------------
How about something like:

// Iterate over all instructions to be reevaluated and remove trivially dead
// instructions.  If any operand of the trivially dead instruction becomes
// dead mark it for deletion as well.  Continue this process until all trivially
// dead instructions have been removed.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2294
@@ -2259,1 +2293,3 @@
+    // Now that we have removed dead instructions, we can reoptimize the
+    // remaining instructions
     while (!RedoInsts.empty()) {
----------------
Please add a period.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2299
@@ -2262,3 +2298,3 @@
         EraseInst(I);
-      else
+      else {
         OptimizeInst(I);
----------------
Please don't add extra curly brackets.


http://reviews.llvm.org/D12345