AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers
BSD 3-Clause "New" or "Revised" License
2.05k stars 347 forks source link

Add API for building interpolated getter free functions. #1765

Closed Uedaki closed 4 months ago

Uedaki commented 5 months ago

Description

Adds a new RendererServices API build_interpolated_getter which allows for building custom free functions that provide values for interpolated datas.

The new API is involved at material compilation time and allow the developers to provide specialized functions leveraging the information known at compile time. It will allow for many optimization opportunities for the compiler by replacing runtime branch with direct memory reads.

This PR is a followup to the API build_attribute_getter proposed in this PR: https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/pull/1704

Tests

TestShade provides an example implementation showing how this compile time information can be used to select an appropriate function to use as the attribute provider, and how to configure the function signature. Since the feature is enable by default when rs_bitcode is used, the existing testsuite ensure the feature is generating the same result as the previous API.

Checklist:

linux-foundation-easycla[bot] commented 5 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

AlexMWells commented 5 months ago

@Uedaki , overall this looks great! There are just some encapsulation/device compatibility issues in the testshade rs_getinterpolated* functions to clear up and a few more comments.

AlexMWells commented 5 months ago

@lgritz , can you comment on the CI /VFX2023 abi check failing? Is that because a new virtual function was added to RendererServices? Do we need to bump a version number or something to indicate the ABI might be changing?

AlexMWells commented 5 months ago

From the CI build artifacts:

rendererservices.h
namespace OSL_v1_14_0
[−] class RendererServices  1 
  | Change | Effect -- | -- | -- Virtual method build_interpolated_getter ( ShaderGroup const&, OpenImageIO_v2_4::ustring const&, struct OpenImageIO_v2_4::TypeDesc, bool, InterpolatedGetterSpec& ) has been added to this class. | The layout of v-table has been changed. Call of any virtual method at higher position in this class or its subclasses may result in crash or incorrect behavior of applications. [+] show v-table (old and new)
[+] affected symbols: 21 (3.7%)

So perhaps the new virtual method needs to be moved after all the other virtual methods, try that. Then on ABI breaking version changes, we could rearrange. Add comment why it's at the end.

lgritz commented 5 months ago

So perhaps the new virtual method needs to be moved after all the other virtual methods, try that. Then on ABI breaking version changes, we could rearrange. Add comment why it's at the end.

I'd rather keep the virtual functions in a good logical order when reading the header. Appending to the end is a bit of a suspect way to avoiding ABI break, I don't think it's guaranteed.

We're allowed to break ABI compatibility in main. We just need to adjust the version numbers to reflect it. Which we can do in a subsequent commit (we already need a subsequent commit in order to update the reference commit for ABI compatibility, so this is not a problem).

Unless you need this backported to 1.13, in which case we should do this (even if not guaranteed) to try to preserve ABI compatibility in practice as best we can.

AlexMWells commented 4 months ago

We're allowed to break ABI compatibility in main. We just need to adjust the version numbers to reflect it. Which we can do in a subsequent commit (we already need a subsequent commit in order to update the reference commit for ABI compatibility, so this is not a problem).

@lgritz (asking as I think I might be missing something here) Why wouldn't we just require this PR update the version number, as it will need to to pass CI before we merge right?

lgritz commented 4 months ago

@lgritz (asking as I think I might be missing something here) Why wouldn't we just require this PR update the version number, as it will need to to pass CI before we merge right?

Bumping the version is what we do when we have to break the ABI, yes!

But that alone won't make it pass the automated test that compares the ABI to a designated prior commit (named by its hash) that is the "reference" ABI we are trying to match. So the sequence we follow is:

  1. Merge the fix that breaks the ABI. It is at this point expected to fail the ABI CI check, and that's ok because we know why it's failing and are going to immediately fix it.
  2. Change the ABI check to make the "reference ABI" name the commit from step 1, and merge that as a separate fix. At this point, the automated ABI check will pass again.

You can't combine these into just one step, because the actual merge of PR (1) will result in a different commit hash than when the PR was submitted. There's no way to know what the new ABI reference commit's hash will be post-merge, so you simply cannot include it in step 1.

If you can think of a better solution to this, I'm all ears. I haven't been able to think of an easy way to say "make THIS commit, whatever its hash may end up being, be the new ABI reference going forward."