Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Incorrect removal of swizzle in vec3 #297

Closed
jwatzman opened this issue Apr 16, 2023 · 2 comments · Fixed by #298
Closed

Incorrect removal of swizzle in vec3 #297

jwatzman opened this issue Apr 16, 2023 · 2 comments · Fixed by #298

Comments

@jwatzman
Copy link
Contributor

Current master miscompiles the following:

vec3 foo(vec3 a) {
  return vec3(a.x);
}

The output with --format indented --no-remove-unused is:

vec3 v(vec3 v)
{
  return vec3(v);
}

which is not equivalent -- the .x swizzle has been completely dropped.
@laurentlb
Copy link
Owner

Thanks for the report. I guess the expected output is return v.xxx

@jwatzman
Copy link
Contributor Author

I think that would work, as would leaving it alone at v.x. I have a PR which does that half-written which I'll get up in about an hour if you want to go that direction.

jwatzman added a commit to jwatzman/Shader_Minifier that referenced this issue Apr 16, 2023
@jwatzman
Fix potential unsafe swizzle optimizations
f453241 
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
@jwatzman jwatzman mentioned this issue Apr 16, 2023
Merged
laurentlb pushed a commit to jwatzman/Shader_Minifier that referenced this issue Apr 17, 2023
@jwatzman @laurentlb
Fix potential unsafe swizzle optimizations
8d8e7b9 
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
laurentlb pushed a commit that referenced this issue Apr 17, 2023
@jwatzman
Fix potential unsafe swizzle optimizations (#298)
ae8445c 
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
laurentlb pushed a commit that referenced this issue Apr 30, 2023
@jwatzman @laurentlb
Fix potential unsafe swizzle optimizations (#298)
5005eaf 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix potential unsafe swizzle optimizations jwatzman/Shader_Minifier
2 participants
@laurentlb @jwatzman