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

[Validator] Check content of PSV0 part in validation #6859

Merged
merged 23 commits into from
Sep 19, 2024

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
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
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.

utils/hct/hctdb.py Outdated Show resolved Hide resolved
utils/hct/hctdb.py Outdated Show resolved Hide resolved
utils/hct/hctdb.py Outdated Show resolved Hide resolved
utils/hct/hctdb.py Outdated Show resolved Hide resolved
utils/hct/hctdb.py Outdated Show resolved Hide resolved
utils/hct/hctdb.py Outdated Show resolved Hide resolved
utils/hct/hctdb.py Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
@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.

lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
lib/DxilValidation/DxilContainerValidation.cpp Outdated Show resolved Hide resolved
tools/clang/unittests/HLSL/ValidationTest.cpp Show resolved Hide resolved
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 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
12 checks passed
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants