OpenShadingLanguage icon indicating copy to clipboard operation
OpenShadingLanguage copied to clipboard

Optimizations for LLVM upgrade

Open marsupial opened this issue 8 years ago • 7 comments

This is a bit large an involves other PRs submitted, but seems to bring performance back to where it was. Curious to see if that stays true on larger scenes. The last commit adds two optimization passes, no idea if it scales well.

ORC provided possibly a slight improvement but not that different (perhaps less memory use). Moving ORC to an on demand compiler was interesting but failed when treading was involved.

What I opted to do was split the precompiled byte code into its constituent parts and load them on request. This seems put performance back to to where it was. This method seems to work with LLVM 34 as well, but there aren't any noticeable gains.

Splitting it increases the lib size by ~1MB. Will require a C++11 compiler to compile the executable that does the splitting of byte code. Support the different LLVM versions via macros is getting a bit ugly.

marsupial avatar Feb 06 '17 18:02 marsupial

Update that last sentence to: Support the different LLVM versions via macros is getting a bit dangerous.

marsupial avatar Feb 06 '17 19:02 marsupial

It is hard to keep up with you!

OK, look, let's take this a step at a time.

  1. Let's apply the minimal fixes to the current master that unbreak anything we inadvertently broke in the last week, so that we are at a stage where we can compile against LLVM 3.4 (with C++03, old JIT), LLVM 3.5 (C++11, old JIT), or LLVM 3.9/4.0 (C++11, MCJIT) and it all works. At this stage, I am only minimally concerned with the performance of LLVM 3.9/4.0, as long as there is no performance regression when using 3.4/3.5.

  2. Branch RB-1.8 to form the basis of a stable release that we won't keep breaking.

  3. In the new master (after the branch), remove all support for LLVM 3.4 and C++03, that should simplify some of the logic and the nested ifdefs.

  4. Apply the additional patches that address MCJIT performance, including a switch to ORC JIT. I don't want to backport this to RB-1.8, that seems unnecessarily complicated.

  5. If the ORC JIT fix is a step forward that helps to close the gap between MCJIT and the JIT performance we had with old JIT, then we will (in master only) remove all the support for LLVM older than 3.9, further simplifying the logic.

  6. Additional cleanup or cosmetic fixes.

lgritz avatar Feb 06 '17 19:02 lgritz

Oh I thought it was clear but I wasn't hoping for this to be merged all at once or even particularly soon. It is pretty close to my branch and you seem to have a better testing framework for timing and was curious if these changes scale/holds up.

If/when you have the time I'm curious to see the timing info for three points in these commits:

  1. 8cb8905 : built with -DOSL_SPLIT_BITCODES=1 through CMake
  2. 093b07c : see if Orc actually provides a benefit (though I think MCJIT is being removed anyway).
  3. 6f646d6 : to see if removing the verifier pass and additional optimization scales well.

There is probably a bit review / consolidation that should occur before merging.

marsupial avatar Feb 06 '17 20:02 marsupial

BTW, I am very very excited about testing an ORC JIT patch!

lgritz avatar Feb 06 '17 22:02 lgritz

Surprisingly nothing changed much using Orc. Hopefully less memory. It's a bit easier interface for trying things out and I think MCJIT is being removed so may be a necessity.

Splitting up the bitcode seems to really have made a difference. Essentially it's mimicking what I would expect from on request compilation.

marsupial avatar Feb 06 '17 22:02 marsupial

I don't know what shaders you are testing on, but our shader networks are ENORMOUS. It's not hard for me to believe that something that looks like "no difference" to you could still be a big boost for us (or, unfortunately, vice versa).

But as long as ORC JIT is not slower, I think we should move to it as soon as possible, because (a) it does seem to be more flexible and a better interface, and (b) there is momentum in the LLVM community for some future version to remove MCJIT and be all ORC all the time (though at the earliest, LLVM 5, at least 6 months away). However, if we can turn the movement to ORC into also being a performance win for us, then so much the better!

lgritz avatar Feb 07 '17 00:02 lgritz

I can confirm that this failed on Opensuse 42.3. I got an LLVM error when everything supposedly is fine.

Here is a log from the MAKE command.

`:~/bin/osl> make -j 8 -- The CXX compiler identification is GNU 4.8.5 -- The C compiler identification is GNU 4.8.5 -- Check for working CXX compiler: /usr/bin/c++ -- Check for working CXX compiler: /usr/bin/c++ -- works -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Detecting CXX compile features -- Detecting CXX compile features - done -- Check for working C compiler: /usr/bin/cc -- Check for working C compiler: /usr/bin/cc -- works -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Detecting C compile features -- Detecting C compile features - done -- Building OSL 1.9.1 -- CMake version is 3.5.2 -- Project build dir = /home/mirdellt410/bin/osl/build/linux64 -- Project install dir = /home/mirdellt410/bin/osl/dist/linux64 -- platform = linux64 -- CMAKE_CXX_COMPILER is /usr/bin/c++ -- CMAKE_CXX_COMPILER_ID is GNU -- Building for C++11 -- Setting Namespace to: OSL_v1_9 -- Using OpenImageIO 1.7.0 CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:148 (message): Could NOT find LLVM (missing: LLVM_LIBRARIES) (found suitable version "3.8.0", minimum required is "3.5") Call Stack (most recent call first): /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:388 (_FPHSA_FAILURE_MESSAGE) src/cmake/modules/FindLLVM.cmake:108 (find_package_handle_standard_args) src/cmake/externalpackages.cmake:67 (find_package) CMakeLists.txt:122 (include)

-- Configuring incomplete, errors occurred! `

Mirppc avatar Oct 11 '17 19:10 Mirppc

This PR is ridiculously out of date and irrelevant. Putting it out of its misery.

lgritz avatar Dec 28 '23 04:12 lgritz