[PATCH] D98054: [LoopVectorize][SVE] Fix crash when vectorising FP negation

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7384
   case Instruction::FNeg: {
-    assert(!VF.isScalable() && "VF is assumed to be non scalable.");
-    unsigned N = isScalarAfterVectorization(I, VF) ? VF.getKnownMinValue() : 1;
+    unsigned N = 1;
+    if (isScalarAfterVectorization(I, VF)) {
----------------
>From looking at `isScalarAfterVectorization` and `collectLoopScalars`, if the node is scalar after vectorization, it could be that:
1. VF is scalar (i.e. VF.getFixedValue() == 1).
2. VF is a vector, and the operation remains scalar. e.g. induction variable updates in the loop, which all remain scalar. There is special code in `collectLoopScalars` that does that analysis.
3. VF is a vector, and the operation is scalarized (i.e. `getWideningDecision(.., VF) == CM_Scalarize`).

For the case where the result is scalar after vectorization, I would have expected `VectorTy->getVectorElementType()` to be passed to `TTI.getArithmeticInstrCost`, not `VectorTy`, so I wonder if that's a bug.

Also, for case 2, we shouldn't be multiplying the cost by `N`. This code should instead check explicitly if the node is scalarized instead of relying on the more broader-defined `isScalarAfterVectorization`.

When scalarization does happen, the cost must be multiplied with `VF.getFixedValue()` instead (which has the implicit assert that VF is not scalable), so that you can avoid adding an unnecessary branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98054