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

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:97
@@ +96,3 @@
+  void reorderAfter(Instruction *I);
+  void reorderAfterHelper(Instruction* I, std::unordered_set<Instruction*>& InstructionsToMove);
+  void reorderBefore(Instruction *I);
----------------
Where do we use reorderAfter?  Should one of the calls to reorderBefore call reorderAfter?  If so, why aren't some of the tests failing?   :)

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:99
@@ -95,1 +98,3 @@
+  void reorderBefore(Instruction *I);
+  void reorderBeforeHelper(Instruction* I, std::unordered_set<Instruction*>& InstructionsToMove);
 
----------------
"&" should bind to the variable name.

(It's helpful to me to run clang-format as part of "arc diff", so I don't ever forget this stuff.  I have a wrapper script around arc that invokes git-clang-format before posting a change. https://github.com/jlebar/conf/blob/master/bin/arc  In fact I call my wrapper "arc" so I don't even have to think about it.  You'll need a file/symlink in the same directory as that script called arc-real, and you'll need git-clang-format on your path -- it's in the LLVM source tree at tools/clang/tools/clang-format/git-clang-format.  I think you'll also need to add

```
[clangformat]
        style = "file"
```

to your .gitconfig.

Easy, right?  :)

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:342
@@ -338,1 +341,3 @@
+void Vectorizer::reorderAfterHelper(Instruction* I,
+                               std::unordered_set<Instruction*> &InstructionsToMove) {
   for (User *U : I->users()) {
----------------
Whitespace alignment here.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:350
@@ +349,3 @@
+        InstructionsToMove.insert(IM);
+        reorderAfterHelper(IM, InstructionsToMove);
+    }
----------------
This is arbitrarily-deep recursion, which I think we tend to avoid, for fear of overflowing the stack given pessimal inputs.  Can we write this with an explicit worklist instead?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:356
@@ +355,3 @@
+void Vectorizer::reorderAfter(Instruction *I) {
+  std::unordered_set<Instruction*> InstructionsToMove;
+  reorderAfterHelper(I, InstructionsToMove);
----------------
Can we use llvm::SmallPtrSet?  unordered_set is much slower and cache-inefficient.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:359
@@ +358,3 @@
+
+  Instruction* InsertAfter=I;
+  for (auto IM = I->getParent()->begin(), E = I->getParent()->end(); IM != E; ++IM) {
----------------
Space around "=" (clang-format should take care off this, too).  Here and elsewhere.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:360
@@ +359,3 @@
+  Instruction* InsertAfter=I;
+  for (auto IM = I->getParent()->begin(), E = I->getParent()->end(); IM != E; ++IM) {
+    if (!is_contained(InstructionsToMove, &*IM))
----------------
Do we know that I->getParent()->end() can't change when we insert new elements?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:360
@@ +359,3 @@
+  Instruction* InsertAfter=I;
+  for (auto IM = I->getParent()->begin(), E = I->getParent()->end(); IM != E; ++IM) {
+    if (!is_contained(InstructionsToMove, &*IM))
----------------
jlebar wrote:
> Do we know that I->getParent()->end() can't change when we insert new elements?
This only considers instructions in I's BB, but InstructionsToMove may contain other instructions, no?  If so that may make this whole thing more complicated...

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:367
@@ +366,3 @@
+    InstructionToMove->insertAfter(InsertAfter);
+    InstructionsToMove.erase(&*InstructionToMove);
+    InsertAfter=InstructionToMove;
----------------
If you're going to do this, maybe we should assert that InstructionsToMove is empty after the loop?

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:367
@@ +366,3 @@
+    InstructionToMove->insertAfter(InsertAfter);
+    InstructionsToMove.erase(&*InstructionToMove);
+    InsertAfter=InstructionToMove;
----------------
jlebar wrote:
> If you're going to do this, maybe we should assert that InstructionsToMove is empty after the loop?
Do we need the `&*`?  I'd think it should work without that.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:925
@@ -872,2 +924,3 @@
     SmallVector<Instruction *, 16> InstrsToReorder;
+    InstrsToReorder.push_back(dyn_cast<Instruction>(Bitcast));
 
----------------
We use dyn_cast when the cast may fail and return null.  But I think you don't want a null pointer inside InstrsToReorder, and we know that Bitcast is an Instruction, so I think you want plain cast<>.

================
Comment at: test/Transforms/LoadStoreVectorizer/AMDGPU/insertion-point.ll:12
@@ -12,2 +11,3 @@
+; CHECK: %w = add i32 %y, 9
 ; CHECK: %foo = add i32 %z, %w
 define void @insert_load_point(float addrspace(1)* nocapture %a, float addrspace(1)* nocapture %b, float addrspace(1)* nocapture readonly %c, i64 %idx, i32 %x, i32 %y) #0 {
----------------
I don't quite get what the original code is testing here.  Like, the adds are completely independent of the loads, right?  If so, can we fix this test so it's not sensitive to implementation details?

================
Comment at: test/Transforms/LoadStoreVectorizer/X86/correct-order.ll:16
@@ +15,3 @@
+
+"for something":
+  %index = phi i64 [ 0, %entry ], [ %index.next, %"for something" ]
----------------
Do all these tests need to be inside loops?

================
Comment at: test/Transforms/LoadStoreVectorizer/X86/correct-order.ll:17
@@ +16,3 @@
+"for something":
+  %index = phi i64 [ 0, %entry ], [ %index.next, %"for something" ]
+  %next.gep = getelementptr i32, i32* %ptr, i64 %index
----------------
Do we have a test which checks that we don't reorder through phi nodes?


http://reviews.llvm.org/D22071