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

[Elena Demikhovsky via Phabricator via llvm-commits]

delena marked 2 inline comments as done.
delena added a comment.

In https://reviews.llvm.org/D27919#662246, @mssimpso wrote:

> Hi Elena,
>
> Here are some more inline comments. Also, please clang-format the patch if you haven't already done so to make the review easier. Thanks!


Done.



================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:1930
+  enum InstWidening {
+    CM_DECISION_NONE,
+    CM_DECISION_WIDEN,
----------------
mssimpso wrote:
> How are we using CM_DECISION_NONE? Aren't we forced to make some sort of decision? I would think the default would be CM_DECISION_WIDEN unless the cost model points to one of the others.
I used it while capturing decisions for an interleave group. At this point I can get rid of it. I return "NONE" if no decision, and it is convenient.
  InstWidening getWideningDecision(Instruction *I, unsigned VF) {
      assert(VF >= 2 && "Expected VF >=2");
      std::pair<Instruction *, unsigned> InstOnVF = std::make_pair(I, VF);
      if (!WideningDecisions.count(InstOnVF))
        return CM_DECISION_NONE;
      return WideningDecisions[InstOnVF];
    }
Then I use it in assertion, to make sure that decision is made.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5503-5504
 
+  if (VF < 2 || Scalars.count(VF))
+    return;
   // If an instruction is uniform after vectorization, it will remain scalar.
----------------
mssimpso wrote:
> Since you've added a new check in collectUnifomsAndScalars to ensure the analysis is performed only once, the check here and in collectLoopUniforms is redundant. Should these be asserts now?
May be. Until somebody will decide to call these functions separately. Right now I don't see a reason. I'll put an "assert" and add a comment.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5556-5563
+bool LoopVectorizationCostModel::hasConsecutiveLikePtrOperand(Instruction *I,
+                                                              unsigned VF) {
+  if (VF > 1 && getWideningDecision(I, VF) == CM_DECISION_INTERLEAVE)
     return true;
   if (auto *Ptr = getPointerOperand(I))
-    return isConsecutivePtr(Ptr);
+    return Legal->isConsecutivePtr(Ptr);
   return false;
----------------
mssimpso wrote:
> I don't think a see a real use for this function anymore. Please see my related comment about memoryAccessMustBeScalarized. This function was only ever used in collectLoopUniforms to help determine how a memory access would be vectorized. I think you can probably greatly simplify the logic in collectLoopUniforms and remove it. Now, we know what the vectorizer will do based the the cost model decision. 
> 
> For a GEP to remain uniform, I think we just need to know that all its users are CM_DECISION_INTERLEAVE or CM_DECISION_WIDEN. Is this right? If so, please work it into the GEP part of collectLoopUniforms.
I removed mustBeScalarized(). (I suppose you meant this function, the lines are mixed up)


Repository:
  rL LLVM

https://reviews.llvm.org/D27919