-
Notifications
You must be signed in to change notification settings - Fork 88
Allow OpSpecConstant in array counts in NSDI.100 #371
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
Conversation
dnovillo
commented
Nov 12, 2025
- Clarified that array counts may also be OpSpecConstant values in addition to OpConstant.
- This addresses Illegal debug information generated for array sized by specialisation constant glslang#3830
| <<DebugValue,*DebugValue*>>. This parameter is optional and so tools can treat it as | ||
| if it were present in OpenCL.DebugInfo.100 too but with no values. | ||
| * All literal parameters are passed as *OpConstant* values. | ||
| * All literal parameters are passed as *OpConstant* values, except for array counts whih may also be *OpSpecConstant* values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some reason other parameters cannot also be OpSpecConstant values?
Strictly speaking, this PR is allowing vector counts for DebugTypeMatrix to be OpSpecConstant values, in addition to array counts.
Regardless, minor / editorial:
| * All literal parameters are passed as *OpConstant* values, except for array counts whih may also be *OpSpecConstant* values. | |
| * All literal parameters are passed as *OpConstant* values, except for array counts, which may also be *OpSpecConstant* values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some reason other parameters cannot also be OpSpecConstant values?
Other parameters to other opcodes? I wanted to address the narrow case reported in KhronosGroup/glslang#3830
Strictly speaking, this PR is allowing vector counts for DebugTypeMatrix to be OpSpecConstant values, in addition to array counts.
Regardless, minor / editorial:
Applied. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took another pass at the spec to see what other instructions could benefit from a looser definition. These seemed to make sense: DebugTypeArray, DebugTypeMatrix, DebugTypeComposite, DebugTypeMember, DebugTypeInheritance, DebugTypeTemplateParameter, and, DebugOperation. All these instructions have size or count operands that can depend on spec constants.
The ones that I left intact are those that have sizes/counts that need a stricter notion of constant (literals, etc): DebugCompilationUnit, DebugTypeBasic, DebugTypePointer, DebugTypeQualifier, DebugTypeComposite, DebugTypeVector, DebugImportedEntity, and others like source locations and build identifiers.
PTAL.
- Clarified that array counts may also be OpSpecConstant values in addition to OpConstant. - This addresses Illegal debug information generated for array sized by specialisation constant KhronosGroup/glslang#3830
4faa91a to
16b13bc
Compare
Updated references to parameters and operands to specify that they must be constant instructions instead of just OpConstant or OpSpecConstant. Affected instructions: DebugTypeArray, DebugTypeMatrix, DebugTypeComposite, DebugTypeMember, DebugTypeInheritance, DebugTypeTemplateParameter, DebugOperation.
| <<DebugValue,*DebugValue*>>. This parameter is optional and so tools can treat it as | ||
| if it were present in OpenCL.DebugInfo.100 too but with no values. | ||
| * All literal parameters are passed as *OpConstant* values. | ||
| * All literal parameters are passed as *OpConstant* values, except for operands denoting a size, count, offset, or value, which can be any constant instruction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really hard sentence to parse, and it's not clear to me why it has to be so specific. Given instructions say what they require, I think we could just say:
| * All literal parameters are passed as *OpConstant* values, except for operands denoting a size, count, offset, or value, which can be any constant instruction. | |
| * As literal parameters cannot be used with non-semantic instructions, static parameters must be passed as constant instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied. Thanks.
bashbaug
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this mostly looks good to me.
SPV_EXT_long_vector (see #377) adds OpTypeVectorIdEXT, which allows the number of vector components to be specified via a spec constant ID. Does this mean that DebugTypeVector should accept a spec constant for its "Component Count" also, rather than a pure OpConstant?
'Component Count' is the '<id>' of a 32-bit integer *OpConstant* denoting the number of
elements in the vector. +
I don't think we need to make this change. The new long vector type will use a new |
|
Merging now. We can discuss #371 (comment) at the next meeting. |