[PATCH] D51313: [LV] Fix code gen for conditionally executed uniform loads

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5877
       if (isa<StoreInst>(&I) && isScalarWithPredication(&I))
         NumPredStores++;
+
----------------
Also this seems inaccurate - which was how I found the bug in the uniform store handling.
`NumPredStores` is really both scatter and (branch + store) predicated stores. A better name would be `NumScalarPredStores`





================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5883
+          // gather/scatters are not considered scalar with predication.
+          !Legal->blockNeedsPredication(I.getParent())) {
         // Scalar load + broadcast
----------------
Ayal wrote:
> Indeed scalarizing a conditional load from a uniform address is currently broken, when it can turn into a gather. The culprit is that VPReplicateRecipe, when constructed for such instructions, uses an incorrect IsPredicate = isScalarWithPredication(). Which, as noted, is false for loads and stores of types that can be gathered/scattered. I.e., really means mustBeScalarWithPredication. (That, on top of IsUniform which is also incorrect, causing replication as noted in D50665#1209958.)
> 
> This proposed fix to refrain from scalarizing such loads, should** make sure they always turn into a gather**, rather than rely on the cost-based gather-vs-scalarize decision below.
> 
> The above holds also for conditional loads from **non-uniform** addresses, that can turn into gathers, but possibly also get incorrectly scalarized w/o branches. It's hard to test, as the scalarization cost needs to be better than the gather for this to occur. But best make sure these also always turn into a gather.
> 
> It would also/alternatively be good to fix the scalarization of such loads (by correcting IsPredicate), and enable cost-based gather-vs-scalarize decision. Presumably gather should always win, unless perhaps if the load feed scalarized users.
It seems to me that we need a new function `LoopVectorizationCostModel::isPredicated` which is a superset of instructions contained by `LoopVectorizationCostModel::isScalarWithPredication`? 
So, the 'complete' fix here is to correct the costmodel as done above *and* fix `isPredicate` in `VPRecipeBuilder::handleReplication`:
```
bool IsPredicated = CM.isPredicated(I);
auto *Recipe = new VPReplicateRecipe(I, IsUniform, IsPredicated);
```

Essentially, whether we use a gather/scatter or we do plain old branch + load/store, (because we don't have gather/scatter support), both are predicated - one is a 'vector predicate', other is scalarized + predicated.



Repository:
  rL LLVM

https://reviews.llvm.org/D51313