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

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1916
+    if (Scalars == InstsToScalarize.end())
+      return false;
+    return Scalars->second.count(I);
----------------
mkuper wrote:
> Can this actually happen, or should there be an assert here?
> 
> This seems weird, because if we sometimes call this before we collect the instructions for the VF, and sometimes after, then we'll get different results?
You're right, this can be an assert. I'll update the patch. Thanks!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4678
           isa<DbgInfoIntrinsic>(&I)) &&
-        Legal->isScalarAfterVectorization(&I)) {
-      scalarizeInstruction(&I);
+        (Legal->isScalarAfterVectorization(&I) ||
+         Cost->isProfitableToScalarize(&I, VF))) {
----------------
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.





================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6546
+    // there's nothing to do.
+    if (ScalarCosts.count(I) || !TheLoop->contains(I))
+      continue;
----------------
gilr wrote:
> It seems the only instructions pushed into Worklist are PredInst and instructions from its basic block. Is the invariance check necessary?
Ah, that's right. Thanks! I think the invariance check is leftover from a previous implementation I had. I'll remove it.


https://reviews.llvm.org/D26083