[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 a comment.

Thanks for making the changes @kmclaughlin! Just a couple more comments ...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5661
   if (UserVF.isNonZero() && !IgnoreScalableUserVF &&
-      Legal->isSafeForAnyVectorWidth())
+      Legal->isSafeForAnyVectorWidth()) {
+    if (!canVectorizeReductions(UserVF)) {
----------------
I think this looks much better now you're just checking reductions only once and early on - thanks for this! However, I think you might need to move this check down to line 5677 where we return UserVF. So the reason I think this is because if we have a loop that contains memory dependences and reductions in the same loop we want to ensure we always do the reduction checks regardless. For example, Legal->isSafeForAnyVectorWidth() could return false and then in the code below we may successfully reduce the UserVF from <vscale x 8 x float> to <vscale x 4 x float> without ever calling canVectorizeReductions.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable_reductions.ll:1
+; REQUIRES: asserts
+; 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
----------------
Thanks for RUN line changes here - looks a lot neater now thanks! If it's not too difficult I think it would be great if you could test the remark here too, since this is user-facing rather than debug. If you want you can even test the remark instead of the debug - this would also mean you can remove the "REQUIRE: asserts" line above too.


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

https://reviews.llvm.org/D95245