[PATCH] D6995: Reassociate: reprocess RedoInsts after each instruction

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Sorry, this one fell completely off my radar.

Per Quentin/Eric, I would merge all the tests into a single file.  Quentin's suggestion to use the instnamer seems fairly reasonable as well.

I'd like to hear David's comments before providing a LGTM as he has actually reviewed this patch in earnest.

I assume this should also be merged into the 3.8 branch, if we're actually crashing without this fix.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2262
@@ +2261,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
----------------
A "Topological" what?  A "Topological" ranking... The sentences seems incomplete.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2263
@@ +2262,3 @@
+  // 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
+  ReversePostOrderTraversal<Function *> RPOT(&F);
----------------
were processed before -> have been processed. (Or something similar)

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2281
@@ +2280,3 @@
+    SmallSet<Instruction *, 8> Worklist;
+    for (Instruction &I : *BI) {
+      Worklist.insert(&I);
----------------
No need for extra curly brackets.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2310
@@ +2309,3 @@
+            RankMap[I->getParent()] <= RankMap[BI]) {
+          if (isInstructionTriviallyDead(I)) {
+            EraseInst(I);
----------------
No need for extra curly brackets.


http://reviews.llvm.org/D6995