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

Make recent run-layer optimizations optional, and fix init ops-related false positive #1672

Conversation

chellmuth
Copy link
Collaborator

Description

My initial implementation of the run layer check optimizations didn’t account for the fact that init ops for symbols have their llvm code generated before we build basic blocks from the OSL IR.

This caused the code to be wrong in two ways: 1) the set of known run layer calls was being reset between instances too late in the pipeline, so a useparam inside an init op would actually be checking the previously compiled layer’s set, and 2) even when reset earlier, it assumed valid basic block ids existed for the init ops when they did not.

So, if a layer had a useparam inside one of its init ops that generally matched the same code location as a useparam in the previously compiled layer, the run layer check could be incorrectly omitted.

I’ve updated the code so that the default optimization behavior is reverted to the way it was before #1665, and added a new option opt_useparam that can be turned on to enable a fixed, restricted version of the optimization that only applies to code in the main section.

Tests

I'm working on a test that properly triggers the issue.

Checklist:

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

… false positive

Signed-off-by: Chris Hellmuth <chellmuth@gmail.com>
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, good catch, I also missed noticing that the init_ops might interact poorly with your prior change

@lgritz lgritz merged commit 37f4bbc into AcademySoftwareFoundation:main Apr 20, 2023
20 checks passed
@AlexMWells
Copy link
Contributor

AlexMWells commented Apr 20, 2023

Is the new opt_useparam just to allow a way to turn this off in case there is a bug or to measure its individual improvement? Seems like, if its correct (the big question) you would always want it.

@@ -1024,6 +1024,7 @@ ShadingSystemImpl::ShadingSystemImpl(RendererServices* renderer,
, m_opt_middleman(true)
, m_opt_texture_handle(true)
, m_opt_seed_bblock_aliases(true)
, m_opt_useparam(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will then be enabled by default once confidence in its correctness is established?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Chris is reverting most of the original change, and also putting this option in that defaults off but will allow it to be turned on selectively (after it's re-introduced, hopefully fixed) while we're gaining confidence, then will eventually be switched to default on (with ability to turn it off if anything fishy happens).

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.

None yet

3 participants