-
Notifications
You must be signed in to change notification settings - Fork 781
[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
Conversation
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.
This got lost somewhere
There was a problem hiding this 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.
tools/clang/test/SemaHLSL/hlsl/types/invalid_longvec_decls.hlsl
Outdated
Show resolved
Hide resolved
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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
There was a problem hiding this 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.
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
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
// 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. |
There was a problem hiding this comment.
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.
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