paulwalker-arm added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:119-131
-Optional<unsigned> RISCVTTIImpl::getMaxVScale() const {
-  // There is no assumption of the maximum vector length in V specification.
-  // We use the value specified by users as the maximum vector length.
-  // This function will use the assumed maximum vector length to get the
-  // maximum vscale for LoopVectorizer.
-  // If users do not specify the maximum vector length, we have no way to
-  // know whether the LoopVectorizer is safe to do or not.
----------------
bsmith wrote:
> I'm not sure that RISCV have made a commitment to use the vscale_range 
> attribute yet have they? In either case I think they should be involved in a 
> change like this.
Perhaps it's worth this patch not removing getMaxVScale just yet but rather 
just AArch64's implementation?  There would only need to be a minor change to 
LoopVectorize.cpp along the lines of `if (!MaxVScale && 
TheFunction->hasFnAttribute(Attribute::VScaleRange...`.  That way getMaxVScale 
can be removed if/when no one needs it, which I hope is not too far away.

If there is agreement to remove it then I imagine code similar to what you've 
done for SVE in CodeGenFunction.cpp will be needed for RISCV otherwise the 
patch will cause a regression in functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106277

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to