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.

Sign up for GitHub

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

Jump to bottom

[DxilOp] Allow generation of illegal DXIL operations. #6543

Merged
merged 5 commits into from
Apr 26, 2024
Merged

[DxilOp] Allow generation of illegal DXIL operations. #6543

merged 5 commits into from
Apr 26, 2024

Conversation

python3kgae
Copy link
Contributor

@python3kgae python3kgae commented Apr 17, 2024 •
edited
Loading

This change removes the IsOverloadLegal check in OP::GetOpFunc.

It will permit the generation of illegal DXIL operations. Subsequently, the validation should catch these illegal DXIL operations if they are not optimized later in SimplifyDxilCall.

Fixes #6410

@python3kgae
[DxilOp] recover old behavior of OP::GetOpFunc
d255599 
This change change move IsOverloadLegal check in OP::GetOpFunc back into DXASSERT.

It will allow illegal dxil op generated for release build.
Then validation should catch those illegal dxil ops if they're not optimized in SimplifyDxilCall.

Fixes microsoft#6410
@python3kgae python3kgae requested a review from a team as a code owner April 17, 2024 20:50
tex3d
tex3d reviewed Apr 17, 2024
View reviewed changes
tools/clang/test/lit.cfg Outdated Show resolved Hide resolved
tex3d
tex3d reviewed Apr 17, 2024
View reviewed changes
lib/DXIL/DxilOperations.cpp Outdated Show resolved Hide resolved
@damyanp
Copy link
Member

damyanp commented Apr 18, 2024

@llvm-beanz - can you have a look at this please? This is the more long-term solution to the issue we had with the 2403 release.

@pow2clk
Copy link
Member

pow2clk commented Apr 22, 2024

The description is no longer accurate as we're not moving it into an active DXASSERT

@python3kgae python3kgae changed the title [DxilOp] recover old behavior of OP::GetOpFunc [DxilOp] Allow generation of illegal DXIL operations. Apr 22, 2024
@python3kgae
Copy link
Contributor Author

The description is no longer accurate as we're not moving it into an active DXASSERT

Fixed.

pow2clk
pow2clk reviewed Apr 22, 2024
View reviewed changes
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.

Per this comment: #6410 (comment) This revert should not "fix" #6410. I think the description and comments should be updated to reflect the removal of the assert as well.

tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl Outdated Show resolved Hide resolved
tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl Outdated Show resolved Hide resolved
tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl Outdated Show resolved Hide resolved
tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl Outdated Show resolved Hide resolved
@python3kgae
Copy link
Contributor Author

Created #6566 and #6565 to track non-literal crashes.

This was referenced Apr 24, 2024
Closed
Closed
@tex3d
Copy link
Contributor

tex3d commented Apr 24, 2024

Created #6566 and #6565 to track non-literal crashes.

Looks good, though I think they shouldn't be considered crashes, since that's a temporary state of affairs that will change once this PR merges.

Also these issues don't cover the TODO comments in the test (// TODO: handle fp specials properly!). These are about the fact that we won't constant-fold generated literal DXIL ops if they produce FP exceptions (or FP specials), and we need to test specific expected results for operations involving FP specials. While issue #6557 is likely caused by this problem, it's about more than just this issue. I think we need another issue to track this problem. I had added a section for these to the fmod test when #6437 was initially created against the release branch. But I removed that section because it was disabled/unused and I wasn't sure if I got the expected results correct. This code could be used to craft a repro for an issue covering this TODO.

Here was the code:
d93ffad#diff-557ef4666494c6d42b516718ea7ed954e7e7153efc844f9dfd0678b9b53edb39R29-R42

@llvm-beanz
Copy link
Collaborator

Are #6566 & #6565 fixed with this change? From the surface those look like duplicates of #6410, but for different intrinsics, so I'm unsure why we have extra issues filed.

@python3kgae
Copy link
Contributor Author

Are #6566 & #6565 fixed with this change? From the surface those look like duplicates of #6410, but for different intrinsics, so I'm unsure why we have extra issues filed.

No. current PR will not fix #6566 and #6565.
They fail for different reason which looks similar.

@llvm-beanz
Copy link
Collaborator

To clarify as we discussed in triage this morning. Once this change goes in the issues described originally as crashes in #6566 & #6565, revert to the pre-existing behavior of producing invalid DXIL that fails validation with a validator error.

Both issues are closed as per @damyanp's comment:

We have no plans to make changes in this area in DXC without considering a design that takes into account what the design is in Clang. For this reason I'm closing this issue as not planned.

@python3kgae
Copy link
Contributor Author

Created #6566 and #6565 to track non-literal crashes.

Looks good, though I think they shouldn't be considered crashes, since that's a temporary state of affairs that will change once this PR merges.

Also these issues don't cover the TODO comments in the test (// TODO: handle fp specials properly!). These are about the fact that we won't constant-fold generated literal DXIL ops if they produce FP exceptions (or FP specials), and we need to test specific expected results for operations involving FP specials. While issue #6557 is likely caused by this problem, it's about more than just this issue. I think we need another issue to track this problem. I had added a section for these to the fmod test when #6437 was initially created against the release branch. But I removed that section because it was disabled/unused and I wasn't sure if I got the expected results correct. This code could be used to craft a repro for an issue covering this TODO.

Here was the code: d93ffad#diff-557ef4666494c6d42b516718ea7ed954e7e7153efc844f9dfd0678b9b53edb39R29-R42

Created #6567 to track the fp special value issue.

pow2clk
pow2clk reviewed Apr 26, 2024
View reviewed changes
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.

Just some changes to commments really.

tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl Outdated Show resolved Hide resolved
lib/DXIL/DxilOperations.cpp Outdated Show resolved Hide resolved
pow2clk
pow2clk approved these changes Apr 26, 2024
View reviewed changes
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.

Thanks!

@python3kgae python3kgae enabled auto-merge (squash) April 26, 2024 20:02
@python3kgae python3kgae merged commit 00cd823 into microsoft:main Apr 26, 2024
12 checks passed
@python3kgae python3kgae deleted the illegal_dxil_op branch April 26, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tex3d tex3d tex3d approved these changes

@pow2clk pow2clk pow2clk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
HLSL Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DXIL] Null pointer exception due to double floating-point literals in fmod intrinsic
5 participants
@python3kgae @damyanp @pow2clk @tex3d @llvm-beanz