[PATCH] Reassociate: reprocess RedoInsts after each instruction

[David Majnemer]

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:1958
@@ -1956,2 +1957,3 @@
   I->eraseFromParent();
-  // Optimize its operands.
+  // Optimize its operands unless it is a PHI node.
+  if(isa<PHINode>(I)) return;
----------------
I think this comment needs to be expanded a bit.  Why must we refrain from optimizing the PHI operands?

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:1959
@@ -1958,1 +1958,3 @@
+  // Optimize its operands unless it is a PHI node.
+  if(isa<PHINode>(I)) return;
   SmallPtrSet<Instruction *, 8> Visited; // Detect self-referential nodes.
----------------
Please format this a little nicer.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2269
@@ +2268,3 @@
+  // Reassociate needs for each instruction to have its operands already
+  // processed, so we first perform a "Topological" of the basic blocks so that
+  // when we process a basic block, all its dominators were processed before
----------------
Why is topological in quotes?

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2281-2284
@@ +2280,6 @@
+    // answer this question.
+    SmallSet<Instruction *, 8> Worklist;
+    for (Instruction &I : *BI) {
+      Worklist.insert(&I);
+    }
+
----------------
This would probably be better as:
  SmallSet<Instruction *, 8> Worklist(BI->begin(), BI->end());

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2299-2314
@@ -2274,10 +2298,18 @@
       }
 
-    // If this produced extra instructions to optimize, handle them now.
-    while (!RedoInsts.empty()) {
-      Instruction *I = RedoInsts.pop_back_val();
-      if (isInstructionTriviallyDead(I))
-        EraseInst(I);
-      else
-        OptimizeInst(I);
+      // If this produced extra instructions to optimize, or set already
+      // processed instructions to reprocess, handle them now.
+      while (!RedoInsts.empty()) {
+        Instruction *I = RedoInsts.pop_back_val();
+        // We only process instructions that are not existing later in the
+        // block, since these will be processing in a subsequent iteration on
+        // the basic block.
+        if (!Worklist.count(I) && &*II != I) {
+          if (isInstructionTriviallyDead(I)) {
+            EraseInst(I);
+          } else {
+            OptimizeInst(I);
+          }
+        }
+      }
     }
----------------
Instead of having both `RedoInsts` and `Worklist`, what if we had a single `SetVector` that held both?

http://reviews.llvm.org/D6995

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/