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

Batched closures #1500

Merged
merged 3 commits into from
May 11, 2022
Merged

Batched closures #1500

merged 3 commits into from
May 11, 2022

Conversation

sfriedmapixar
Copy link
Contributor

Description

Tests

Checklist:

  • [ X] I have read the contribution guidelines.
  • [ X] I have previously submitted a Contributor License Agreement.
  • [ X] I have updated the documentation, if applicable.
  • [ X] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • [ X] My code follows the prevailing code style of this project.

AlexMWells
AlexMWells reviewed May 4, 2022
View reviewed changes
AlexMWells
AlexMWells reviewed May 4, 2022
View reviewed changes
src/liboslexec/batched_llvm_gen.cpp
// zero out the closure parameter memory.
if (clentry->prepare) {
// Call clentry->prepare(renderservices *, int id, void *mem)
llvm::Value *funct_ptr = rop.ll.constant_ptr((void *)clentry->prepare, rop.llvm_type_prepare_closure_func());
Copy link
Contributor

Choose a reason for hiding this comment

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

Was(is) there a unit test that exercised the prepare function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK There is not -- there isn't anything that appears to exercise it in the single-point testsuite either. I did a best-effort at making it match the single point implementation. Maybe Larry or someone else can comment on if this is a critical feature or not?

AlexMWells
AlexMWells reviewed May 4, 2022
View reviewed changes
AlexMWells
AlexMWells approved these changes May 4, 2022
View reviewed changes
Copy link
Contributor

@AlexMWells AlexMWells 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 approved these changes May 11, 2022
View reviewed changes
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM

@sfriedmapixar @lgritz
Add support for closures to the batched interface.
69cddb3 
The renderer interface for closures, including registering
and processing them remains unchanged.

The Ci variable in BatchedShaderGlobals has moved from
the uniform portion to the varying portion, which may
require changes to any renderer code that was utilizing
the BatchedShaderGlobals.

To utilize closures with the batched interface, you register
the closures per usual, then once shading has finished,
the Ci in BatchedShaderGlobals will contain an Block of pointers
to the roots of a closure trees that can be processed
independently just like in the normal single-point interface.

This initial implementation is largely functional, however,
there may be more optimization gains available in the code
that copies the run-time SIMD values and scatters them
to the separate closure-data locations.

One remaining un-implemented feature is passing
_closures_ via get/setmessage() when using the batched
shading interface, and the locations where that would
need to be implemented are marked in the source.

Signed-off-by: Stephen Friedman <[email protected]>
@sfriedmapixar @lgritz
Address batched closures review comments:
d29eb21 
* Remove unneded Block specializations for pointers.
* Rename null variable to null_value.
* remove a 0 default param to rop.llvm_void_ptr in batched_llvm_gen.cpp:llvm_gen_mul
* Optimizations to masking and loading in wide_opclosure.cpp
* Fix LLVM11+ builds by using llvm_vecor_type instead of llvm::VectorType::get

Signed-off-by: Stephen Friedman <[email protected]>
@sfriedmapixar @lgritz
Remove unneeded oslclosure.h in wide.h now that we aren't explicitly …
e46e75c 
…instantiating the templates.

Signed-off-by: Stephen Friedman <[email protected]>
@lgritz lgritz force-pushed the batched_closures branch from aa639b4 to e46e75c Compare May 11, 2022 06:49
@lgritz lgritz merged commit d8a9993 into AcademySoftwareFoundation:main May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgritz lgritz lgritz approved these changes

@AlexMWells AlexMWells AlexMWells approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfriedmapixar @lgritz @AlexMWells