-
Notifications
You must be signed in to change notification settings - Fork 848
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
WIP: Implement swizzles containing 0 and 1 to represent numbers not indexes. #2208
base: main
Are you sure you want to change the base?
Conversation
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.
Looking good. I like it.
I think there might be a bug in the check for letter-class consistency?
error(loc, "vector swizzle selection out of range", compString.c_str(), ""); | ||
selector.resize(i); | ||
break; | ||
} | ||
|
||
if (i > 0 && fieldSet[i] != fieldSet[i-1]) { | ||
if (i > 0 && fieldSet[i] != enumeric && fieldSet[i-1] != enumeric && |
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.
This seems wrong. Would it catch the following bad case?
.x0u
Seems you need a longer lookback. Effectively I'd keep a state variable recording which letter-category has been forced so far. Then check for conflict.
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.
I see no problem here. Any struct could have a member x0u
. There were no changes to tokenizing (that's entirely in the preprocessor). And the preprocessor still doesn't know anything about swizzling.
Rather, this is about parsing and semantic analysis. The 'u' will just fail to be a valid swizzle selector, and you'll get a semantic error, same as always.
'x0u' : unknown swizzle selection
Think of .x0u
as a struct member that's allow on the vector type. It's just an identifier like any other.
Question
The reason I posted this as a draft is to see what direction to really go, and to show that one direction is non-trivial, despite appearances.
There are two independent decisions to make:
- (A) Emulate it (what this PR starts) or (B) add a new AST operator?
Emulating is this big complicated thing above, which is only partially implemented and will need lots of testing. Adding the new AST operator will mean that back ends must write new code to accept in order to support it.
- (C) Use a leading "_" or (D) change lexical analysis.
(D) Means adopting the new principle that the set of swizzle selectors cannot overlap the set of numeric suffixes, somewhat artificial and limiting to language growth and portability.
The fast trivial way to get it working is (B) and (C). This PR was to demonstrate the complexity of (A).
Do you have input on what direction to go?
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.
Sorry, I'm smart and dumb at the same time. The 'u' was wrong, should have been a swizzle letter from another swizzle letter-sequence.
This is the bad example:
#version 450
void main() {
vec4 old;
vec3 new = old.x0a;
}
This example compiles with this PR's branch. The problem is that the conflict between the x from the exyzw and the a from the ergba set is not detected because of the intervening 0.
So, that's the bug I was trying to point out.
Will think a bit more on your larger questions.
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.
I have clear preference for (C) over (D).
I think a leading underscore is unobtrusive to the user, and easy to remember.
When I was thinking of this on my own, I thought of the underscore as ignorable anywhere in the swizzle. This is in analogy to using underscore as an ignorable digit separator as appeared in Go 1.13 https://golang.org/doc/go1.13
But maybe that's overkill. I'm perfectly happy with an ignorable leading underscore.
And to double-check: we only look for a swizzle when the object it follows is a vector, not a structure. That's what makes this avoid naming conflict with any user's type definition.
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.
Ah, I see, right, it can no longer rely on transitivity of being in the same partition percolating through. Haven't written any tests yet, not knowing whether to even do this.
Will fix once having input on direction.
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.
I think I lean toward (B) over (A).
Questions:
- What backends exist other than the SPIR-V code generator?
- But also, I don't know the implications for folding in the front-end. I don't know what is required for GLSL support.
On the latter point, constant-folding of swizzles are required to be able to do things like size arrays:
#version 450
const ivec4 sizes = ivec4(10,20,30,40);
shared int w[sizes._1]; // with this PR's branch w is an array of size 1.
void main() {
w[0] = 1;
}
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.
Okay, I'll give (B) a shot sometime.
Right, GLSL mandates what's folded or not (and has to be, or not, for portability).
Add numeric swizzles, as suggested in twitter, to support things like
v.xyz1
. Just 0 and 1 are supported, mixed in with the normal letter swizzlers.While this looks trivial, it is actually quite involved to do it without changing the output interface.
Perhaps, this should only be done though as changing the output interface by adding a new operator that means this.
Without that, trying to magically keep existing back ends working and support the new feature, a numeric swizzle turns into something far more complex than a normal swizzle.
For example, a normal swizzle might turn into the subtree:
(v++).yx
->swizzle(increment(v), yx)
While a similar numeric swizzle requires:
Where, the desired obvious simplifications of that are all special cases.
This PR still leaves some of the as TBD, and begs the question of instead only doing it by widening the output interface.