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

[Alina Sbirlea via llvm-commits]

asbirlea added a comment.

Still looking into updating the tests and some of the comments I missed.


================
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);
----------------
Fair point. I need to figure out if there si a case when reorderAfter is needed for stores. In the mean time, removing it.

================
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);
 
----------------
Yes. Postponing running clang-format until all other comments are addressed.
I also have another patch on top of this which may lead to conflicts after formatting, in which case I will clang-format the next patch.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:368
@@ +367,3 @@
+    InstructionToMove->insertAfter(InsertAfter);
+    InstructionsToMove.erase(&*InstructionToMove);
+    InsertAfter=InstructionToMove;
----------------
All this was removed, but I use the comments for reorderBefore.

================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:928
@@ -875,2 +927,3 @@
     SmallVector<Instruction *, 16> InstrsToReorder;
+    InstrsToReorder.push_back(dyn_cast<Instruction>(Bitcast));
 
----------------
Yes, updated.

================
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 {
----------------
jlebar wrote:
> 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?
I'm not sure what the original purpose was. It looks to me it is intentionally testing an implementation detail ("insert_load_point").

================
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
----------------
jlebar wrote:
> Do we have a test which checks that we don't reorder through phi nodes?
I don't think so. Feel free to add one :).


http://reviews.llvm.org/D22071