-
Notifications
You must be signed in to change notification settings - Fork 346
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
Enable userdata derivatives for interpolated params on GPU #1657
Enable userdata derivatives for interpolated params on GPU #1657
Conversation
src/liboslexec/llvm_instance.cpp
Outdated
* 3 // make room for derivs by default | ||
: type.numelements(); | ||
type.arraylen = n; | ||
type.arraylen = type.numelements() * 3; // make room for derivs by default |
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.
Now that I'm looking at this, I think that we should only multiply by 3 if type.basetype == TypeDesc::FLOAT
. Because anything based on int or string can't possibly have derivatives, so perhaps you can reclaim a little space that way (if indeed anything is ever declared userdata with those types, but it is possible).
Also, I'm seeing a CI complaint about clang-format on this line.
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.
Sounds good, I pushed an update addressing both points.
Signed-off-by: Chris Hellmuth <chellmuth@gmail.com>
e789908
to
466b5e7
Compare
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.
LGTM
When generating calls to osl_bind_interpolated_param in llvm_instance.cpp, the userdata_has_derivs field is set to true if the parameter's derivatives are needed, but the code does not take into account that on gpu there is no space allocated for these derivatives. This leads to an incorrect derivatives pointer getting passed into the renderer, which the renderer will use to write into unrelated groupdata fields. For our renders, getting the correct image was worth the 1-2% inflation of groupdata struct size that we observed by simply re-enabling gpu userdata derivatives. If desired, adding an option to turn them on/off is straightforward, but I didn't want to preemptively bloat the code if it's not necessary. Signed-off-by: Chris Hellmuth <chellmuth@gmail.com>
…1685) Following #1657, it turns out we forgot to zero initialize the userdata derivatives for the optix path. This leads to some rendering glitches caused by the memory garbage left in those derivatives. Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Co-authored-by: Pascal Lecocq <plecocq@imageworks.com>
Description
Enable userdata derivatives for interpolated parameters on the GPU.
When generating calls to
osl_bind_interpolated_param
in llvm_instance.cpp, theuserdata_has_derivs
field is set to true if the parameter's derivatives are needed, but the code does not take into account that on gpu there is no space allocated for these derivatives.This leads to an incorrect derivatives pointer getting passed into the renderer, which the renderer will use to write into unrelated groupdata fields.
For our renders, getting the correct image was worth the 1-2% inflation of groupdata struct size that we observed by simply re-enabling gpu userdata derivatives. If desired, adding an option to turn them on/off is straightforward, but I didn't want to preemptively bloat the code if it's not necessary.
Tests
Checklist: