[PATCH] Reassociate: reprocess RedoInsts after each instruction

[Mehdi AMINI]

Thanks for your comment. I'll update the patch soon. I found out that I could use the RankMap to simplify what I'm trying to achieve.
I also found another but where we end up with an instruction moved to a new block (and hit an assertion).


================
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;
----------------
majnemer wrote:
> I think this comment needs to be expanded a bit.  Why must we refrain from optimizing the PHI operands?
Because the operand might be locate in a block that wasn't already processed.
I'll relax that by allowing operands that are located in block already processed.

================
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
----------------
majnemer wrote:
> Why is topological in quotes?
Because we don't have a DAG.

================
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);
+    }
+
----------------
majnemer wrote:
> This would probably be better as:
>   SmallSet<Instruction *, 8> Worklist(BI->begin(), BI->end());
That was my first intention, but it seems that SmallSet does not provide such constructor. 

I also tried:


```
    SmallSet<Instruction *, 8> Worklist;
    Worklist.insert(BI->begin(), BI->end());

```

But was bitten by: "candidate function not viable: no known conversion from 'llvm::Instruction' to 'llvm::Instruction *' for 1st argument; take the address of the argument with &"

================
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);
+          }
+        }
+      }
     }
----------------
majnemer wrote:
> Instead of having both `RedoInsts` and `Worklist`, what if we had a single `SetVector` that held both?
I don't see how would it be possible?

http://reviews.llvm.org/D6995

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