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

RaytracingTier1_1 flag should not be set in module based on subobject #6321

Closed
tex3d opened this issue Feb 16, 2024 · 2 comments · Fixed by #6320 or #6333
Closed

RaytracingTier1_1 flag should not be set in module based on subobject #6321

tex3d opened this issue Feb 16, 2024 · 2 comments · Fixed by #6320 or #6333
Labels
bug Bug, regression, crash sm6.8 Shader Model 6.8

Comments

@tex3d
Copy link
Contributor

tex3d commented Feb 16, 2024

Description
DxilModule checks StateObjectConfig in subobjects for AllowStateObjectAdditions, then sets the module flag RaytracingTier1_1 based on this, if not already set for some other reason.

This is incorrect, as the runtime will enforce feature requirement based on the subobjects used, and the module flag will prevent a library containing the subobject from being created, even if this Tier 1.1 subobject is not used.

On top of this, since subobjects are removed from the module during container serialization, recomputation of flags for validation will not see the requirement. This doesn't fail right now because flag validation is disabled for libraries (which it shouldn't be). This validation will be enabled for 1.8, so it's necessary to fix this issue before DXIL 1.8 release.

Steps to Reproduce

// dxc -T lib_6_5
StateObjectConfig soc = { STATE_OBJECT_FLAGS_ALLOW_LOCAL_DEPENDENCIES_ON_EXTERNAL_DEFINITONS | STATE_OBJECT_FLAG_ALLOW_STATE_OBJECT_ADDITIONS };

"Raytracing tier 1.1 features" should not be a required feature for this module.

Actual Behavior
Disassembly output states that "Raytracing tier 1.1 features" is required.

Environment

  • DXC version: any
  • Host Operating System: any
@tex3d tex3d added bug Bug, regression, crash needs-triage Awaiting triage labels Feb 16, 2024
@tex3d tex3d added sm6.8 Shader Model 6.8 and removed needs-triage Awaiting triage labels Feb 16, 2024
@tex3d tex3d removed this from HLSL Triage Feb 16, 2024
@github-project-automation github-project-automation bot moved this to 🆕 New in HLSL Roadmap Feb 16, 2024
@tex3d tex3d moved this from 🆕 New to 👀 In review in HLSL Roadmap Feb 16, 2024
@tex3d
Copy link
Contributor Author

tex3d commented Feb 16, 2024

Note, in PR #6207, the computation of RaytracingTier1_1 flag was moved from inside per-function flag computation into per-module (in DxilModule).

It turns out that this didn't change the behavior overall, because before #6207, subobjects got stripped from the module just before the per-function RDAT flags were computed, and the module flag still had RaytracingTier1_1 set because they were computed before that point during DxilPreparePasses.

tex3d added a commit that referenced this issue Feb 16, 2024
…on (#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 #6321
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in HLSL Roadmap Feb 16, 2024
tex3d added a commit to tex3d/DirectXShaderCompiler that referenced this issue 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 issue 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)
@tex3d
Copy link
Contributor Author

tex3d commented Feb 21, 2024

It turns out that this didn't change the behavior overall, because before #6207, subobjects got stripped from the module just before the per-function RDAT flags were computed, and the module flag still had RaytracingTier1_1 set because they were computed before that point during DxilPreparePasses.

It turns out this was wrong. The flags were computed before stripping the subobjects. Merging the change based on this assumption causes a failure when testing against the previous released validator.

Reactivating to merge a more complete fix which restores this only for validator version 1.7 and below.

@tex3d tex3d reopened this Feb 21, 2024
tex3d added a commit that referenced this issue Feb 21, 2024
It turns out that in the prior validator version, if a subobject
required DXR 1.1, the DXR 1.1 flag would be set on each function in RDAT
output, as well as the global flags. It didn't appear this was the case
through a D3DReflect test because that goes through disassembly to
assembly step, where subobjects are lost because they aren't in the llvm
IR. The previous change assumed this was not the case when the
subobjects were removed, but this removal occurs after the RDATWriter
constructor, which does the full RDAT serialization since size if
required right away.

This restores the detection code and hooks it into
DxilModule::ComputeShaderCompatInfo when validator version is in range
[1.5, 1.7]. DXR 1.1 was introduced in 1.5, and we no longer tag every
function as requiring DXR 1.1 based on subobjects in 1.8.

At the moment, there is no way to CHECK the subobject RDAT in D3DReflect
because they get stripped from the module (even reflection and debug
modules) before serialization, and the test path for D3DReflect goes
through a disassemble/re-assemble step between the prior stage and the
D3DReflect stage.

Fixes #6321 (really).
tex3d added a commit to tex3d/DirectXShaderCompiler that referenced this issue Feb 21, 2024
It turns out that in the prior validator version, if a subobject
required DXR 1.1, the DXR 1.1 flag would be set on each function in RDAT
output, as well as the global flags. It didn't appear this was the case
through a D3DReflect test because that goes through disassembly to
assembly step, where subobjects are lost because they aren't in the llvm
IR. The previous change assumed this was not the case when the
subobjects were removed, but this removal occurs after the RDATWriter
constructor, which does the full RDAT serialization since size if
required right away.

This restores the detection code and hooks it into
DxilModule::ComputeShaderCompatInfo when validator version is in range
[1.5, 1.7]. DXR 1.1 was introduced in 1.5, and we no longer tag every
function as requiring DXR 1.1 based on subobjects in 1.8.

At the moment, there is no way to CHECK the subobject RDAT in D3DReflect
because they get stripped from the module (even reflection and debug
modules) before serialization, and the test path for D3DReflect goes
through a disassemble/re-assemble step between the prior stage and the
D3DReflect stage.

Fixes microsoft#6321 (really).

(cherry picked from commit 22bb078)
tex3d added a commit that referenced this issue Feb 21, 2024
It turns out that in the prior validator version, if a subobject
required DXR 1.1, the DXR 1.1 flag would be set on each function in RDAT
output, as well as the global flags. It didn't appear this was the case
through a D3DReflect test because that goes through disassembly to
assembly step, where subobjects are lost because they aren't in the llvm
IR. The previous change assumed this was not the case when the
subobjects were removed, but this removal occurs after the RDATWriter
constructor, which does the full RDAT serialization since size if
required right away.

This restores the detection code and hooks it into
DxilModule::ComputeShaderCompatInfo when validator version is in range
[1.5, 1.7]. DXR 1.1 was introduced in 1.5, and we no longer tag every
function as requiring DXR 1.1 based on subobjects in 1.8.

At the moment, there is no way to CHECK the subobject RDAT in D3DReflect
because they get stripped from the module (even reflection and debug
modules) before serialization, and the test path for D3DReflect goes
through a disassemble/re-assemble step between the prior stage and the
D3DReflect stage.

Fixes #6321 (really).

(cherry picked from commit 22bb078)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash sm6.8 Shader Model 6.8
Projects
Archived in project
1 participant