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

Fix a PIX pass's attempt to set the validator version #6707

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

jeffnn
Copy link
Collaborator

@jeffnn jeffnn commented Jun 19, 2024

This pass was attempting to compare different things. The return values of GetDxilVersion are not shader models, but... dxil version. Since the code is trying to upgrade the validator version, I changed this to GetValidatorVersion, to pair with SetValidatorVersion.
The previous code was breaking the nvidia driver on workgraphs.

@jeffnn jeffnn self-assigned this Jun 19, 2024
@jeffnn jeffnn requested a review from a team as a code owner June 19, 2024 23:46
Copy link
Contributor

github-actions bot commented Jun 19, 2024

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

Copy link
Collaborator

@bob80905 bob80905 left a comment

Choose a reason for hiding this comment

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

Though I also don't understand the reasoning, the code and tests look correct. Although I would personally put a blurb on top of each test specifying why each test exists. (Like for example, in validatorNoDowngrade, "This test tests that a shader with dxil validator version of 1.8 does not downgrade to 1.4" and for validatorUpgrade "This test tests that a shader with dxil validator version of 1.3 gets upgraded to 1.4 after the annotate with virtual regs pass.")

@jeffnn
Copy link
Collaborator Author

jeffnn commented Jun 26, 2024

Though I also don't understand the reasoning, the code and tests look correct. Although I would personally put a blurb on top of each test specifying why each test exists. (Like for example, in validatorNoDowngrade, "This test tests that a shader with dxil validator version of 1.8 does not downgrade to 1.4" and for validatorUpgrade "This test tests that a shader with dxil validator version of 1.3 gets upgraded to 1.4 after the annotate with virtual regs pass.")

Good idea. Your comment suggestions used, almost verbatim

@jeffnn jeffnn closed this Jun 26, 2024
@jeffnn jeffnn reopened this Jun 26, 2024
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.

The change looks fine, though I still don't know why the validator version was upgraded in the first place.

@jeffnn jeffnn merged commit 1fefbc4 into microsoft:main Jul 1, 2024
13 checks passed
@jeffnn jeffnn deleted the PIX_ValidatorUpgrade branch July 1, 2024 19:58
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.

None yet

3 participants