Add API for building attribute getter free functions.

[curtisblack]

Description

Adds a new RendererServices API build_attribute_getter which allows for building custom free functions that provide values for attributes, effectively replacing both get_attribute and get_array_attribute.

This new API is invoked at material compilation time to allow render developers to provide specialized functions capable of taking advantage of known compile time information. This will allow for many optimization opportunities, such as replacing run-time map lookups with direct memory reads from known locations, remove brancing required for checking if derivatives are needed, branching on type-conversions if required, etc.

This PR currently implements this approach for attributes, a future PR could do the same for user-data.

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.

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.

[curtisblack]

We might need to add one more argument to this function to allow render developers to pass through their own information.

e.g. consider a typical wrapper class:

class Material
{
  ShaderGroup group;

  void Compile()
  {
    shadingSystem->optimize_group(&group);
  }
};

class Renderer : RendererServices
{
  bool build_attribute_getter(ShaderGroup& group, ...) override
  {
    // how do we get access to the current Material here?
  }
};

Some ideas:

• Add a user supplied pointer member to ShaderGroup: void Compile() { group.user_ptr = this; ... }
• Pass a user supplied pointer through compilation: optimize_group(this, &group);
• Something else?

[sfriedmapixar]

I think it would be good to have a pointer passed through to jit-compilation. We use the same shader group for multiple compiles depending on how we're going to execute it, so we'd want it to be different-per-compile call. Calls that compile as a convenient side-effect would need to have access to to it as well, so it should be piped through there.

It would also be useful in the future for things like satisfying get-texture-info calls and the like at compile time.

[AlexMWells]

Well, during shader execution, ShaderGlobals::renderstate exists so derived RendererServices can stow what ever it wants away. We are suggesting a move considering this ShaderGlobals as ExecContext, meaning this is the context during shader execution. So it figures we need a similar context during shader compilation/JIT/OPT. Would CompileContext work? Nice part is CompileContext is host only so free to be as complex as it wants. Now ExecContext contains OSL lib side stuff and Renderer stuff as well, not sure if CompileContext needs to be as complex or can just be an Opaque Pointer to whatever renderer passes into optimize/jit_group. I guess that is simplest to start with. If so, renderer is the supplier of it, maybe typedef void * OpaqueRendCompileContextPtr,

void optimize_group(ShaderGroup* group, OpaqueRendCompileContextPtr, ShadingContext* ctx,
                        bool do_jit = true)

Although looking at that, we do have the existing host side ShadingContext which is already sort of a per thread thing. Should the ShadingContext move towards being a CompileContext with a setter/getter for RendCompileContext. And simply pass the ShadingContext through to the bool build_attribute_getter(ShadingContext &sc, ShaderGroup& group, ...) override

[curtisblack]

These additions are not strictly necessary for the rest of this PR to work, but it did seem to be useful to be able to query current symbol locations for a shader group inside the build_attribute_getter function.

[sfriedmapixar]

Overall looks useful as-is, but would like to see the suggested improvements.

[sfriedmapixar]

I think it would be good to have a pointer passed through to jit-compilation. We use the same shader group for multiple compiles depending on how we're going to execute it, so we'd want it to be different-per-compile call. Calls that compile as a convenient side-effect would need to have access to to it as well, so it should be piped through there.

It would also be useful in the future for things like satisfying get-texture-info calls and the like at compile time.

[sfriedmapixar]

I think if the attribute name isn't constant, since a constant getattribute("", result); can be pretty trivially turned into a "just fail" whereas if it was not constant, we want to do the run-time lookup. I bet it's pretty easy to get into a place where a "default" shader ends up doing a getattribute("",result) that can basically be used to skip a bunch of work, but if there's an attribute set in the shading node to cause that to turn into getattribute("mycoolval", result), there could be a lot of other stuff that would get run. If a non-constant attribute name causes us to call build_attribute_getter with "", we can no longer optimize the cases where it's a constant "".

[sfriedmapixar]

The other thing we could do is just pass in the Symbol& and let the renderer inspect them to find that info out.

[AlexMWells]

The other thing we could do is just pass in the Symbol& and let the renderer inspect them to find that info out.

If the name isn't constant, what could the renderer figure out from the Symbol&? (not rhetorical)

[sfriedmapixar]

Sorry, my first comment was a bit nonsensical. It could discriminate between a non-const name and a "".

[curtisblack]

After a bit of discussion with Larry and Alex, our proposal here would be to handle the compile time constant empty attribute name string internally, it would always produce a fail. In such case the build_attribute_getter function would not be called. The reason being is that a constant empty attribute name is not very sensical. With this in place, you could then be sure that on the renderer side if you receive an empty attribute name, the only possibility is because it's not known at compile time.

