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

Disallow structs, arrays, and overwide vectors from typed buffers #7130

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pow2clk
Copy link
Member

@pow2clk pow2clk commented Feb 7, 2025

These never reliably generated the right results, so they are being disallowed. The check takes place in SemaHLSL and replaces a few places that checked for such things in special cases. To facilitate the check, the definition of the texture bit was expanded to include RW textures. This is consistent with the description and since the bit was never actually used before, has no other consequences.

Some code that executed after Sema that either produced errors or processed resources with these now forbidden types was removed. Most of this is from CodeGen and HLOperationLower where code was generated or expanded for load/store operations on such resources.

Additionally moved the check for overlarge elements in typed resources to enable keeping some tests around that tested these side by side as well as adding a new one. I changed the nature of the check a bit to limit the types to four elements and also that they be no larger than 4 32-bit elements. This means that 64-bit elements are limited to two. While there wasn't an error that caught this before, there was an assert that fired.

Incidentally fixes an issue with error reporting for modified types after the first mismatch of that intrinsic is encountered. Because the insertion code changed them in the temp array and the stored function type, but printed from the temp array and the later encounters didn't add the modifiers to the temp array, later errors would incorrectly leave off the reference. By retrieving the type from the function type, the reference is always preserved.

This required changing a fair number of tests that relied on this behavior including removing test cases and entire test files that existed solely for such tests.

Fixes #7080

…didates

The param array populated by MatchArguments only gets modifiers such as references applied when the intrinsic is new and needs to be added to the cache. By using the type of the parameter in the found candidate function regardless of whether it was created and added or just retrieved maintains the modifier for the sake of error messages giving the correct result for failed conversions of the same intrinsic after the first.
@pow2clk pow2clk requested a review from a team as a code owner February 7, 2025 08:15
@pow2clk
Copy link
Member Author

pow2clk commented Feb 7, 2025

The first four commits are intended to facilitate reviewing of mostly independent parts:

  • 61d8196 is incidental, but saved me from having to make a nonsensical change to atomics-on-bitfields.hlsl because the first encounter of the atomic ops would be removed, making the second the first which correctly had the references. Now they all correctly do
  • 00f7e11 is the meat of the change. This is where the new checks are added along with the tests targeted to this behavior.
  • 36af8da is a cleanup that the last commit allows. Because the errors are caught in Sema, later processing that accounted for these sorts of resources can be removed
  • 04f476a groups all the messy test cleanups into one change. Most of these are removing tests that focused on the disallowed behavior.

These never reliably generated the right results, so they are being disallowed. The check takes place in SemaHLSL and replaces a few places that checked for such things in special cases. To facilitate the check, the definition of the texture bit was expanded to include RW textures. This is consistent with the description and since the bit was never actually used before, has no other consequences.

Some code that executed after Sema that either produced errors or processed resources with these now forbidden types was removed. Most of this is from CodeGen and HLOperationLower where code was generated or expanded for load/store operations on such resources.

Additionally moved the check for overlarge elements in typed resources to enable keeping some tests around that tested these side by side as well as adding a new one. I changed the nature of the check a bit to limit the types to four elements and also that they be no larger than 4 32-bit elements. This means that 64-bit elements are limited to two. While there wasn't an error that caught this before, there was an assert that fired.

Incidentally fixes an issue with error reporting for modified types after the first mismatch of that intrinsic is encountered. Because the insertion code changed them in the temp array and the stored function type, but printed from the temp array and the later encounters didn't add the modifiers to the temp array, later errors would incorrectly leave off the reference. By retrieving the type from the function type, the reference is always preserved.

This required changing a fair number of tests that relied on this behavior including removing test cases and entire test files that existed solely for such tests.

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

Successfully merging this pull request may close these issues.

Disallow anything but scalars and vectors in Typed Buffers
1 participant