Validate enums have sensible versions and are visible #369
Conversation
|
I have a PR in flight that relaxes SPIRV-Tools' parsing, so we don't need to be as strict as this. I'll modify this PR in a corresponding way. |
dneto0
changed the title
|
WRT #278, a pretty common mistake is to guard an instruction by both a capability and its extension, for example (this is just an example, it's not in the actual json file): {
"opname" : "OpControlBarrierArriveINTEL",
"class" : "Barrier",
"opcode" : 6142,
"operands" : [
{ "kind" : "IdScope", "name" : "'Execution'" },
{ "kind" : "IdScope", "name" : "'Memory'" },
{ "kind" : "IdMemorySemantics", "name" : "'Semantics'" }
],
"capabilities" : [ "SplitBarrierINTEL" ],
"extensions" : [ "SPV_INTEL_split_barrier" ],
"version" : "None"
},I tried modifying the json file to do this and it didn't get flagged as an error. Actually, it doesn't seem to get flagged by an error if I only include the extension and do not include a capability. Is this intended behavior? |
|
Bah. I think I didn't capture the intent of #278. Non-capability should normally be guarded by the capability alone. will fix. |
There are 78 cases like this already, e.g. OpSubgroupBallotKHR. |
|
I also fixed the logic for checking capabilites. See the commit message (and the comments in the patch) |
Add version field for each eumerant. For capabilities and instructions introduced by an extension (its first version is "None"): - the capability should be guarded by an extension - the instruction should be guarded by a capability. Other enums are presumed guarded transitiviely by use as an operand to an instruction or another operand. Fixes: KhronosGroup#278, KhronosGroup#368
For capabilities, only check for lack of an extension. If capability X lists capabilities Y and Z, those are not guards *for* X, but rather when X is enabled it also implicitly enables Y and Z. Also, an instruction that is *not* in a core SPIR-V version must not be directly enabled by *both* and extension and a capability. There are 78 existing cases that break this rule, so grandparent them in with an allow-list.
Add it for the HostAccessQualifier enums from SPV_INTEL_global_variable_host_access
There was a problem hiding this comment.
Thanks! This is a nice improvement.
I verified that the example I included previously that guards an instruction by both a capability and an extension is flagged an an error now. 👍
Should we also flag instructions that are guarded by an extension but not a capability as an error, for example (again, this is just an example, it's not actually in the json file):
{
"opname" : "OpControlBarrierArriveINTEL",
"class" : "Barrier",
"opcode" : 6142,
"operands" : [
{ "kind" : "IdScope", "name" : "'Execution'" },
{ "kind" : "IdScope", "name" : "'Memory'" },
{ "kind" : "IdMemorySemantics", "name" : "'Semantics'" }
],
"extensions" : [ "SPV_INTEL_split_barrier" ],
"version" : "None"
},I'm fine merging this as-is; I mostly want to understand the policy and track the additional checks, assuming they're something we want to do.
| namespace spv { | ||
|
|
||
| bool IsLegacyDoublyEnabledInstruction(const std::string& instruction) { |
There was a problem hiding this comment.
I suspect you've considered this, but just to ask the question: it's too late to change these in the json file?
|
Merging as discussed in the August 23rd teleconference. |
Add version field for each eumerant.
For capabilities and instructions introduced by an extension (its
first version is "None"):
Other enums are presumed guarded transitiviely by use as an operand
to an instruction or another operand.
Fixes: #278, #368