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

Operands are treated as literals instead of ids at capability check #248

Closed
AlexeySotkin opened this issue Jun 28, 2016 · 3 comments
Closed
Labels

Comments

@AlexeySotkin
Copy link

In particular during validation of atomic instructions memory semantics operands are treated as literals, but according to the spec they must be id.

In the attached example, it happened that OpAtomicIAdd has memory semantics operand = %64. Literally it corresponds to 0x40 (UniformMemory, which require shader capability). But actually %64 is an id of an integer constant = 0x10(SequentiallyConsistent, no capability required).

%64 = OpConstant %2 16
...
%63 = OpAtomicIAdd %2 %50 %35 %64 %35

This causes false positive errors to be reported by spirv-val tool:

error: 129: Operand 5 of AtomicIAdd requires one of these capabilities: Shader

atomic_exchange.global_atomic_double_declared_in_program.used_in_generic_function.order_relaxed.spv20_32.zip

atomic_exchange.global_atomic_double_declared_in_program.used_in_generic_function.order_relaxed.spv20_32.txt

@dneto0
Copy link
Collaborator

dneto0 commented Jun 28, 2016

Confirmed.

I tracked it down to the CapCheck function at https://github.com/KhronosGroup/SPIRV-Tools/blob/master/source/validate_instruction.cpp#L87

For operands that are IDs, it should not be checking the ID value itself, but rather the underlying constant if it can find one. Note that we should have a one-sided error, i.e. if we can't determine the value then let it pass. (This might occur if there's a complex expression the code doesn't know how to evaluate.

Looking at the operand types in libspirv.h, this kind of problem would only affect SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID (as in this reported but), or SPV_OEPRAND_TYPE_SCOPE_ID, but none of those values have required capabilities.

Sending to @umar456

@dneto0
Copy link
Collaborator

dneto0 commented Jun 28, 2016

I have a stopgap PR that I'll post shortly.

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Jun 28, 2016
Works around issue 248 by weakening the test:
KhronosGroup#248

The validator should try to track (32-bit) constant values, and then
for capability checks on IDs, check the referenced value, not the
raw ID number.
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Jun 29, 2016
Works around issue 248 by weakening the test:
KhronosGroup#248

The validator should try to track (32-bit) constant values, and then
for capability checks on IDs, check the referenced value, not the
raw ID number.
@dneto0
Copy link
Collaborator

dneto0 commented Jun 29, 2016

Workaround has been merged. I filed #252 for the "proper" (completist) fix.

@dneto0 dneto0 closed this as completed Jun 29, 2016
rjodinchr pushed a commit to rjodinchr/SPIRV-Tools that referenced this issue Jun 22, 2022
Implement header definitions for SPV_NV_bindless_texture
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants