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

Don't set RaytracingTier1_1 based on subobjects; enable flag validation #6320

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

tex3d
Copy link
Contributor

@tex3d tex3d commented Feb 16, 2024

In validator version 1.7 and below, the RaytracingTier1_1 module flag was set on every function if any StateObjectConfig subobject had the AllowStateObjectAdditions flag set. This was incorrect, as the requirement is validated in the runtime based on the use of the subobject instead. Subobjects are removed from the module and placed in RDAT during container serialization, so the requirement would be lost when recomputing the flags in validation. This didn't break validation because flag validation was completely disabled for libraries!

This change fixes this problem, and allows DxilValidation to validate ShaderFlags because they will no longer mismatch due to this issue. Running CollectShaderFlagsForModule is also necessary for collecting per-function shader flags which will be used by call graph validation in a subsequent change, so enabling flag validation unblocks that as well.

Fixes #6321

@tex3d tex3d requested review from python3kgae and pow2clk February 16, 2024 18:52
In validator version 1.7 and below, the RaytracingTier1_1 module flag was set on every function if any StateObjectConfig subobject had the AllowStateObjectAdditions flag set.
This was incorrect, as the requirement is validated in the runtime based on the use of the subobject instead.
Subobjects are removed from the module and placed in RDAT during container serialization, so the requirement would be lost when recomputing the flags in validation.
This didn't break validation because flag validation was completely disabled for libraries!

This change fixes this problem, and allows DxilValidation to validate ShaderFlags because they will no longer mismatch due to this issue.
Running CollectShaderFlagsForModule is also necessary for collecting per-function shader flags which will be used by call graph validation in a subsequent change, so enabling flag validation unblocks that as well.
@tex3d tex3d force-pushed the no-rt11-flag-from-subobject branch from 8a7b441 to 06ab69a Compare February 16, 2024 19:30
Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to acknowledge #6207 and the changes it made to this code. The description refers to 1.7 behavior, but is actually removing code that was added by that change which confused me at first.

In particular, I'd appreciate an explanation of why any consideration of the subobjects isn't needed to determine whether raytracing 1.1 is required. After this change, it seems that the only way to set the raytracing 1.1 flag is if these opcodes are used:

case DXIL::OpCode::AllocateRayQuery:
case DXIL::OpCode::GeometryIndex:
hasRaytracingTier1_1 = true;

That doesn't sound like checking for the use of subobjects to me and I think there are other ops that make use of them?

@tex3d
Copy link
Contributor Author

tex3d commented Feb 16, 2024

I think it would be helpful to acknowledge #6207 and the changes it made to this code. The description refers to 1.7 behavior, but is actually removing code that was added by that change which confused me at first.

In particular, I'd appreciate an explanation of why any consideration of the subobjects isn't needed to determine whether raytracing 1.1 is required. After this change, it seems that the only way to set the raytracing 1.1 flag is if these opcodes are used:

case DXIL::OpCode::AllocateRayQuery:
case DXIL::OpCode::GeometryIndex:
hasRaytracingTier1_1 = true;

That doesn't sound like checking for the use of subobjects to me and I think there are other ops that make use of them?

The code was basically moved, not exactly added. I added a note to issue #6321. Let me know if that helps regarding the churn of that code.

That issue should also explain why this flag should not be set from the subobject, since for subobjects the feature requirement is checked independently in the runtime, and the subobjects are part of RDAT, and not part of the module.

There are multiple things that could require raytracing 1.1 though. As you see based on the link, a function could use a DXR 1.1 feature directly, in which case, the function that uses it will reflect the raytracing 1.1 requirement flag, and this requirement will bubble up to the module flags as well.

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

@tex3d tex3d enabled auto-merge (squash) February 16, 2024 22:54
@tex3d tex3d merged commit fafbc42 into microsoft:main Feb 16, 2024
12 checks passed
@tex3d tex3d deleted the no-rt11-flag-from-subobject branch February 16, 2024 23:21
tex3d added a commit to tex3d/DirectXShaderCompiler that referenced this pull request Feb 16, 2024
…on (microsoft#6320)

In validator version 1.7 and below, the RaytracingTier1_1 module flag
was set on every function if any StateObjectConfig subobject had the
AllowStateObjectAdditions flag set. This was incorrect, as the
requirement is validated in the runtime based on the use of the
subobject instead. Subobjects are removed from the module and placed in
RDAT during container serialization, so the requirement would be lost
when recomputing the flags in validation. This didn't break validation
because flag validation was completely disabled for libraries!

This change fixes this problem, and allows DxilValidation to validate
ShaderFlags because they will no longer mismatch due to this issue.
Running CollectShaderFlagsForModule is also necessary for collecting
per-function shader flags which will be used by call graph validation in
a subsequent change, so enabling flag validation unblocks that as well.

Fixes microsoft#6321

(cherry picked from commit fafbc42)
tex3d added a commit that referenced this pull request Feb 20, 2024
…on (#6320) (#6322)

In validator version 1.7 and below, the RaytracingTier1_1 module flag
was set on every function if any StateObjectConfig subobject had the
AllowStateObjectAdditions flag set. This was incorrect, as the
requirement is validated in the runtime based on the use of the
subobject instead. Subobjects are removed from the module and placed in
RDAT during container serialization, so the requirement would be lost
when recomputing the flags in validation. This didn't break validation
because flag validation was completely disabled for libraries!

This change fixes this problem, and allows DxilValidation to validate
ShaderFlags because they will no longer mismatch due to this issue.
Running CollectShaderFlagsForModule is also necessary for collecting
per-function shader flags which will be used by call graph validation in
a subsequent change, so enabling flag validation unblocks that as well.

Fixes #6321

(cherry picked from commit fafbc42)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

RaytracingTier1_1 flag should not be set in module based on subobject
3 participants