[PATCH] D106277: [SVE] Remove the interface for getMaxVScale in favour of the IR attributes

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:134
+  unsigned getMaxNumElements(ElementCount VF,
+                             const Instruction *I = nullptr) const {
     if (!VF.isScalable())
----------------
Can this parameter be a `Function*`? given there's no real link between this function and LLVM Instructions.




================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:142-143
+                         ->getFnAttribute(Attribute::VScaleRange)
+                         .getVScaleRangeArgs()
+                         .second;
+    }
----------------
This can return `0` implying there is no know maximum.  With the current code this means `0` will be returned instead of a sensible default.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5620
+        TheFunction->getFnAttribute(Attribute::VScaleRange);
+    MaxVScale = VScaleRangeAttr.getVScaleRangeArgs().second;
+  }
----------------
I think you only want to set `MaxVScale` when `VScaleRangeAttr.getVScaleRangeArgs().second` is non-zero.

Given this and the above similar comment perhaps there's need for extra tests that cover `vscale_range(2,0)` for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106277