Fix potential unsafe swizzle optimizations #298
Conversation
|
|
||
| float withExtraComponentsUnsafe(in vec3 a) { | ||
| vec3 v1 = vec3(a.x); | ||
| vec3 v2 = vec3(1.0, a.x); |
There was a problem hiding this comment.
An alternative could be to do the transformation only when there are at least two arguments in the constructor call?
If I understand you, that would apply to calls like this, which are AIUI unsafe to transform (this is equivalent to vec3(1.0, a.xx) not vec3(1.0, a.xy)).
There was a problem hiding this comment.
vec3(1.0, a) is equivalent to vec3(1.0, a.xy)
So I think that a.xy can become a whenever it's the last argument and there's more than one argument.
| vec4 v4 = vec4(v2, v3.xy); | ||
| return v2.x + v3.x + v4.x; | ||
| vec3 v5 = vec3(1, v3.rg); |
There was a problem hiding this comment.
So I think that a.xy can become a whenever it's the last argument and there's more than one argument.
Again, assuming I understand you correctly -- that is also properly handled by this PR, and tested via these cases here?
There was a problem hiding this comment.
alright, I think we can merge
Thank!
The optimization in laurentlb#253 is unsafe -- `vec3(a.x)` and `vec3(x)` are not equivalent. For a single-element swizzles, they are only sure to be safe if they are the Nth argument to a vecN constructor. This change counts the number of arguments, and only allows a single-element swizzle to be optimized in that case. This does miss some edge cases -- for example, I think the swizzle in code like `vec3(foo.xy, bar.x)` is safe to remove (`vec3(foo.xy, bar)`) but that doesn't seem to actually happen in any of the examples in this repo, and it's super tricky to get right in practise (if `foo.xy` is some variable instead of a swizzle right there), etc etc. So there is potentially still something on the table, but at least for the cases available this is good enough for now. Fixes laurentlb#297
The optimization in #253 is unsafe -- `vec3(a.x)` and `vec3(x)` are not equivalent. For a single-element swizzles, they are only sure to be safe if they are the Nth argument to a vecN constructor. This change counts the number of arguments, and only allows a single-element swizzle to be optimized in that case. This does miss some edge cases -- for example, I think the swizzle in code like `vec3(foo.xy, bar.x)` is safe to remove (`vec3(foo.xy, bar)`) but that doesn't seem to actually happen in any of the examples in this repo, and it's super tricky to get right in practise (if `foo.xy` is some variable instead of a swizzle right there), etc etc. So there is potentially still something on the table, but at least for the cases available this is good enough for now. Fixes #297
The optimization in #253 is unsafe --
vec3(a.x)andvec3(x)are not equivalent. For a single-element swizzles, they are only sure to be safe if they are the Nth argument to a vecN constructor. This change counts the number of arguments, and only allows a single-element swizzle to be optimized in that case.This does miss some edge cases -- for example, I think the swizzle in code like
vec3(foo.xy, bar.x)is safe to remove (vec3(foo.xy, bar)) but that doesn't seem to actually happen in any of the examples in this repo, and it's super tricky to get right in practise (iffoo.xyis some variable instead of a swizzle right there), etc etc. So there is potentially still something on the table, but at least for the cases available this is good enough for now.Fixes #297