[PATCH] D22071: Correct ordering of loads/stores.

[Justin Lebar via llvm-commits]

jlebar added a comment.

Can you elaborate a bit more in the commit message exactly what bug we're fixing here?  It's not immediately clear to me.


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:95
@@ -94,1 +94,3 @@
+  void reorderBeforeHelper(Instruction *I,
+                           SmallPtrSet<Instruction *, 16> &InstructionsToMove);
 
----------------
Maybe "reorderUsers" or "moveUsers" would be a better name, especially since we no longer have reorderAfter?  Or even just "reorder", I guess.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:342
@@ +341,3 @@
+  SmallVector<Instruction *, 16> Worklist;
+  Worklist.insert(Worklist.end(), I);
+  while (Worklist.size()) {
----------------
push_back

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:343
@@ +342,3 @@
+  Worklist.insert(Worklist.end(), I);
+  while (Worklist.size()) {
+    auto LastElement = Worklist.end();
----------------
`!Worklist.empty()` (I think?)

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:347
@@ +346,3 @@
+    I = *LastElement;
+    Worklist.erase(LastElement);
+    int NumOperands = I->getNumOperands();
----------------
`I = Worklist.pop_back_elem();`

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:356
@@ +355,3 @@
+        InstructionsToMove.insert(IM);
+        Worklist.insert(Worklist.end(), IM);
+      }
----------------
push_back

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:364
@@ +363,3 @@
+  SmallPtrSet<Instruction *, 16> InstructionsToMove;
+  reorderBeforeHelper(I, InstructionsToMove);
+
----------------
Since this is no longer recursive, we don't need the helper anymore?  We can keep it if you think it's a useful way to break things down (I'm not sure it is), but then we should change the name so it's more descriptive and have it return the SmallPtrSet instead of take it by reference.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:367
@@ +366,3 @@
+  for (auto IM = I->getParent()->begin(), E = I->getParent()->end(); IM != E;
+       ++IM) {
+    if (!is_contained(InstructionsToMove, &*IM))
----------------
Could we call this iterator BBI, and then call the instruction to move IM?  That would be consistent with the helper, and also would fix the problem of InstructionToMove and InstructionsToMove looking very similar.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:374
@@ +373,3 @@
+    InstructionToMove->insertBefore(I);
+    InstructionsToMove.erase(InstructionToMove);
+  }
----------------
Now that I think about this, could we just have a DEBUG() loop that checks that every element of InstructionsToMove is in the same BB as I?  Then we don't need to erase from the set, which is overhead we don't need.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:377
@@ +376,3 @@
+  // This failure can occur if the helper marks for reordering instructions
+  // outside this BB
+  assert(InstructionsToMove.size() == 0 &&
----------------
Nit, end sentences with periods.

Also suggest swapping the order of the prepositional phrases; this order is awkward (although I can't articulate a rule :).

================
Comment at: test/Transforms/LoadStoreVectorizer/AMDGPU/insertion-point.ll:7
@@ -6,2 +6,3 @@
 ; existing adds.
+; Vectorized loads should be inserted at the position of the first load.
 
----------------
Please reflow.  Also thanks for adding this; I now understand why the test output is what it is.  That makes me happy.


http://reviews.llvm.org/D22071