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

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6583
+        DenseMap<Instruction *, unsigned> ScalarCosts;
+        if (computePredInstDiscount(&I, ScalarCosts, VF) >= 0)
+          InstsToScalarizeVF.insert(ScalarCosts.begin(), ScalarCosts.end());
----------------
mssimpso wrote:
> gilr wrote:
> > mkuper wrote:
> > > 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?
> > Actually in such a case it seems the addition gets scalarized but cannot be sunk down because of the multiple uses.
> > I also wonder if we calculate the cost correctly when one predicated instruction feeds another.
> Yeah, Gil is right here in that the add in this example will not be sunk. Thanks, Gil! This is because our current predication method places each udiv in it's own separate block. Long term I think we will want to keep the predicated instructions in the same block after vectorization if they were in the same block before vectorization. I can add some comments about this in the meantime.
> 
> So we will need to check for the multi-context case, then. But this doesn't mean that we can't sink instructions with multi-uses. We should still be able to handle the following, for example.
> 
> ```
> if:
>   %a = add i32, ...
>   %r = add i32 %a, %a
>   %s = udiv i32 %x, %r
>   br
> ```
> 
> I'll add some tests.
Michael/Gil,

After thinking about the multi-context case a bit more (and not wanting to revisit this again later), I decided to take a stab at modifying code generation for the predicated instructions so that we don't place them in separate basic blocks if we don't have to. I'll upload this as a separate patch shortly.

Matt.


https://reviews.llvm.org/D26083