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

[Justin Lebar via llvm-commits]

jlebar added a comment.

OK, I'll have another look once you've figured the rest out.  Just lmk.


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:97
@@ -95,3 +96,3 @@
 
   /// Returns the first and the last instructions in Chain.
   std::pair<BasicBlock::iterator, BasicBlock::iterator>
----------------
> in which case I will clang-format the next patch.

I can help you resolve the conflicts if it gets hairy (I have some ideas for rebase tricks), but it's a requirement each commit be properly clang-formatted.  We can't commit improperly-formatted code.

================
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 {
----------------
> It looks to me it is intentionally testing an implementation detail ("insert_load_point").

Looks like it to me, too.  When we committed the original patches, we all agreed that we wouldn't act with a bias towards the existing code, since we committed with existing unresolved issues.  I think this should count under that rubric.

That is, can we fix the test so it no longer tests an implementation detail?  I suppose you don't need to do that in this patch if you don't want.

================
Comment at: test/Transforms/LoadStoreVectorizer/X86/correct-order.ll:18
@@ +17,3 @@
+  %index = phi i64 [ 0, %entry ], [ %index.next, %"for something" ]
+  %next.gep = getelementptr i32, i32* %ptr, i64 %index
+  %a1 = add nsw i64 %index, 1
----------------
You don't think it's worth adding a test as part of this patch?  It seems relevant because we could otherwise get infinite recursion or something...


http://reviews.llvm.org/D22071