[PATCH] D27919: [Loop Vectorizer] Interleave vs Gather - in some cases Gather is better.

[Elena Demikhovsky via Phabricator via llvm-commits]

delena marked 3 inline comments as done.
delena added inline comments.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5616-5619
+  // If the memory instruction is in an interleaved group, it will be
+  // vectorized and its pointer will remain uniform.
+  if (isAccessInterleaved(I) || isLegalGatherOrScatter(I))
+    return false;
----------------
mssimpso wrote:
> This is not true now, right? An interleaved access may be scalarized based on the cost model.
It is still true. Instruction **must ** be scalarized if there is no any other option. This is "legality" check, cost model comes later.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5625-5626
+void LoopVectorizationCostModel::collectLoopUniforms(unsigned VF) {
+  if (!Uniforms[VF].empty())
+    return;
   // We now know that the loop is vectorizable!
----------------
mssimpso wrote:
> Can we change this to something like:
> 
> ```
> if (Uniforms.count(VF))
>   return;
> 
> auto &UniformsVF = Uniforms[VF];
> 
> ```
> We want to distinguish the case that (1) Uniforms have not been computed for VF from (2) Uniforms have been computed for VF but there aren't any, so we don't need to compute them again. We can end up calling this twice for the same VF if we have a user-selected VF and then compute the expected cost for interleaving. This is similar to the way we do this check in collectInstsToScalarize.
> 
> This will also apply to collectLoopScalars.
> 
Yes. I've changed.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5687
+      if (!UsersAreMemAccesses || Legal->memoryInstructionMustBeScalarized(&I) ||
+          (VF >= 2 && getWideningDecision(&I, VF) == CM_DECISION_SCALARIZE))
         PossibleNonUniformPtrs.insert(Ptr);
----------------
mssimpso wrote:
> Why not roll this check into memoryInstructionMustBeScalarized? Either way, I think this check and the check below in isVectorizedMemAccessUse that calls memoryInstructionMustBeScalarized, should be the same.
Yes, I don't need to check legality any more. It is already implied in CM decision.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:7007
   // the scalar version.
-  if (Legal->isUniformAfterVectorization(I))
+  if (VF > 1 && isUniformAfterVectorization(I, VF))
     VF = 1;
----------------
mssimpso wrote:
> The VF > 1 check is not needed because you check that condition in isUniformAfterVectorization.
Yes. You are right, thanks!


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:7466-7479
   // Insert values known to be scalar into VecValuesToIgnore. This is a
   // conservative estimation of the values that will later be scalarized.
   //
   // FIXME: Even though an instruction is not scalar-after-vectoriztion, it may
   //        still be scalarized. For example, we may find an instruction to be
   //        more profitable for a given vectorization factor if it were to be
   //        scalarized. But at this point, we haven't yet computed the
----------------
mssimpso wrote:
> Can we just delete this in favor of a helper function that checks VecValuesToIgnore and IsScalarAfterVectorization for a given VF? Something like:
> 
> ```
> bool LoopVectorizationCostModel::shouldIgnoreVecValueInCostModel(Instruction *I, unsigned VF) {
>   return VecValuesToIgnore.count(I) || isScalarAfterVectorization(I, VF);
> }
> ```
> 
> This way we won't have to be imprecise.
Yes, it is possible.


Repository:
  rL LLVM

https://reviews.llvm.org/D27919