[PATCH] D134422: Scalarize calls to masked functions in LV

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:129
+
+  unsigned getParamIndexForMask() const {
+    auto MaskPos = getParamIndexForOptionalMask();
----------------
Perhaps have this return `Optional<unsigned>` to give people the option to use either `if (isMasked())` or `if (auto Pos = getParamIndexForMask())` depending on what works best for them?

It just seems bad to force people to traverse the shape parameters multiple time and means `getParamIndexForOptionalMask` can be removed.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1120-1123
+      for (VFInfo Info : Mappings)
+        HasMaskedVersion |= Info.isMasked();
+
+      if (HasMaskedVersion) {
----------------
Is it possible to use `any_of` here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3464
+  // masked call with branches.
+  if (!TLI || CI->isNoBuiltin() || !VecFunc || Legal->isMaskRequired(CI))
     return Cost;
----------------
Given we cannot scalarise scalable vectors should this return `InvalidCost` somewhere? Or perhaps `blockCanBePredicated` should return false? Perhaps I'm being paranoid but the code should be resilient to the case of somebody adding a bunch of scalable vector TLI mapping before all the LoopVectorize support is finished. Or do tests already exist for scalable vectors that show it does not matter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134422