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

Bug fix: optimizer interaction with output param that also is userdata #1295

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Nov 7, 2020

There's a "pass-through parameter" idiom, where an output param gets
its initial value from an input, it gets modified by the shader, but
it's also an output.

If the modification is optimized away -- say, if it was originally
theparam *= scale and scale is 1 -- then it may be that you end up
with no ops in the shader that read from the parameter. If the
parameter was hooked up to userdata, then get_userdata will never be
called to retrieve the value (because it's usually lazy, only doing
the callback the first time it's needed), and so the output value of
parameter will be its parameter default value in the shader, NOT the
value of the interpolated variable, which was never retrieved and
copied into it.

This patch adjusts the logic somewhat so that if that output parameter
is connected downstream or is a renderer output, then we definitely
initialize it with a useparam op, even if it is otherwise apparently
not read from within the shader, and also that a shader consisting of
only that particular flavor of useparam is not marked as "does
nothing" (thus skipping any code generation at all).

Added a new test for this specific case.

Modified testshade to allow arbitrary command line-specificed userdata,
to make constructing this kind of test easier, by adding a --userdata
command which constructs a ParamValueList that will be used to look up
"userdata" from the shader.

Also necessitated update to reference output of debug_uninit test,
only because the extra useparam op caused some of the instruction
numbering to change.

There's a "pass-through parameter" idiom, where an output param gets
its initial value from an input, it gets modified by the shader, but
it's also an output.

If the modification is optimized away -- say, if it was originally
`theparam *= scale` and scale is 1 -- then it may be that you end up
with no ops in the shader that read from the parameter. If the
parameter was hooked up to userdata, then get_userdata will never be
called to retrieve the value (because it's usually lazy, only doing
the callback the first time it's needed), and so the output value of
parameter will be its parameter default value in the shader, NOT the
value of the interpolatd variable, which was never retrieved and
copied into it, and also that a shader consisting of *only* that
particular flavor of useparam is not marked as "does nothing" (thus
skipping any code generation at all).

This patch adjusts the logic somewhat so that if that output parameter
is connected downstream or is a renderer output, then we definitely
initialize it with a useparam op, even if it is otherwise apparently
not read from within the shader.

Added a new test for this specific case.

Also necessitated update to reference output of debug_uninit test,
only because the extra useparam op caused some of the instruction
numbering to change.

Signed-off-by: Larry Gritz <[email protected]>
To ease testing, add `--userdata` command line option to testshade,
which constructs a ParamValueList that will be used to look up
"userdata" from the shader.

Signed-off-by: Larry Gritz <[email protected]>
Copy link
Contributor

@aconty aconty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lgritz lgritz merged commit 82292a5 into AcademySoftwareFoundation:master Nov 10, 2020
@lgritz lgritz deleted the lg-userdata branch November 10, 2020 01:31
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Nov 10, 2020
AcademySoftwareFoundation#1295)

* Bug fix: optimizer interaction output param that also is userdata

There's a "pass-through parameter" idiom, where an output param gets
its initial value from an input, it gets modified by the shader, but
it's also an output.

If the modification is optimized away -- say, if it was originally
`theparam *= scale` and scale is 1 -- then it may be that you end up
with no ops in the shader that read from the parameter. If the
parameter was hooked up to userdata, then get_userdata will never be
called to retrieve the value (because it's usually lazy, only doing
the callback the first time it's needed), and so the output value of
parameter will be its parameter default value in the shader, NOT the
value of the interpolatd variable, which was never retrieved and
copied into it, and also that a shader consisting of *only* that
particular flavor of useparam is not marked as "does nothing" (thus
skipping any code generation at all).

This patch adjusts the logic somewhat so that if that output parameter
is connected downstream or is a renderer output, then we definitely
initialize it with a useparam op, even if it is otherwise apparently
not read from within the shader.

Added a new test for this specific case.

Also necessitated update to reference output of debug_uninit test,
only because the extra useparam op caused some of the instruction
numbering to change.

Signed-off-by: Larry Gritz <[email protected]>

* testshade: allow arbitrary command line-specificed userdata

To ease testing, add `--userdata` command line option to testshade,
which constructs a ParamValueList that will be used to look up
"userdata" from the shader.

Signed-off-by: Larry Gritz <[email protected]>
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.

2 participants