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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1519
 
+  // Returns true if the target machine supports all of the reduction
+  // variables found for the given VF
----------------
nit: Perhaps use `///` here instead of '//' in line with other function comments?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5690
 
+      if (!canVectorizeReductions(MaxSafeVF)) {
+        LLVM_DEBUG(dbgs() << "LV: Scalable vectorization not supported for the "
----------------
I wonder if it's worth bailing out even earlier, i.e. in the same place as above where you check initially? I think the main benefit to bailing out here is if you can reduce the VF to something smaller so that it becomes legal. However, I think for reductions changing the VF won't make a difference in practice.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5699
+                 << "Scalable vectorization not supported for the "
+                 << "reduction types found in this loop. "
+                 << "Using fixed-width vectorization instead.";
----------------
nit: Perhaps use "operations" here instead of types? I'm thinking that the user probably isn't aware of the RecurrenceKind so type might not make as much sense?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable_reductions.ll:2
+; REQUIRES: asserts
+; RUN: opt < %s -loop-vectorize -transform-warning -mtriple aarch64-unknown-linux-gnu -mattr=+sve -debug-only=loop-vectorize -S 2>&1 | FileCheck %s -check-prefix=CHECK
+; RUN: opt < %s -loop-vectorize -transform-warning -mtriple aarch64-unknown-linux-gnu -mattr=+sve -debug-only=loop-vectorize 2>&1 | FileCheck %s -check-prefix=CHECK-DEBUG
----------------
I think you can reduce the number of RUN lines here by piping stderr for the first RUN line to a temporary file, e.g. something like 

; RUN: opt < %s -loop-vectorize -transform-warning -mtriple aarch64-unknown-linux-gnu -mattr=+sve -debug-only=loop-vectorize -S 2>%t | FileCheck %s -check-prefix=CHECK
; RUN cat %t | FileCheck %s -check-prefix=CHECK-DEBUG



================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable_reductions.ll:4
+; RUN: opt < %s -loop-vectorize -transform-warning -mtriple aarch64-unknown-linux-gnu -mattr=+sve -debug-only=loop-vectorize 2>&1 | FileCheck %s -check-prefix=CHECK-DEBUG
+; RUN: opt < %s -loop-vectorize -transform-warning -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S 2>&1 | FileCheck %s -check-prefix=CHECK-WARN
+
----------------
Is it worth changing this to check for the new remark instead? You can use something like this:

; RUN: opt < %s  -loop-vectorize -pass-remarks='loop-vectorize' -disable-output -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S 2>&1 | ...


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable_reductions.ll:223
+  %0 = load float, float* %arrayidx, align 4
+  %add = fadd float %0, %sum.07
+  %iv.next = add nuw nsw i64 %iv, 1
----------------
I'm a bit surprised this vectorises to be honest, since there is no 'fast' flag here! Perhaps for IEEE math you have to add specific attributes to the function?


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

https://reviews.llvm.org/D95245