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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

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

> Hi Elena,
>
> I'm not really sure what the best way to fix this would be. Any thoughts?  One way would be to remove scalarization from your widening decision.


That sounds a bit unfortunate. I think we'd rather like to the cost model to be able to say "please scalarize this".



================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:1702
+  /// Decision that was taken during cost calculation for memory instruction.
+  enum InstWidening {
+    LV_NONE,
----------------
I'm not a fan of the name of this enum, but don't really have any good ideas. Anyone else?


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:1725
+    for (unsigned i = 0; i < Grp->getFactor(); ++i) {
+      if (auto *I = Grp->getMember(i))
+        WideningDecisions[std::make_pair(I, VF)] = W;
----------------
This looks like a suboptimal way to iterate over InterleaveGroup, because it requires a lookup for every index, instead of just iterating above the Members collection. (we don't care about the order here, right?)
Perhaps add a better interface?


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:1731
+  /// Return the cost model decision for the given instruction \p I and vector
+  /// width \p VF. Return LV_NONE if this instruction did not pass through the cost
+  /// modeling.
----------------
Is this really a per-instruction thing, or do we just always end up with LV_NONE when the cost model is not in use (e.g. explicit user-provided VF).

If the former, when does this happen?
If the latter, then I think the comment is a bit misleading - and it would be good to be able to assert on this happening only when there's no cost model.

Edit: Oh, I think I see, you care about temporary LV_NONE values for Interleave groups. I still think it'd be nice if we could somehow distinguish between the cases, so we could assert on not having a cost when we should.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2878
+    Legal->getCostModelDecision(Instr, VF);
+  bool NoCostModelDecision =  (Decision == LoopVectorizationLegality::LV_NONE);
+  
----------------
Extra space after =.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2891
+
   Type *ScalarDataTy = LI ? LI->getType() : SI->getValueOperand()->getType();
   Type *DataTy = VectorType::get(ScalarDataTy, VF);
----------------
assert(!Legal->memoryInstructionMustBeScalarized(Instr, VF)), maybe?

Because now we leave the door open for the cost model decision being to widen, even though memoryInstructionMustBeScalarized(). 
Another option is to replace the if with:

```
if (Decision == LoopVectorizationLegality::LV_SCALARIZE ||     
    Legal->memoryInstructionMustBeScalarized(Instr, VF))
```

But I'm not sure it makes sense, because there's no good reason to always call memoryInstructionMustBeScalarized() in a non-asserts build - in theory, the cost model should have already checked this.


================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:7168
+        // Only calculate the cost once for the group.
+        InterleaveGrpCost = getInterleaveGroupCost(I, VF);
+
----------------
Wouldn't you calculate it several times per group if it's unprofitable?


================
Comment at: ../test/Analysis/CostModel/X86/interleave-load-i32.ll:13
 ;CHECK-LABEL: load_i32_interleave4
-;CHECK: Found an estimated cost of 1 for VF 1 For instruction:   %0 = load
-;CHECK: Found an estimated cost of 5 for VF 2 For instruction:   %0 = load
----------------
What happened to the VF = 1 cost?


Repository:
  rL LLVM

https://reviews.llvm.org/D27919