[PATCH] D115329: [LoopVectorize] Pass a vector type to isLegalMaskedGather/Scatter

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1590
+      Ty = VectorType::get(Ty, VF);
     return (LI && TTI.isLegalMaskedGather(Ty, Align)) ||
            (SI && TTI.isLegalMaskedScatter(Ty, Align));
----------------
david-arm wrote:
> According to the default `VF = ElementCount::getFixed(1)` it looks like we can still have an inconsistency during vectorisation where we get different answers? I think it's also called from `collectElementTypesForWidening` so we could potentially say a gather is legal when the type is scalar, but then say it's illegal in setCostBasedWideningDecision. I wonder if it's worth adding a comment there saying something like:
> 
>   // We're simply querying at this point if the target even supports any vector
>   // gathers and scatters for the given element type? Certain vector forms may
>   // still be illegal, but setCostBasedWideningDecision will distinguish between
>   // the legal behaviour for different VFs at that point and generate costs accordingly.
> 
> Also, does it matter that `LoopVectorizationCostModel::isScalarWithPredication` is still passing in an element type rather than a vector? For example, `collectLoopUniforms` calls `isScalarWithPredication` and at that point knows the VF. It may be fine, but I'm just a bit worried we might be swapping one inconsistency for another that's all.
The code in `collectElementTypesForWidening` seems like a hack to get the LV to choose a wider VF and it seems specific to X86. If I replace that whole expression with `if (!T->isSized()) continue`, I only get a single test that fails because it explicitly tests for it to use a wider VF. I doubt this is the right mechanism to use for that kind of purpose going forward.

In any case, if `collectElementTypesForWidening` would consider using a wider VF, it's still the cost-model that will then narrow that down (after this patch), because passing in the VF will result in a more restricted answer for `isLegalMaskedGather`.

> Also, does it matter that LoopVectorizationCostModel::isScalarWithPredication is still passing in an element type rather than a vector? For example, collectLoopUniforms calls isScalarWithPredication and at that point knows the VF. It may be fine, but I'm just a bit worried we might be swapping one inconsistency for another that's all.
I don't know if there's an actual problem atm, but I agree it makes sense to pass it to isScalarWithPredication as well. It should be possible since VF is available in all functions that call it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115329/new/

https://reviews.llvm.org/D115329