[PATCH] D95245: [SVE] Add support for scalable vectorization of loops with int/fast FP reductions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes. I only have some more comments about the tests now.



================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions.ll:101
+
+define dso_local i32 @xor(i32* nocapture %a, i32* nocapture readonly %b, i64 %n) {
+; CHECK-LABEL: @xor
----------------
nit: remove `dso_local` here and in other definitions.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions.ll:340
+
+; CHECK-DEBUG: Scalable vectorization not supported for the reduction operations found in this loop. Using fixed-width vectorization instead.
+define dso_local i32 @mul(i32* nocapture %a, i32* nocapture readonly %b, i64 %n) {
----------------
This CHECK-DEBUG (with it's own RUN line) is not checking //which// function is not vectorizing, it could just as well be emitted for one of the other functions. I'd suggest explicitly adding checks for  `@mul` and adding a CHECK-DEBUG line for the other tests as well.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions.ll:376
+
+; CHECK-DEBUG: Scalable vectorization not supported for the reduction operations found in this loop. Using fixed-width vectorization instead.
+define dso_local i32 @memory_dependence(i32* noalias nocapture %a, i32* noalias nocapture readonly %b, i64 %n) {
----------------
Same as above. Can you also add a comment saying why you're testing a `memory_dependence` issue in a test file called `scalable-reductions.ll` ?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions.ll:424
+
+; CHECK-DEBUG: loop not vectorized: value that could not be identified as reduction is used outside the loop
+define dso_local float @fmin(float* noalias nocapture readonly %a, i64 %n) {
----------------
These two fmin/fmax tests are not very useful, because the loop doesn't fail to vectorize because of code added in this patch.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-reductions.ll:470
+
+attributes #0 = { "no-nans-fp-math"="true" }
+
----------------
nit: use `nnan` directly in the fp operation instead of an attribute.


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

https://reviews.llvm.org/D95245