I'd prefer to avoid passing LLVM symbols out into user facing code if possible.

[AlexMWells]

Great Work!
LGTM

[lgritz]

Sorry, I've got a few minor last-minute concerns about this addition to RendererServices, just want to be sure we're nailing down the parameters before it becomes part of the public API.

virtual void build_attribute_getter(ShaderGroup& group, bool object_lookup,
                                    ustring* object_name,
                                    ustring* attribute_name,
                                    bool array_lookup, int* array_index,
                                    TypeDesc type, bool derivatives,
                                    AttributeGetterSpec& spec);

What's the difference between object_lookup being false and object_name being null? Is the object_lookup parameter needed? Same question with array_lookup and array_index.

Should any or all of those pointer or reference parameters be const?

[lgritz]

Oh, I think I see now? The bool can be true and the pointer null in situations where the lookup will provide a value, but it isn't known at compile time?

If that's the case, then my only concern here is whether those ptrs/refs should be const.

[curtisblack]

Yeah that's right, there are a few combinations, for example:

object_lookup=false, object_name=nullptr

int result;
getattribute("attr", result);

object_lookup=true, object_name=nullptr

int result;
string obj = GetSomeDynamicString();
getattribute(obj, "attr", result);

object_lookup=true, *object_name="obj"

int result;
getattribute("obj", "attr", result);

The same concept applies to array_lookup/array_index and attribute_name. The convention we are setting is that nullptr is for values not known at compile time.

Agreed they should probably be const though, I'll update that.

I suppose it might be more self documenting if we switch to std::optional with C++17.

[lgritz]

OK, let's make sense.

Please const-ify what makes sense, then I'm ready to merge.

[AlexMWells]

I suppose it might be more self documenting if we switch to std::optional with C++17.

Agreed, but the minimum c++ is 14 right now.
Short of introducing our own OSL::optional...

Maybe we should just bake it into the parameter name, to increase readability:

                                    ustring* optional_object_name,
                                    ustring* optional_attribute_name,
                                    ..., int* optional_array_index,

or embed comments to that effect

                                    ustring* /*optional*/ object_name,
                                    ustring* /*optional*/ attribute_name,
                                    ..., int* /*optional*/ array_index,

[lgritz]

I'm not worried about this. The documentation for the function explains it just fine. I do think it should be const ustring* to be extra clear that the function is not going to change what it points to. But the rest does not concern me at all.

[lgritz]

LGTM in general, nice work.

I'm satisfied with Alex's thorough review and don't have anything additional to add, except for two minor comments/suggestions strictly related to making extra sure that the new public method is absolutely as clear as possible in its declaration and comments. All the behind-the-scenes stuff can be easily fixed at any point later, but public APIs tend to linger.

[AlexMWells]

To match the AttributeSpecBuiltinArg::IsArrayLookup, can we change
@param array_lookup
to
@param is_array_lookup

Maybe we should do the same for
bool object_lookup
to
bool is_object_lookup

[curtisblack]

Done!

[AlexMWells]

@lgritz how "const" did you want to make these parameters? Should the copy by value be const as well, ei:
virtual void build_attribute_getter(ShaderGroup& group, const bool is_object_lookup,
const ustring* object_name,
const ustring* attribute_name,
const bool is_array_lookup, const int* array_index,
const TypeDesc type, const bool derivatives,
AttributeGetterSpec& spec);

[lgritz]

I just meant the pointer and reference parameters. Making them const:
(a) makes it clear to the reader that these are input parameters and the things they pass pointers/refs to will not be modified.
(b) allows callers to pass things to this function that they themselves received as const and would have to make awkward casts to ram it through the compiler otherwise.

I know there are some people who even declare value parameters as const, but that's not something I've ever made a habit of doing.

[lgritz]

Last one: group? const or not? Do we expect it may be modified by a possible reasonable implementation of build_attribute_getter?

[curtisblack]

Good question, by this point in the code we are jitting so I'd expect all modifications to group to have already been completed. So in that case I'd vote const.

Most of the ShadingSystem functions to access ShaderGroup info already accept const ShaderGroups. There look to be two exceptions: ShadingSystem:getattribute(ShaderGroup*, string_view, TypeDesc) and ShadingSystem::find_symloc(ShaderGroup*, ustring) which would seem reasonable to access within these functions which might also need to get the const treatment if possible.

[lgritz]

It seems like those could be const? But this feels like a rathole.

Let's make the group& parameter to build_attribute_getter be const as well. But don't change SS::getattribute or find_symloc in this PR. We'll look at that later, and change down the road if we need to.

[lgritz]

LGTM, merging!