Skip to content
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

Inconsistency regarding OpPtrAccessChain and storage classes #107

Open
Hugobros3 opened this issue Apr 26, 2021 · 3 comments
Open

Inconsistency regarding OpPtrAccessChain and storage classes #107

Hugobros3 opened this issue Apr 26, 2021 · 3 comments

Comments

@Hugobros3
Copy link

There seems to be conflicting information regarding what pointers OpPtrAccessChain may be applied to:

The spec classifies values obtained from that instruction as "variable pointers" (2.2.2, page 19), and later in 2.16.1 it says variable pointers may only point to StorageBuffer and Workgroup storage classes.

But in the definition of OpPtrAccessChain, it is explicitly specified what the behavior is when using the Element operand to offset a pointer in the PushConstant storage class, and the behavior is also implied to be valid (but the offset impl-defined) for other storage classes too.

To compute the new element address, Element is treated as a signed count of elements E, relative to the original Base element B, and the address of element B + E is computed using enough precision to avoid overflow and underflow. For objects in the Uniform, StorageBuffer, or PushConstant storage classes, the element’s address or location is calculated using a stride, which will be the Base-type’s Array Stride if the Base type is decorated with ArrayStride. For all other objects, the implementation calculates the element’s address or location.

@Hugobros3 Hugobros3 changed the title Inconsistency reguarding OpPtrAccessChain and storage classes Inconsistency regarding OpPtrAccessChain and storage classes Apr 26, 2021
@sfricke-samsung
Copy link
Contributor

If I am reading your question correctly, why do you think you can't have a OpPtrAccessChain that is not a "variable pointer" and then it doesn't have the restrictions

variable pointers are putting the restrictions in place (on other parts of the spec as well), which I think is different from having "inconsistent OpPtrAccessChain information"

@Hugobros3
Copy link
Author

Hugobros3 commented May 5, 2021

Section 2.2.2 defines a variable pointer as:

Variable pointer: A pointer of logical pointer type that results from one of the following instructions:
• OpSelect
• OpPhi
• OpFunctionCall
• OpPtrAccessChain // !
• OpLoad
• OpConstantNull

There are no further restrictions given, so that implies all results of OpPtrAccessChain are variable pointers, regardless of operands.

Furthermore, if the variable ptr capability is not declared, 2.16 excludes a logical pointer (which pointers within the Push Constant storage classe are) from being the result of OpPtrAccessChain:

If neither the VariablePointers nor VariablePointersStorageBuffer capabilities are declared, the following rules apply
to logical pointer types:

[...]

It is invalid for a pointer to be the Result <id> of any instruction other than:
* OpVariable
* OpAccessChain
* OpInBoundsAccessChain
* OpFunctionParameter
* OpImageTexelPointer
* OpCopyObject

This is a problem for at least one implementation: Mesa assumes this all means OpPtrAccessChain cannot operate on pointers into push constants, a while back I talked to people on IRC and they confirmed that was their interpretation too. I can also point to that assumption being baked in the code here, as nir_deref_instr_get_variable will return null when the deref chain is obtained from translating an OpPtrAccessChain operation to NIR.

My understanding is to get this fixed in Mesa will require clarification from the specification on where exactly OpPtrAccessChain is valid.

@raunraun
Copy link
Contributor

raunraun commented Jul 7, 2021

Khronos is discussing a clarification. OpPtrAccessChain with push constants is never intended to be a valid usage.

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

No branches or pull requests

3 participants