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

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin added a comment.

Thanks for reviewing this patch, all!



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3924
     EVT VecVT = InVec.getValueType();
+    if (VecVT.isScalableVector())
+      break;
----------------
fhahn wrote:
> Can you add a test for this? Also, this seems completely unrelated, can you split it off?
I've removed this from the patch, I don't think it's required for the tests here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6198
       TTI.enableAggressiveInterleaving(HasReductions);
-  if (!InterleavingRequiresRuntimePointerCheck && LoopCost < SmallLoopCost) {
+  if (!InterleavingRequiresRuntimePointerCheck && (unsigned)*LoopCost.getValue() < SmallLoopCost) {
     // We assume that the cost overhead is 1 and we use the cost model
----------------
CarolineConcatto wrote:
> Can you change SmallLoopCost to be instruction cost as LoopCost, so you don't need to use *LoopCost.getValue()?
> And I believe that in the std::min you will not need to use getValue
Hi @CarolineConcatto, thanks for your suggestions on InstructionCost! I didn't change the SmallLoopCost flag to be an instruction cost in the last revision as this caused tests which use -small-loop-cost to fail (e.g. LoopVectorize/unroll_novec.ll)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7665
 
+  if (!CM.isLegalWideningOperation(UserVF))
+    return {{UserVF, InstructionCost::getInvalid()}};
----------------
fhahn wrote:
> This should only be checked  in the code handling `UserVF` below? Also, This seems like a property that generally limits to vectorization factor to fixed-width vectorization factors and would be good to check beforehand.
> 
>  Would it be possible to just limit vectorization factors to fixed width factors in `computeFeasibleMaxVF`? This way, we won't need extra checks once automatically picked VFs are supported. You'd also won't need any extra code in the caller of `::plan`.
> 
> This is similar to how we deal with other 'legality' properties that depend on the vectorization factor, like dependencies that may limit the vectorization factor.
Thanks for this suggestion, @fhahn. I've moved the canVectorizeReductions check to `computeFeasibleMaxVF` & updated the affected test in scalable_reductions.ll, where we can use fixed-width vectorization instead (`@mul`)


================
Comment at: llvm/test/Transforms/LoopVectorize/scalable_reductions.ll:1
+; 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 -S 2>&1 | FileCheck %s -check-prefix=CHECK-WARN
----------------
david-arm wrote:
> Needs a "REQUIRES: asserts" here I think because you're relying upon debug output. Also, since you're explicitly adding "-mattr=+sve" here I think you'll either have to:
> 
> 1. Make the test generic work for all targets (this test will fail on some builds due to lack of AArch64 support), or
> 2. Move the test for LoopVectorize/AArch64
Added `REQUIRES: asserts` & moved the test to `Transforms/LoopVectorize/AArch64`


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

https://reviews.llvm.org/D95245