Add HLSL Matrix expression #357
Conversation
fixes llvm#354 Adds a working plan for the new Expression type.
| (e.g., M._m00_m01, M._m10_m01 = 1.xx). Existing AST constructs such as | ||
| MatrixSubscriptExpr or ExtVectorElementExpr cannot fully capture this behavior | ||
| because they would not exactly match source spelling and per-component locations | ||
| and would not have correct l-value semantics and duplication rules. |
There was a problem hiding this comment.
I think we could extend the ExtVectorElementExpr if we wanted to. That AST node is used with a bunch of different spellings not all of which HLSL supports, (see: hi, lo, even and odd). It also handles the l-value semantics correctly.
I'm not saying this is the right direction, but it might be worth more consideration and a discussion with stakeholders outside this team.
There was a problem hiding this comment.
I've been experimenting with this approach and think it has legs. I put up an RFC here:
https://discourse.llvm.org/t/rfc-extend-extvectorelementexpr-for-hlsl-matrix-accessors/88802
| * Preserve exact spelling (token sequence after the dot) and per-component | ||
| source locations for faithful printing and rewriting. | ||
| + That means we store source location start and stop for each matrix element | ||
| accessor in the swizzle. |
There was a problem hiding this comment.
I don't think we need source locations for each element in the accessor, we just need to know the elements being accessed and whether they use 0-indexed or 1-indexed spelling.
Storing source locations for the start and stop of each accessor would significantly increase the memory requirements for the AST node and that would be bad.
| Stmt *Base; // matrix-typed expression | ||
| llvm::SmallVector<Component, 4> Comps; // selected (r,c) list | ||
| SourceLocation DotLoc, UnderLoc; // '.' and first '_' (after dot) | ||
| StringRef FullSuffixSpelling; // e.g. "_m00_m01" (owned by ASTContext) |
There was a problem hiding this comment.
A pointer to the IdentifierInfo for the spelling is more idiomatic than storing the StringRef, and we can quickly pull the name from the IdentifierInfo.
| StringRef FullSuffixSpelling; // e.g. "_m00_m01" (owned by ASTContext) | ||
| bool FromIdentifierToken : 1; // was lexed as one ident (i.e., one index) | ||
| bool HasDuplicates : 1; | ||
| }; |
There was a problem hiding this comment.
The ExtVectorElementExpr handles all the cases you need with an AST node that is a fraction the size. It does make the conscious decision to re-compute (via re-parsing) some data when needed, I'd probably start with a design along those lines and consider caching information only if that causes performance problems.
farzonl
force-pushed
the
matrix_accessor_swizzle_proposal
branch
from
e2960e7 to
feddcdd
Compare
2 participants
fixes #354
Adds a working plan for the new Expression type.