[PATCH] D101260: [LoopVectorize][SVE] Remove assert for scalable vector in InnerLoopVectorizer::fixReduction

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-reduction-inloop.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -loop-vectorize -mtriple aarch64-unknown-linux-gnu -mattr=+sve  -S | FileCheck %s
+
----------------
fhahn wrote:
> Can this test be target independent by using `-force-target-supports-scalable-vectors` or does it contain any AArch64 cost-modeling?
Thank you @fhahn for that.
I don't see direct dependency in this test and the fix with the cost model, but I could be wrong. 
I've added the flag as you suggested and it works fine.
So I've moved the test to be outside AArch64.
Is that ok?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-reduction-inloop.ll:15
+; CHECK:         [[WIDE_LOAD:%.*]] = load <vscale x 8 x i8>, <vscale x 8 x i8>*
+; CHECK-NEXT:    [[TMP22:%.*]] = call i32 @llvm.vscale.i32()
+; CHECK-NEXT:    [[TMP23:%.*]] = mul i32 [[TMP22]], 8
----------------
david-arm wrote:
> nit: I think you can also just remove lines TMP22-24 as they're unused now.
Thank you @david-arm,
for some reason before this test was failing without these lines, but now it is passing. Probably I was doing something wrong.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-reduction-inloop.ll:41
+
+.lr.ph:                                           ; preds = %entry, %.lr.ph
+  %indvars.iv = phi i32 [ %indvars.iv.next, %.lr.ph ], [ 0, %entry ]
----------------
fhahn wrote:
> might be helpful to use a nicer name for the loop block to make the test a bit easier to parse, maybe `%loop`? Same for `._crit_edge`, perhaps `%exit`?
Hey @fhahn thank you for your input,
I  have changed .lr.ph to loop and ._crit_edge to exit.
But just for reference, this test has the same llvm-ir as in reduction-inloop.ll but now using scalable vectors instead of fixed vector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101260