[PATCH] D95363: [SVE][LoopVectorize] Add support for scalable vectorization of loops with vector reverse

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:451
 
+  // Avoid to call BasicTTImpl for scalable vector and then assert
+  // ATM in the BasicTTImpl the cost for ISD that accepts scalable
----------------
Hi Carol, is the reason why you bail out here because we are casting between at least one illegal type? If so, perhaps you could add a TODO here, something like:

// TODO : For scalable vectors we avoid calling the BaseT version for now because it doesn't yet work for illegal vector types as it assumes these are fixed width.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2801
+      unsigned NumElts = VF.isScalable()
+                             ? VectorLoopValueMap.getNumCachedLanes()
+                             : VF.getKnownMinValue();
----------------
Hi Carol. I don't think this is right here. The number of cached lanes in my patch (D95139) simply refers to the number of values we have managed to cache. It's just an optimisation the vectoriser uses to avoid constantly calculating the same lane index expressions. However, what you're after here is a runtime calculation for the last lane index - you can use getRuntimeVF in my patch for this. I think the code below will probably need splitting out into fixed width vs scalable cases, i.e.

Builder.getInt32(1 - NumElts)

would need to be something like:

Builder.CreateSub(Builder.getInt32(1), getRuntimeVF())

for scalable vectors. Hope that makes sense!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95363