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

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.


================
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:
> > mssimpso wrote:
> > > gilr 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?)
> > > > I think Michael's comment raises a more general question: should it matter why we scalarize?
> > > > Put differently, once an instruction is marked for scalarization by legal or cost, shouldn't we treat it uniformly in the code?
> > > > [if so, then the question "will the instruction be scalarized" would only be well-defined in the context of some VF]
> > > 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.
> That's right. There is some conservatism in collectValuesToIgnore. Currently, we place a value in VecValuesToIgnore only if we're sure (for all VFs) that the value will be scalar. But this doesn't mean that all other values will be vectorized. With this patch, we'll have a more precise determination of the known scalars after VF selection.
> 
> For the question about scalarization by legality vs. the cost model, for code generation, it's not going to matter why we decide to scalarize. In legality, we know that for any (legal) VF that we select, a value will be scalarized. In the cost model, we decide for particular VFs if it's more profitable to scalarize. When we come to code generation, we know what that VF is.
> 
> If the comment is that InnerLoopVectorizer shouldn't need to make a distinction between Legal and Cost scalarization, then we can create a helper function like Michael suggested for InnerLoopVectorizer to use. What do you think?
That sounds good to me.
I think eventually we'll probably need to keep a full "this will get scalarized" list per-VF instead of the current VecValuesToIgnore, but that's an issue for another patch.

(By the way, do we really expect to see cases where scalarization is better for some VFs, but worse for others?)


https://reviews.llvm.org/D26083