OpenShadingLanguage icon indicating copy to clipboard operation
OpenShadingLanguage copied to clipboard

Draft: Remove more unnecessary conditional run layer calls

Open chellmuth opened this issue 1 year ago • 6 comments

This PR is a work-in-progress that I've hit a (hopefully) minor stumbling block on.

Overview

@tgrant-nv was looking at some of our shader ptx awhile ago and pointed out that a surprisingly high amount of the total instructions were just conditional layer call checks (due to all the stores and loads surrounding calls). The concern was that this was slowing down OptiX module compilation.

OSL has some basic checks in place to remove unnecessary conditional layer calls, but this patch takes things further. The high level strategy is:

  • Mark all our conditional layer calls during llvm generation
  • Run a custom llvm optimization pass that associates calls with their basic blocks
  • For each call, walk up llvm's dominator tree to find basic blocks guaranteed to have already run
  • If a guaranteed basic block already contains the same call as the current one, remove the current one

It doesn't help the compilation times for every shader, but we've seen speedups as high as 5x on some of our most expensive shaders.

Work in progress

The problem I've run into is that the optimization pass introduces quite a bit of overhead to the ptx generation. Most assets take 10-20% longer to generate ptx with this patch. For us, it's probably still a good trade to turn on for GPU (module compilation is more expensive than ptx generation), but I was hoping some more eyes could help improve things. Especially since I've never written a LLVM pass before, maybe I'm doing things particularly inefficiently.

Any feedback or ideas would be appreciated!

Tests

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.

chellmuth avatar Aug 18 '23 18:08 chellmuth

CLA Missing ID CLA Not Signed

  • :x: The email address for the commit (af31c7b9ef73b20c519df61c2eaaf00851493d57) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

This sounds very promising. A 10-20% increase in JIT time isn't so bad, if it increases the shader performance or even reduces the PTX compile time. We'll take a closer look and see what recommendations we can make.

tgrant-nv avatar Aug 21 '23 15:08 tgrant-nv

This sounds very promising. A 10-20% increase in JIT time isn't so bad, if it increases the shader performance or even reduces the PTX compile time. We'll take a closer look and see what recommendations we can make.

Thanks! Looking forward to your findings.

chellmuth avatar Aug 22 '23 05:08 chellmuth

Is this ready to be a non-draft? Looks like it might need a rebase as well.

lgritz avatar Sep 14 '23 18:09 lgritz

Is this ready to be a non-draft?

It's close. It turned out the speed hit we've been seeing is because removing so many run-layer checks made the remaining ones more palatable for inlining. So LLVM's inlining pass times are growing quite a bit.

Now that it's been merged, I want to test this patch internally on #1680, since that also has inlining ramifications and I want to make sure everything interacts okay.

chellmuth avatar Sep 14 '23 20:09 chellmuth

@chellmuth , as an experiment could you add a no-inline attribute to the layer functions to prevent them from being inlined? Not sure you really want to do that, but could be an interesting data point.

AlexMWells avatar Sep 18 '23 16:09 AlexMWells