[PATCH] D91398: [LoopVectorizer] Lower uniform loads as a single load (instead of relying on CSE)

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2667
+    if (!Operand || !OrigLoop->contains(Operand) ||
+        (Cost->isUniformAfterVectorization(Operand, State.VF)))
+      InputInstance.Lane = 0;
----------------
fhahn wrote:
> The VPRecplicateRecipe contains a `IsUniform` flag. I think it should be possible to pass the flag through from the recipe to `scalarizeInstruction`. Ideally the recipes should contain all information required for code-generation to avoid having to tie code generation directly to the cost-model.
I think you're misreading the code slightly.  This isn't checking whether the recipe for the instruction being scalarized is uniform.  It's checking whether the *input* to the instruction is uniform.  

It seems like overkill to record a per operand uniform flag in the recipe?  


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6272
                                                          ElementCount VF) {
+  assert(Legal->isUniformMemOp(*I));
+
----------------
fhahn wrote:
> nit: message for assert?
Aside from being pedantic, why?  I'm happy to comply, but I don't really see any value in cases like this where it's obvious from context.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6451
 
-      if (Legal->isUniform(Ptr) &&
-          // Conditional loads and stores should be scalarized and predicated.
-          // isScalarWithPredication cannot be used here since masked
-          // gather/scatters are not considered scalar with predication.
-          !Legal->blockNeedsPredication(I.getParent())) {
+      if (Legal->isUniformMemOp(I)) {
         // TODO: Avoid replicating loads and stores instead of
----------------
fhahn wrote:
> this seems an unrelated refactoring, which could be split out and committed independently?
Happy to do so since you seem to think the interface made sense.  I'm very new to vectorizer code and didn't want to jump to that conclusion.  :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91398/new/

https://reviews.llvm.org/D91398