[PATCH] D21755: Refine the set of UniformAfterVectorization instructions

[Wei Mi via llvm-commits]

wmi added inline comments.

================
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.
----------------
mkuper wrote:
> "inside current" -> "inside the current"
Fixed.

================
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 {
----------------
mkuper wrote:
> "scope where" -> "the scope which"
Fixed.

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

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4909
@@ +4908,3 @@
+
+  SetVector<Value *> Worklist;
+  BasicBlock *Latch = TheLoop->getLoopLatch();
----------------
mkuper wrote:
> 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?
It can be SetVector<Instruction *>, but then we need to use cast<instruction> before adding values into Worklist or checking whether users are already in Worklist.

================
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();
----------------
mkuper wrote:
> Can you use a range for here, iterating over TheLoop->blocks()?
Fixed.

================
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) {
----------------
mkuper wrote:
> Range for here too?
Fixed.

================
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);
----------------
mkuper wrote:
> 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.
If I use Uniforms.insert(Worklist.begin(), Worklist.end()), The elem type of Worklist should be "Instruction *" instead of "Value *". I feel that will increase the number of cast<Instruction> needed. I am not sure which way is better. Either way is fine with me. I updated the patch according to suggestion here. 


Repository:
  rL LLVM

http://reviews.llvm.org/D21755