[PATCH] D52656: [LV] Teach vectorizer about variant value store into uniform address

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Thanks, added only minor optional suggestions.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1181
   /// element)
-  /// TODO: Test the extra cost of the extract when loop variant value stored.
   unsigned getUniformMemOpCost(Instruction *I, unsigned VF);
----------------
Check the cost estimates printed in debugging?


================
Comment at: test/Transforms/LoopVectorize/X86/illegal-parallel-loop-uniform-write.ll:7
+; CHECK-LABEL: @foo(
+; CHECK: <4 x i32>
+; CHECK: ret void
----------------
anna wrote:
> Ayal wrote:
> > CHECK how LV actually vectorizes the invariant store, rather than if it emits a vector type?
> > 
> > Would be good to include a test having two conditional invariant stores.
> > CHECK how LV actually vectorizes the invariant store, rather than if it emits a vector type?
> 
> LV vectorizing the invariant store in this case is undefined behaviour because the user incorrectly tagged the loop as parallel. Pls see the PR15794 mentioned below. 
> 
> Do we still need to see how LV vectorizes the invariant store because the code generated is anyway incorrect in the presence of output dependency. See test case below which is the same test case without the incorrect mem_parallel.loop metadata and we don't vectorize it.
> 
> > Would be good to include a test having two conditional invariant stores.
> Will do in the invariant-store-vectorization case below.
>> CHECK how LV actually vectorizes the invariant store, rather than if it emits a vector type?
>> 
>LV vectorizing the invariant store in this case is undefined behaviour because the user incorrectly tagged the loop as parallel. Pls see the PR15794 mentioned below.

Ahh, right; so indeed no point in checking how LV vectorizes the invariant store.


>>Would be good to include a test having two conditional invariant stores.
>>
>Will do in the invariant-store-vectorization case below.

Very good, thanks. It may also be good to test that LV does not vectorize two conditional stores (of variant values) to the same invariant address, in addition to  @inv_val_store_to_inv_address_conditional_diff_values which checks for two stores of invariant values to the same invariant address.


Repository:
  rL LLVM

https://reviews.llvm.org/D52656