[PATCH] D98509: [LV] Calculate max feasible scalable VF.

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5565
+  // Limit MaxScalableVF by the maximum safe dependence distance.
+  if (!Legal->isSafeForAnyVectorWidth()) {
+    Optional<unsigned> MaxVScale = TTI.getMaxVScale();
----------------
Flip this and return early?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5582-5584
+  MinBWs = computeMinimumValueSizes(TheLoop->getBlocks(), *DB, &TTI);
+  unsigned SmallestType, WidestType;
+  std::tie(SmallestType, WidestType) = getSmallestAndWidestTypes();
----------------
should this be moved closer to where it's used?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5652-5654
+    if (MaxVF.isScalable())
+      LLVM_DEBUG(dbgs() << "LV: Found feasible scalable VF = " << MaxVF
+                        << "\n");
----------------
could you add a comment stating this is ignored for now but will be addressed in a later patch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5819
+      ComputeScalableMaxVF);
+  MaxVectorSize = MinVF(MaxVectorSize, MaxSafeVF);
   LLVM_DEBUG(dbgs() << "LV: The Widest register safe to use is: "
----------------
I know this came before your patch but I find it a bit confusing VF and VectorSize are use interchangeably 


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-analysis.ll:2
+; REQUIRES: asserts
+; RUN: opt -mtriple=aarch64-none-linux-gnu -mattr=+sve -force-target-instruction-cost=1 -loop-vectorize -S -debug-only=loop-vectorize        < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK_SCALABLE_ON
+; RUN: opt -mtriple=aarch64-none-linux-gnu -mattr=+sve -force-target-instruction-cost=1 -loop-vectorize -S -debug-only=loop-vectorize -vectorizer-maximize-bandwidth < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK_SCALABLE_ON_MAXBW
----------------
nit: multiple spaces


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-vf-analysis.ll:32
+}
+; Test that the MaxVF for the following loop, with a dependence distance
+; of 64 elements, is calculated as (maxvscale = 16) * 4.
----------------
nit: add newline above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98509