Validator: Support SPV_NV_raw_access_chains #5568
Conversation
|
|
source/val/validate_memory.cpp
Outdated
| const Instruction* inst) { | ||
| std::string instr_name = "Op" + std::string(spvOpcodeString(inst->opcode())); | ||
|
|
||
| if (!_.features().raw_access_chains) { |
There was a problem hiding this comment.
This requirement is already communicated via the grammar.
source/val/validate_memory.cpp
Outdated
|
|
||
| // The pointed storage class must be valid. | ||
| const auto storage_class = result_type->GetOperandAs<spv::StorageClass>(1); | ||
| if (storage_class != spv::StorageClass::Workgroup && |
There was a problem hiding this comment.
Workgroup is not listed as a valid storage class in the spec. Is that a spec bug or tooling bug?
There was a problem hiding this comment.
It was a bug here. Removed it.
| } | ||
|
|
||
| const auto stride = _.FindDef(inst->GetOperandAs<uint32_t>(3)); | ||
| if (stride->opcode() != spv::Op::OpConstant) { |
There was a problem hiding this comment.
There is no type checking of stride, index, or offset. Is that intentional?
There was a problem hiding this comment.
Added type checking. It was not intentional.
source/val/validation_state.h
Outdated
| @@ -71,6 +71,9 @@ class ValidationState_t { | |||
| // and its values to be used without | |||
| // requiring any capability | |||
|
|
|||
| // Allow using raw access chains through the RawAccessChainsEXT capability | |||
| bool raw_access_chains = false; | |||
There was a problem hiding this comment.
I think this is unnecessary. If feature is checked for it should be done via HasCapability(spv::Capability::RawAccessChainsNV).
This is needed to unblock builds with updated SPIR-V headers. It is not a replacement for KhronosGroup#5568.
This is needed to unblock builds with updated SPIR-V headers. It is not a replacement for KhronosGroup#5568.
This is needed to unblock builds with updated SPIR-V headers. It is not a replacement for KhronosGroup#5568.
This is needed to unblock builds with updated SPIR-V headers. It is not a replacement for #5568.
rlocatti-nv
force-pushed
the
SPV_NV_raw_access_chains
branch
from
666c3c4 to
a5daa1d
Compare
|
Added unit tests for most of the checks. |
|
You'll need to update DEPS to include the SPIRV-Headers that includes raw access chains for the bots to work correctly. |
rlocatti-nv
force-pushed
the
SPV_NV_raw_access_chains
branch
from
a5daa1d to
d4061a5
Compare
|
Now the bots are complaining about a redefinition of the operand enums. Please rebase on main. |
rlocatti-nv
force-pushed
the
SPV_NV_raw_access_chains
branch
from
896f002 to
71bfaf4
Compare
| OpFunctionEnd | ||
| )"; | ||
| CompileSuccessfully(str.c_str()); | ||
| EXPECT_EQ(SPV_ERROR_INVALID_CAPABILITY, ValidateInstructions()); |
There was a problem hiding this comment.
When testing error cases, the style is generally to have an additional expectation along the lines of:
EXPECT_THAT(getDiagnosticString(),
HasSubstr("error string or relevant substring to confirm right error is triggered"));
71bfaf4 to
352963b
Compare
rlocatti-nv
force-pushed
the
SPV_NV_raw_access_chains
branch
from
352963b to
81d6676
Compare
|
Is there any ETA on when we get some forward progress on this PR? I'm waiting for this PR to get merged. |
|
I think this likely needs a rebase to address some of the bot failures. |
rlocatti-nv
force-pushed
the
SPV_NV_raw_access_chains
branch
from
81d6676 to
a39e7bc
Compare
Add support for KhronosGroup/SPIRV-Registry#238 .