[PATCH] D101916: [LoopVectorize] Fix crash for predicated instructions with scalable VF

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6827
         // Do not apply discount logic if hacked cost is needed
         // for emulated masked memrefs.
+        if (!VF.isScalable())
----------------
Can you extend the comment here explaining that the discount logic is not applied when the VF is scalable, because that would lead to invalid scalarization costs.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6828-6829
         // for emulated masked memrefs.
-        if (!useEmulatedMaskMemRefHack(&I) &&
-            computePredInstDiscount(&I, ScalarCosts, VF) >= 0)
-          ScalarCostsVF.insert(ScalarCosts.begin(), ScalarCosts.end());
+        if (!VF.isScalable())
+          if (!useEmulatedMaskMemRefHack(&I) &&
+              computePredInstDiscount(&I, ScalarCosts, VF) >= 0)
----------------
Can you merge these conditions, i.e. `if (!VFScalable() && !useEmulatedMaskMemRefHack(&I) && ..`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6917
           APInt::getAllOnesValue(VF.getKnownMinValue()), true, false);
       assert(!VF.isScalable() && "scalable vectors not yet supported.");
       ScalarCost +=
----------------
I think this assert can be removed in favour of using `VF.getFixedValue()` instead of `VF.getKnownMinValue()` everywhere in this function.


================
Comment at: llvm/test/Transforms/LoopVectorize/scalable-predicate-instruction.ll:1
+; RUN: opt < %s -loop-vectorize -force-target-supports-scalable-vectors -S | FileCheck %s
+; RUN: opt < %s -loop-vectorize -force-target-supports-scalable-vectors -prefer-predicate-over-epilogue=predicate-dont-vectorize -S | FileCheck %s --check-prefix=CHECK-PREFER-PREDICATE
----------------
The RUN lines don't actually enable scalable vectorization (=off by default), so this test always passes.
You'll need to add `-scalable-vectorization=preferred`.

I'd also recommend moving this function to llvm/test/Transforms/LoopVectorize/AArch64. The `-force-target-supports-scalable-vectors` flag doesn't work properly, because the BasicTTIImpl costmodel implementation has defaults that must be overridden for scalable auto-vec to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101916