[PATCH] D26083: [LV] Scalarize operands of predicated instructions

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

In https://reviews.llvm.org/D26083#585225, @mssimpso wrote:

> - Added call to collectInstsToScalarize for user-selected VFs (I realized we needed this with the new assert)


Yay, finally being pedantic and asking for an assert pays off! :-)



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4678
           isa<DbgInfoIntrinsic>(&I)) &&
-        Legal->isScalarAfterVectorization(&I)) {
-      scalarizeInstruction(&I);
+        (Legal->isScalarAfterVectorization(&I) ||
+         Cost->isProfitableToScalarize(&I, VF))) {
----------------
mssimpso wrote:
> mkuper wrote:
> > We have 5 places that call isScalarAfterVectorization().
> > Is this the only call site that cares about this? (If all of them should care, perhaps wrap in a helper function?)
> You're asking if all places where we call isScalarAfterVectorization should also be concerned with isProfitableToScalarize? I think a helper makes sense in general, but this is the only case (at least currently) where I think it will make a difference.
> 
> In needsScalarInduction and widenIntInduction, the IVs shouldn't be predicated so it shouldn't make a difference. And in collectValuesToIgnore, we haven't yet computed the instruction costs.
> 
> 
> 
> And in collectValuesToIgnore, we haven't yet computed the instruction costs.
So, we get some imprecision here, right? If we end up scalarizing things that aren't in VecValuesToIgnore then we'll overestimate the register pressure? Or am I confused?

Anyway, assuming I got it right - I'm not saying we need to fix this in this patch. The patch, as it is, is probably still a strict improvement. But a FIXME would be good.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1962
 
+  /// A map holding instruction-cost pairs for each choice of vectorization
+  /// factor. The presence of an instruction in the mapping indicates that it
----------------
Maybe make more explicit that the cost is the cost of the instruction if scalarized? ("instruction-cost pairs for each choice of vectorization factor" seems to imply the vectorized cost.)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1968
+
+  // Returns the expected difference in cost from scalarizing the expression
+  // feeding a predicated instruction \p PredInst. The instructions to
----------------
This can also be more explicit. "A negative returned value implies..."


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6154
     Factor.Width = UserVF;
+    collectInstsToScalarize(UserVF);
     return Factor;
----------------
Could you add a test for this?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6570
+  // Initialize a mapping for VF in InstsToScalalarize. If we find that it's
+  // not profitable to to scalarize any instructions, the presence of VF in the
+  // map will indicate that we've analyzed it already.
----------------
"to to" -> "to"


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6583
+        DenseMap<Instruction *, unsigned> ScalarCosts;
+        if (computePredInstDiscount(&I, ScalarCosts, VF) >= 0)
+          InstsToScalarizeVF.insert(ScalarCosts.begin(), ScalarCosts.end());
----------------
Just trying to understand if I got this right.
Let's say we have:

```
%a = add i32, ...
%r = udiv i32 %x, %a
%s = udiv i32 %y, %a
%t = udiv i32 %z, %a
```
In the same block. 
Will we do the right thing evaluating the cost of scalarizing the add in a separate context for each udiv?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6624
+    if (Legal->isScalarWithPredication(I) && !I->getType()->isVoidTy())
+      ScalarCost += getScalarizationOverhead(ToVectorTy(I->getType(), VF), true,
+                                             false, TTI);
----------------
Can we end up with a non-scalar type during the traversal?
I'm thinking about something like a div that depends on an extractvalue from a struct.


https://reviews.llvm.org/D26083