[SM6.10] Add built-in type for LinAlg Matrix handle #8090
Conversation
Adds a built-in type `__builtin_LinAlg_Matrix` that will be used in LinAlg Matrix implementation.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
LGTM, but you probably want an approval from someone more knowledgeable.
|
About to start a deeper review but wanted to get this out there as quickly as possible. How do you feel about Looking back on chat history it looks like I +1'd My thoughts on why are below:
If you are okay with the change I'm more than happy to do the chore work of renaming everything |
| case BuiltinType::Int8_4Packed: Out << "int8_t4_packed"; break; | ||
| case BuiltinType::UInt8_4Packed: Out << "uint8_t4_packed"; break; | ||
| case BuiltinType::LinAlgMatrix: | ||
| Out << "23__builtin_LinAlg_Matrix"; |
There was a problem hiding this comment.
What does 23 refer to?
Plus, don't we need to capture the matrix properties into mangling?
Or maybe we should never expect to use/allow mangling with this type, since types passed through user functions should always be the linalg::Matrix wrapper class, and builtins will be handled in a custom way, without using this mangling.
There was a problem hiding this comment.
23 is the length of "__builtin_LinAlg_Matrix". This is mangling of just the built-in base type. The matrix properties and the corresponding type mangling are added in a separate PR.
Additionally, this is Itanium mangling, so this is actually never used by DXC. I have added it here for completeness since we already have entries for other HLSL types.
| Out << "$ui8_4pk@"; | ||
| break; | ||
| case BuiltinType::LinAlgMatrix: | ||
| Out << "$linalg_matrix@"; |
There was a problem hiding this comment.
As before, don't we need to capture matrix properties into mangling?
There was a problem hiding this comment.
This is mangling of just the base built-in type, which can be spell out in the code, although the __builtin prefix should discourage people from doing that. The attributes are added in a separate PR.
| case BuiltinType::OCLSampler: ID = PREDEF_TYPE_SAMPLER_ID; break; | ||
| case BuiltinType::OCLEvent: ID = PREDEF_TYPE_EVENT_ID; break; | ||
| case BuiltinType::LinAlgMatrix: | ||
| ID = PREDEF_TYPE_LINALG_MATRIX_ID; |
There was a problem hiding this comment.
Fine, but we never supported AST serialization in this code base, and there are many missing pieces already. That's why we never added the other HLSL types here.
V-FEXrt
left a comment
V-FEXrt
left a comment
There was a problem hiding this comment.
LGTM*, like Damyan I'm going to leave someone else to approve since this shape of adding a type is very new looking to me :)
*of course I'd like to discuss the time name as mentioned above
I agree, |
4 participants
Adds a built-in type
__builtin_LinAlgMatrixthat will be used in LinAlg Matrix implementation for SM 6.10.Closes #8121