Skip to content

[LangRef] Cap maximum value of vscale at 2^31-1. #144607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Jun 17, 2025

Cap the maximum value of vscale at 2^31-1. This should exceed what is available on actual hardware by far but should be enough to ensure that computing the runtime VF vscale x V generated by LoopVectorize won't wrap when using i64 index types, as V cannot exceed 2^32. LoopVectorize already assumes it won't wrap, this change tries to back up the assumption in LangRef.

Cap the maximum value of vscale at 2^31-1. This should exceed what is
available on actual hardware by far but should be enough to ensure that
computing the runtime VF `vscale x V` generated by LoopVectorize won't wrap when
using i64 index types, as `V` cannot exceed 2^32. LoopVectorize already
assumes it won't wrap, this change tries to back up the assumption in
LangRef.
@llvmbot
Copy link
Member

llvmbot commented Jun 17, 2025

@llvm/pr-subscribers-llvm-ir

Author: Florian Hahn (fhahn)

Changes

Cap the maximum value of vscale at 2^31-1. This should exceed what is available on actual hardware by far but should be enough to ensure that computing the runtime VF vscale x V generated by LoopVectorize won't wrap when using i64 index types, as V cannot exceed 2^32. LoopVectorize already assumes it won't wrap, this change tries to back up the assumption in LangRef.


Full diff: https://github.com/llvm/llvm-project/pull/144607.diff

1 Files Affected:

  • (modified) llvm/docs/LangRef.rst (+2-1)
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index cc72a37f68599..b876d0bdbc29e 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -4446,6 +4446,7 @@ of elements; vscale is a positive integer that is unknown at compile time
 and the same hardware-dependent constant for all scalable vectors at run
 time. The size of a specific scalable vector type is thus constant within
 IR, even if the exact size in bytes cannot be determined until run time.
+vscale can be at most 2^31-1.
 
 :Examples:
 
@@ -30399,7 +30400,7 @@ Semantics:
 """"""""""
 
 ``vscale`` is a positive value that is constant throughout program
-execution, but is unknown at compile time.
+execution, but is unknown at compile time. The returned value can be at most 2^31-1.
 If the result value does not fit in the result type, then the result is
 a :ref:`poison value <poisonvalues>`.
 

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@efriedma-quic
Copy link
Collaborator

runtime VF vscale x V generated by LoopVectorize won't wrap when using i64 index types

What about 32-bit targets?

(Not that I'm objecting to this... you'd need to be doing something really unusual with your target to end up with vscales above 1000.)

@topperc
Copy link
Collaborator

topperc commented Jun 17, 2025

runtime VF vscale x V generated by LoopVectorize won't wrap when using i64 index types

What about 32-bit targets?

(Not that I'm objecting to this... you'd need to be doing something really unusual with your target to end up with vscales above 1000.)

If it helps, the maximum vscale for RISC-V is currently 1024. If we ever fix some other issues that prevent us from supporting the smallest vector length of 32, then the maximum vscale will be 2048.

@paulwalker-arm
Copy link
Collaborator

Consider the PR as is:
Assuming hardware doesn't use a value of zero to mean disabled, the upper bound is likely to be a power-of-two? So perhaps "no more than 2^30" or something smaller?

Thinking more broadly. To validate LoopVectorize's usage you effectively want to document the largest runtime vector length of VectorType be one that is representable as an i64. Why not say this rather than restricting vscale based on the current implementation of VectorType?

I'm sure restricting the range of vscale will have other benefits but that feels like an independent change and we already have mechanisms to achieve this via vscale_range (and range?) attributes.

@preames
Copy link
Collaborator

preames commented Jun 18, 2025

I'm sure restricting the range of vscale will have other benefits but that feels like an independent change and we already have mechanisms to achieve this via vscale_range (and range?) attributes.

I think this is a case of false generality. We could explore alternate options here, but this patch has the major advantage of being simple. We can come back and revisit this (even reverse it) if needed, but I see no reason not to go with the simple answer for the moment.

@paulwalker-arm
Copy link
Collaborator

paulwalker-arm commented Jun 18, 2025

I'm sure restricting the range of vscale will have other benefits but that feels like an independent change and we already have mechanisms to achieve this via vscale_range (and range?) attributes.

I think this is a case of false generality. We could explore alternate options here, but this patch has the major advantage of being simple. We can come back and revisit this (even reverse it) if needed, but I see no reason not to go with the simple answer for the moment.

I'm not suggesting we do any work, only that the restriction should be specified in terms of the runtime vector length and not about vscale. I think this would actually be less work and easier to walk back because with the current vscale suggestion the next obvious step is to update getVScaleRange() to implement the new requirement, whereas with my suggestion there's no change to any part of the IR.

For context, this PR is in response to #143532 albeit it has no real bearing because in that PR I'm simply saying that in order to use the helper functions you commit to the result type being big enough. If you cannot commit to that then you should do the scaling explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants