Skip to content

[SM6.9] Allow native vectors longer than 4 #7143

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

Merged
merged 22 commits into from
Mar 11, 2025

Conversation

pow2clk
Copy link
Contributor

@pow2clk pow2clk commented Feb 18, 2025

Remove errors in Sema diagnostics for vectors longer than 4 in 6.9.
Test for failures using long vectors in unspported contexts and for correct codegen in
supported contexts. Verify errors persist in pre-6.9 shader models

The type buffer cache expects a max vector size of 4. By just skipping the cache for longer vectors, we don't overrun and store float7 vectors in the double3 slot or retrieve the double3 in place of float7.

Testing is for acceptance, mangling and basic copying that takes place
at the high level to ensure they are being accepted and recognized
correctly. The intent is not to tully test the passing of data as that
requires enabling vector operations to do properly. This test is used to
verify that these same constructs are disallowed in 6.8 and earlier.

A separate test verifies that disallowed contexts produce the
appropriate errors

Fixes #7117

Remove errors in Sema diagnostics for vectors longer than 4 in 6.9.
Test for failures using long vectors in unspported contexts and for correct codegen in
supported contexts. Verify errors persist in pre-6.9 shader models

The type buffer cache expects a max vector size of 4. By just skipping the cache for longer vectors, we don't overrun and store float7 vectors in the double3 slot or retrieve the double3 in place of float7.

Testing is for acceptance, mangling and basic copying that takes place
at the high level to ensure they are being accepted and recognized
correctly. The intent is not to tully test the passing of data as that
requires enabling vector operations to do properly. This test is used to
verify that these same constructs are disallowed in 6.8 and earlier.

A separate test verifies that disallowed contexts produce the
appropriate errors

Fixes microsoft#7117
Disallow long vectors, and arrays or structs containing long vectors in
cbuffers, entry functions, node records, tessellation patchs, or special intrinsic parameters with
user-defined struct parameters.
@pow2clk pow2clk requested a review from a team as a code owner February 18, 2025 16:20
@llvm-beanz llvm-beanz self-assigned this Feb 18, 2025
Copy link
Contributor Author

@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 for the feedback! Followup commit soon.

pow2clk added 5 commits March 3, 2025 09:40
Expand resource attribute to all resource types by adding reskind and
resclass arguments indicating the specific resource type. Change detection
in HlslTypes to use these attribute arguments. Similarly add vertex
number arguments to output stream attribute and a boolean indicator of
input or output for tessellation patches.

Add geomstream attr to detect those objects

Use attribute to detect tesselation patches
Removes template arg counts and startswith stirngs to identify
tesslations patches and distinguish them from multisampled textures
Add setting for max vec size.

Determine long vector presence using DefinitionData bit?
OR
Rename testing for long vecs function?

Add attribute for geometry streams, produce and test errors for long vectors there.

Add and test errors for > 1024 element vectors.

Add vector size to error messages

good test changes
Go for consistent test filename formatting. most LLVM tests have dashes,
so dashes it is. Remove redundant sm68 test
Expand existing tests to different target and contexts. Add thorough
testing for geometry streams and tessellation patches.

Add toolong vector test. Verify that vectors that are over the maximum
for 6.9 fail.

Add subobjects and template classes to tests. These are unfortunately
disabled because the code to make them work causes other tests to fail.
Copy link
Contributor

github-actions bot commented Mar 3, 2025

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

pow2clk and others added 7 commits March 3, 2025 13:00
Use RequireCompleteType to force specialization of templates encountered
in global and other scopes where finding long vectors is necessary where
possible. This populates the definitiondata which contains the base
class chain needed to detect when a base class has disqualifying long
vectors. It was also needed to detect when dependent types in a template
class result in long vectors.

Work graph node types didn't check their base classes for failures. This
affects base classes with longvectors that have sub classes used for
node objects which should fail for having long vector members.

Respond to feedback about iterating through fields in clunky manner
which got left out of the last reviewer feedback response
I guess it was about time. Should simplify some things later as well as at present and it was too easy to not do. Specifically, I was going to need to add another string check to the template instantiation code to identify longvectors. This is cleaner.

Incidentally convert another feedback texture string check to use attribs.

Incidentally resort the recently-added attribs to not break up the node shader attribs.
Vector types can be cached in a 2D array that has a column for lenghts 1-4. This uses the added contant to indicate the length and for the checks that confirm it isn't exceeded.
By setting the bit when the vector template is instantiated and then propagating it when members, be they standard members or base classes, the bit will be set correctly for any struct or struct-like type. For arrays, the arrays are pealed away in a utility function to get at the elements.

Decided to separate the check for completeness from the check for long vectors. Even though the latter almost always requires the former, they are separate concepts and embedding the first in the second would be unexpected
Output Streams, Tessellation patches, and global variables should be complete when receiving other correctness checks. If they cannot be made complete, they should produce an error. This was omitted for various of these including non-template globals, which was fine, but it meant that redundant errors were produced for templates, but not standard globals likely just because that was what was tested. This removes that distinction and adds testing for all of the above to the existing incomplete-type.hlsl test.
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.

Finished with code review part. Haven't thoroughly reviewed all the tests yet.

@damyanp damyanp assigned pow2clk and unassigned tex3d Mar 7, 2025
remove some stale elements. Add some HLSL type helper functions and add some new ones. Make resource type retreiveals type-safe. Add some parameter comments and names to make clearer what the effect of them are. Pass resource attribute to cbuffer/tbuffer creation. Clean up and clarify error messages. Remove redundant type canonization from type queries. Correct resclass of tbuffers. Use multimatch utility of verify to condense checks
Copy link
Contributor Author

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

Respond to comments.

Correct UAVness of consume/append buffers.

Add HLSL notes for changes to DeclCXX

Share more code in IsHLSLVecMatType
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.

Looks good.

@pow2clk pow2clk merged commit 7c8005a into microsoft:main Mar 11, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in HLSL Roadmap Mar 11, 2025
@pow2clk pow2clk deleted the longvec_decls_pr branch March 11, 2025 00:07
// RUN: %dxc -T ps_6_9 -DTYPE=float -DNUM=1025 -verify %s

// A test to verify that declarations of longvecs are permitted in all the accepted places.
// Only tests for acceptance, most codegen is ignored for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

When you say "most codegen is ignored for now" do you mean until long vecs are implemented? Do we intend to use this hlsl file to verify codegen at some point? Just wondering if this comment is going to become stale.

@damyanp damyanp moved this from Needs Review to Closed in HLSL Support Apr 22, 2025
@damyanp damyanp removed this from HLSL Support Jun 25, 2025
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.

[SM6.9] Support long vector declaration, casting, and assignment
4 participants