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

Add tests for literal overloads of fmod, length, and normalize #6437

Merged

Conversation

tex3d
Copy link
Contributor

@tex3d tex3d commented Mar 20, 2024

Certain HL ops with no double overload will generate double overloads for literal types. These are lowered to double DXIL overloads, some of which are not legal for these ops. We currently rely on illegal intermediate DXIL op overloads for constant evaluation. If constant evaluation fails, we end up with illegal DXIL overloads in final DXIL, which is caught by the validator.

The prior revert restored the ability to run these scenarios (with asserts disabled).

This change adds tests for #6419 so automated testing will test across configurations and prevent regressions of this scenario in this branch.

Certain HL ops with no double overload will generate double overloads for literal types.
These are lowered to double DXIL overloads, some of which are not legal for these ops.
We currently rely on illegal intermediate DXIL op overloads for constant evaluation.

Disabling this assert allows test cases to be written to verify the current behavior for these operations.
@tex3d tex3d requested a review from a team as a code owner March 20, 2024 02:56
@tex3d tex3d changed the title Remove assert on illegal DXIL overload Remove assert on illegal DXIL overload - add tests Mar 20, 2024
- Last minute reordering of NO_FOLD to the end caused unary sqrt op function to be listed first, and the first illegal overload is the only one reported.
- Changed NO_FOLD test to directly use full normalize output, instead of just the first component.
What matters is 'error: DXIL intrinsic overload must be valid.'
It's unnecessary to check the specific intrinsic reported,
partly because it will only report the first one in function declaration order,
which could change with changes to other code.

The purpose of the test is to make sure the validator gracefully caught the invalid DXIL overload.
Also remove unused, incomplete, and potentally wrong SPECIALS section from fmod_const_eval.hlsl.
@tex3d tex3d changed the title Remove assert on illegal DXIL overload - add tests Add tests for literal overloads of fmod, length, and normalize Mar 21, 2024
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 suppose it's fine, but it has no effect at present.

@tex3d tex3d merged commit 9c2b828 into microsoft:release-1.8.2403 Mar 21, 2024
14 checks passed
@tex3d tex3d deleted the rem-assert-on-bad-dxil-overload branch March 21, 2024 19:57
@tex3d
Copy link
Contributor Author

tex3d commented Mar 21, 2024

I suppose it's fine, but it has no effect at present.

Other than testing this scenario across configurations in our build pipeline, and preventing any further changes in the branch from regressing it, right?

pow2clk pushed a commit to pow2clk/DirectXShaderCompiler that referenced this pull request Jul 17, 2024
…soft#6437)

Certain HL ops with no double overload will generate double overloads
for literal types. These are lowered to double DXIL overloads, some of
which are not legal for these ops. We currently rely on illegal
intermediate DXIL op overloads for constant evaluation. If constant
evaluation fails, we end up with illegal DXIL overloads in final DXIL,
which is caught by the validator.

The prior revert restored the ability to run these scenarios (with
asserts disabled).

This change adds tests for microsoft#6419 so automated testing will test across
configurations and prevent regressions of this scenario in this branch.

(cherry picked from commit 9c2b828)
pow2clk added a commit that referenced this pull request Jul 17, 2024
#6799)

Certain HL ops with no double overload will generate double overloads
for literal types. These are lowered to double DXIL overloads, some of
which are not legal for these ops. We currently rely on illegal
intermediate DXIL op overloads for constant evaluation. If constant
evaluation fails, we end up with illegal DXIL overloads in final DXIL,
which is caught by the validator.

The prior revert restored the ability to run these scenarios (with
asserts disabled).

This change adds tests for #6419 so automated testing will test across
configurations and prevent regressions of this scenario in this branch.

(cherry picked from commit 9c2b828)

Co-authored-by: Tex Riddell <[email protected]>
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.

3 participants