Skip to content

Conversation

@python3kgae
Copy link
Contributor

@python3kgae python3kgae commented Aug 14, 2024

Replace the memcpy check for PSV0 part.
Build DxilPipelineStateValidation object from data in PSV0 part. Then compare the content with information from input DxilModule.

With this change, the order of signature elements and resources could be different between the container and the DxilModule.
And the StringTable could be in different order as well.

Second step for #6817

@python3kgae python3kgae requested a review from a team as a code owner August 14, 2024 03:52
Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

I feel like there is a lot of missing test coverage here. I don't see tests for the various errors. I also think the error messages don't seem to be particularly descriptive, so I'm not even sure how a compiler implementer would interpret the errors and know what was wrong.

@python3kgae python3kgae marked this pull request as draft August 15, 2024 22:09
@github-actions
Copy link
Contributor

github-actions bot commented Aug 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@python3kgae python3kgae marked this pull request as ready for review September 5, 2024 18:32
@python3kgae python3kgae requested a review from damyanp September 10, 2024 16:31
Copy link
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

This does two major things I don't think we should do:

  • relaxes ordering requirement for signature elements relative to DxilModule (absolutely should not do this)
  • relaxes ordering requirement for resources relative to DxilModule (I don't see any need or advantage to this)

I have other more minor comments, and I haven't completed reviewing some parts and the tests at this point.

@python3kgae
Copy link
Contributor Author

This does two major things I don't think we should do:

  • relaxes ordering requirement for signature elements relative to DxilModule (absolutely should not do this)
  • relaxes ordering requirement for resources relative to DxilModule (I don't see any need or advantage to this)

I have other more minor comments, and I haven't completed reviewing some parts and the tests at this point.

Updated.

Copy link
Collaborator

@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 appreciate the new code consolidation. I was glad to see how well LoadViewIDStateFromPSV subbed in for the SimpleViewID class. I still think the error messages are unwieldy, but I'm willing to defer to you there.

Copy link
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

One potential missing validation:
Where does it make sure the PSV part size is the expected size for the data for that PSV version based on the validator version? Otherwise it could contain extra data tacked on to the end which would get ignored by the validator.

@python3kgae python3kgae merged commit 5155b09 into microsoft:main Sep 19, 2024
@python3kgae python3kgae deleted the psv_content branch September 19, 2024 14:32
@python3kgae
Copy link
Contributor Author

One potential missing validation: Where does it make sure the PSV part size is the expected size for the data for that PSV version based on the validator version? Otherwise it could contain extra data tacked on to the end which would get ignored by the validator.

Created #6924 to check PSV part size.

SjMxr233 pushed a commit to ShaderHelper/DirectXShaderCompiler that referenced this pull request Jul 24, 2025
Replace the memcpy check for PSV0 part.
Build DxilPipelineStateValidation object from data in PSV0 part. Then
compare the content with information from input DxilModule.

With this change, the order of signature elements and resources could be
different between the container and the DxilModule.
And the StringTable could be in different order as well.

Second step for microsoft#6817
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.

5 participants