[PATCH] D21755: Refine the set of UniformAfterVectorization instructions

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

The logic LGTM, now that I understand it. :-)
A few nits about the code inline.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4900
@@ -4901,3 +4899,3 @@
 
-  // Start with the conditional branch and walk up the block.
-  Worklist.push_back(Latch->getTerminator()->getOperand(0));
+  // If V is not an instruction inside current loop, it is a Value
+  // outside of scope where we are interesting in.
----------------
"inside current" -> "inside the current"

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4901
@@ +4900,3 @@
+  // If V is not an instruction inside current loop, it is a Value
+  // outside of scope where we are interesting in.
+  auto isOutOfScope = [&](Value *V) -> bool {
----------------
"scope where" -> "the scope which"

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4904
@@ +4903,3 @@
+    Instruction *I = dyn_cast<Instruction>(V);
+    if (!I || !TheLoop->contains(I))
+      return true;
----------------
Why not just "return (!I || !TheLoop->contains(I))"?
But I'm ok with this if you think it's clearer.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4909
@@ +4908,3 @@
+
+  SetVector<Value *> Worklist;
+  BasicBlock *Latch = TheLoop->getLoopLatch();
----------------
Can this be a SetVector<Instruction *>?
I think all of your inserts are protected by isOutOfScope(), so you can never have a non-instruction Value there now, right?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4917
@@ -4908,2 +4916,3 @@
+  // after vectorization (and subsequent cleanup).
   for (Loop::block_iterator B = TheLoop->block_begin(),
                             BE = TheLoop->block_end();
----------------
Can you use a range for here, iterating over TheLoop->blocks()?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4920
@@ +4919,3 @@
+       B != BE; ++B) {
+    for (BasicBlock::iterator I = (*B)->begin(), IE = (*B)->end(); I != IE;
+         ++I) {
----------------
Range for here too?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4971
@@ -4927,3 +4970,3 @@
 
-    // Insert all operands.
-    Worklist.insert(Worklist.end(), I->op_begin(), I->op_end());
+  for (auto UV : Worklist) {
+    Instruction *UI = cast<Instruction>(UV);
----------------
Can you do something like Uniforms.insert(Worklist.begin(), Worklist.end()) instead?
We can move the debug print to where things are added to Worklist - I think it may be more useful there regardless, since it will then print the uniforms in the order that they are found.


Repository:
  rL LLVM

http://reviews.llvm.org/D21755