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.

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

Fix mutated idents in functions which are inlined more than once #299

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

jwatzman
Copy link
Contributor

The function inlining directly drops the inlined function's body into the place it gets inlined. This is problematic for mutable Idents -- in particular, the renamer could do some very strange things as it tried to rename the same Ident to more than one thing. For example, the included test case was previously renamed to the following:

uniform vec3 v[42];
vec3 N()
{
  return v[4];
}
vec3 N(float v)
{
  return v[4]+v; // <-- Whoops, used "v" for both the global and the local!
}

Therefore all Idents (which aren't function parameters that get substituted away) need to get copied during inlining.

The function inlining directly drops the inlined function's body into
the place it gets inlined. This is problematic for mutable Idents -- in
particular, the renamer could do some very strange things as it tried to
rename the same Ident to more than one thing. For example, the included
test case was previously renamed to the following:

```
uniform vec3 v[42];
vec3 N()
{
  return v[4];
}
vec3 N(float v)
{
  return v[4]+v; // <-- Whoops, used "v" for both the global and the local!
}
```

Therefore all Idents (which aren't function parameters that get
substituted away) need to get copied during inlining.
@eldritchconundrum
Copy link
Collaborator

eldritchconundrum commented Apr 18, 2023

I tried to prevent this from happening by detecting it during function inlining candidate analysis, with the rule "Only inline a function if it never refers to a global function or variable by a name that is shadowed by a local variable in scope at the call site", but it looks like I didn't take into account the renaming pass.

@laurentlb
Copy link
Owner

The side-effect in the renaming pass is a bit annoying (I'd like to move the new name from Ident to VarDecl). Making a copy makes sense to me.

Thanks!

@laurentlb laurentlb merged commit b0fe7ed into laurentlb:master Apr 18, 2023
laurentlb pushed a commit that referenced this pull request Apr 30, 2023
The function inlining directly drops the inlined function's body into
the place it gets inlined. This is problematic for mutable Idents -- in
particular, the renamer could do some very strange things as it tried to
rename the same Ident to more than one thing. For example, the included
test case was previously renamed to the following:

```
uniform vec3 v[42];
vec3 N()
{
  return v[4];
}
vec3 N(float v)
{
  return v[4]+v; // <-- Whoops, used "v" for both the global and the local!
}
```

Therefore all Idents (which aren't function parameters that get
substituted away) need to get copied during inlining.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants