[OMM] Remove validation masking, update spec #377
Conversation
proposals/0024-opacity-micromaps.md
Outdated
|
|
||
| ```hlsl | ||
| enum RAYQUERY_FLAG : uint | ||
| enum class RAYQUERY_FLAG : uint32_t |
There was a problem hiding this comment.
Should probably back-out this changed line.
This is HSL-equivalent code, which doesn't actually have a defined enum type, so it represents a kind of virtual ("as-if") enum. The defined values are not scoped under an enum, since they are actually static const uint values, so enum "class" is inappropriate, as that would change the scope of the values.
Also, since this is HLSL, uint is fine as the storage type as well.
proposals/0024-opacity-micromaps.md
Outdated
| @@ -200,46 +217,79 @@ In addition to the AST traversal detecting any explicit use of the new flag, | |||
| the same DefaultError warning diagnostic will be added to detect when the | |||
| new ray flag is used at the `RayQuery` template argument, `TraceRay()`, or | |||
| `RayQuery::TraceRayInline()` (when it is immediate). | |||
There was a problem hiding this comment.
The preceding paragraph talks about checking immediate constant RayFlags values on the method calls. We decided not to do this and only use the availability check mechanism.
Two paragraphs earlier (I can't comment directly on unchanged areas), it talks about looking for the RaytracingPipelineFlags::AllowOpacityMicromaps in the flags for the Raytracing pipeline config1 subobject. This is on top of checking availability attributes on expressions used. We should remove this paragraph as well, since we decided not to add this checking.
proposals/0024-opacity-micromaps.md
Outdated
| Proposed DefaultError warning diagnostic: | ||
|
|
||
| - `"%select{RaytracingPipelineFlags|RayFlags}0 (0x%1) includes unsupported bits for shader model %2; valid mask: 0x%3"`. | ||
| - `"%select{RaytracingPipelineFlags|RayFlags}0 (0x%1) includes unsupported bits for shader model %2"`. | ||
| This new warning will have a different warning group, such as | ||
| `hlsl-availability`. |
There was a problem hiding this comment.
This pertains to the checks being removed above, so it should also be removed.
proposals/0024-opacity-micromaps.md
Outdated
| Currently, the `AllocateRayQuery` DXIL Op has the following function signature: | ||
| `uint [[hidden]] AllocateRayQuery(in uint flags);` | ||
| The new DXIL Op, `AllocateRayQuery2`, will instead have this signature: | ||
| `uint [[hidden]] AllocateRayQuery2(in uint flags, in uint ray_query_flags);` |
There was a problem hiding this comment.
These are HLSL intrinsic signatures, and they are expressed inline instead of in a code block. I meant that we need the DXIL signature for the dx.op.allocateRayQuery2 operation. Something like:
; Function Attrs: nounwind
declare i32 @dx.op.allocateRayQuery2(i32 OpCode, i32 constRayFlags, i32 RayQueryFlags)
| `RAYTRACING_PIPELINE_FLAG_ALLOW_OPACITY_MICROMAPS`, | ||
| `RAY_FLAG_FORCE_OMM_2_STATE`, and `RAYQUERY_FLAG_ALLOW_OPACITY_MICROMAPS` | ||
| will have an availability attribute, which restricts their usage to | ||
| shader model 6.9. `RAYQUERY_FLAG_NONE` will be left unrestricted. | ||
|
|
||
| This will have implications for uses of these flags outside of the intended | ||
| targets. Since they are just uint values, it's technically legal to refer to |
There was a problem hiding this comment.
I think we can use DefaultError here now, since this will be the only diagnostic used to catch use of these flags in an earlier shader model.
proposals/0024-opacity-micromaps.md
Outdated
| Validation will be added to check the `RayFlags` parameters, and, if | ||
| applicable, the `RayQueryFlags` parameters, for each applicable DXIL operation, | ||
| including the new `AllocateRayQuery2` operation. An error will be emitted if | ||
| the flags are constant, the new flag is used, and the shader model | ||
| is less than 6.9. | ||
|
|
||
| Proposed validation error diagnostic: | ||
|
|
||
| - `"RayFlags used in '%0' specifies unknown flags (0x%1) for shader model %2; valid mask: 0x%3"` | ||
| - `"RayFlags used in '%0' specifies unknown flags (0x%1) for shader model %2"` |
There was a problem hiding this comment.
We shouldn't be doing this validation of the RayFlags values. Plus, AllocateRayQuery2 should be defined to only be valid for SM 6.9+, so a separate check should catch any usage of that intrinsic.
| Finally, validation will be added for the `AllocateRayQuery2` DXIL operation | ||
| to ensure that when `RAY_FLAG_FORCE_OMM_2_STATE` is set on the ConstRayFlags | ||
| argument, the `RAYQUERY_FLAG_ALLOW_OPACITY_MICROMAPS` is also set on the | ||
| RayQueryFlags argument. |
There was a problem hiding this comment.
Just a reminder to synchronize argument names referred to here with whatever's used in the signature above, and use the same for the hctdb.py argument names. It turns out that ConstRayFlags should be constRayFlags, as it's currently defined for AllocateRayQuery.
No validation mask is needed for the new OMM flags. Source level validation is still needed to ensure the flags are used correctly, but the validator will not need to run the flags through a mask to validate them. This PR updates the spec to reflect this change to the validator, and proposes some new diagnostics with the new RAYQUERY_FLAG::RAYQUERY_FLAG_ALLOW_OPACITY_MICROMAPS flag.
This PR is a transfer from #374
Fixes #375
Fixes #360