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.

Sign up for GitHub

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

Jump to bottom

Add API for building attribute getter free functions. #1704

Merged
merged 40 commits into from
Sep 1, 2023
Merged

Add API for building attribute getter free functions. #1704

Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
d318962
Add API for building attribute getter free functions.
curtisblack Jun 23, 2023
1176fd4
Support all standard C++ types and ustring/ustringhash.
curtisblack Jul 21, 2023
c312d24
Fix different sign comparison.
curtisblack Jul 21, 2023
07bf09a
Fix memcpy error.
curtisblack Jul 21, 2023
6e352b6
Merge branch 'main' into getattribute_builder
curtisblack Jul 24, 2023
d3ec4fe
Disambiguate function call.
curtisblack Jul 24, 2023
637275d
Fix formatting.
curtisblack Jul 24, 2023
e4efe31
Fix formatting.
curtisblack Jul 24, 2023
4ad7c1d
Add helper functions and comments.
curtisblack Aug 17, 2023
9c929bd
Merge branch 'main' into getattribute_builder
curtisblack Aug 17, 2023
587fe5e
Use string params to ensure optimisation.
curtisblack Aug 18, 2023
46cc9e2
Merge branch 'main' into getattribute_builder
curtisblack Aug 18, 2023
9f05e03
Fix rebase conflict.
curtisblack Aug 18, 2023
02c4237
Convert return type to match virtual function signature.
curtisblack Aug 20, 2023
028aa6c
clang format
curtisblack Aug 20, 2023
e90f4e9
Fix incorrect indexing.
curtisblack Aug 21, 2023
0a1498d
Support userdata in attribute builder.
curtisblack Aug 21, 2023
0db9c26
Add missing derivatives argument.
curtisblack Aug 21, 2023
6c03cda
Fix typo.
curtisblack Aug 21, 2023
f914eeb
Use find_pv correctly.
curtisblack Aug 21, 2023
1577a8d
Merge branch 'main' into getattribute_builder
curtisblack Aug 21, 2023
630530f
Remove unused variable.
curtisblack Aug 21, 2023
a0dd0ed
clang format
curtisblack Aug 21, 2023
6f68b60
Fix some attribute types.
curtisblack Aug 21, 2023
651c7f9
Fix incorrect size.
curtisblack Aug 21, 2023
3edffb9
Cast to avoid ambiguity.
curtisblack Aug 25, 2023
697d1d3
Refactor for future reusability.
curtisblack Aug 25, 2023
217a238
Formatting.
curtisblack Aug 25, 2023
3006684
Fix template specialisations.
curtisblack Aug 25, 2023
3a0d66c
Formatting.
curtisblack Aug 25, 2023
74048bc
Remove templates.
curtisblack Aug 25, 2023
f8e449b
Fix asserts.
curtisblack Aug 25, 2023
c9cebdf
Fix type.
curtisblack Aug 25, 2023
8c5e57c
Merge branch 'main' into getattribute_builder
curtisblack Aug 28, 2023
e21087a
Use POD type with known size.
curtisblack Aug 28, 2023
7c62b2f
Update src/include/OSL/rendererservices.h
curtisblack Aug 31, 2023
c260dac
Make optional args const pointers.
curtisblack Sep 1, 2023
e72ddfb
clang format.
curtisblack Sep 1, 2023
2798354
clang format.
curtisblack Sep 1, 2023
c7971ef
constify ShaderGroup.
curtisblack Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,15 @@ configure_package_config_file ("${PROJECT_SOURCE_DIR}/src/cmake/Config.cmake.in"
install (FILES "${OSL_PROJECT_CONFIG}" "${OSL_VERSION_CONFIG}"
DESTINATION "${OSL_CONFIG_INSTALL_DIR}")

install (FILES src/cmake/llvm_macros.cmake DESTINATION cmake)

set (PERMISSION_FLAGS OWNER_EXECUTE OWNER_READ OWNER_WRITE
GROUP_EXECUTE GROUP_READ
WORLD_EXECUTE WORLD_READ)
install (FILES src/build-scripts/serialize-bc.py
DESTINATION build-scripts
PERMISSIONS ${PERMISSION_FLAGS})

# install targets files
install (EXPORT OSL_EXPORTED_TARGETS
DESTINATION ${OSL_CONFIG_INSTALL_DIR}
Expand Down
24 changes: 8 additions & 16 deletions src/cmake/llvm_macros.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
# SPDX-License-Identifier: BSD-3-Clause
# https://github.com/AcademySoftwareFoundation/OpenShadingLanguage

# TODO: Use CMAKE_CURRENT_FUNCTION_LIST_DIR in cmake-3.17
set(_THIS_MODULE_BASE_DIR "${CMAKE_CURRENT_LIST_DIR}")

function ( EMBED_LLVM_BITCODE_IN_CPP src_list suffix output_name list_to_append_cpp extra_clang_args)
function ( EMBED_LLVM_BITCODE_IN_CPP src_list suffix output_name list_to_append_cpp extra_clang_args include_dirs)

if (VERBOSE)
message (STATUS "EMBED_LLVM_BITCODE_IN_CPP src_list=${src_list}")
Expand Down Expand Up @@ -75,12 +77,8 @@ function ( EMBED_LLVM_BITCODE_IN_CPP src_list suffix output_name list_to_append_
# endif ()
#endif ()

list (TRANSFORM IMATH_INCLUDES PREPEND -I
OUTPUT_VARIABLE ALL_IMATH_INCLUDES)
list (TRANSFORM OPENEXR_INCLUDES PREPEND -I
OUTPUT_VARIABLE ALL_OPENEXR_INCLUDES)
list (TRANSFORM OpenImageIO_INCLUDES PREPEND -I
OUTPUT_VARIABLE ALL_OpenImageIO_INCLUDES)
list (TRANSFORM include_dirs PREPEND -I
OUTPUT_VARIABLE ALL_INCLUDE_DIRS)

if (${LLVM_VERSION} VERSION_GREATER_EQUAL 15.0)
# Until we fully support opaque pointers, we need to disable
Expand All @@ -93,13 +91,7 @@ function ( EMBED_LLVM_BITCODE_IN_CPP src_list suffix output_name list_to_append_
add_custom_command ( OUTPUT ${src_bc}
COMMAND ${LLVM_BC_GENERATOR}
${LLVM_COMPILE_FLAGS}
"-I${CMAKE_CURRENT_SOURCE_DIR}"
"-I${CMAKE_SOURCE_DIR}/src/include"
"-I${CMAKE_BINARY_DIR}/include"
${ALL_OpenImageIO_INCLUDES}
${ALL_IMATH_INCLUDES}
#"-isystem ${Boost_INCLUDE_DIRS}" #Does not pick up usr installed boost/thread/tss.hpp for oslexec_pvt.h
"-I${Boost_INCLUDE_DIRS}"
${ALL_INCLUDE_DIRS}
-DOSL_COMPILING_TO_BITCODE=1
-Wno-deprecated-register
# the following 2 warnings can be restored when all 3rd parties have fixed their export macros
Expand Down Expand Up @@ -131,9 +123,9 @@ function ( EMBED_LLVM_BITCODE_IN_CPP src_list suffix output_name list_to_append_
# Serialize the linked bitcode into a CPP file
set ( src_bc_cpp "${CMAKE_CURRENT_BINARY_DIR}/${output_name}.bc.cpp" )
add_custom_command ( OUTPUT ${src_bc_cpp}
COMMAND ${Python_EXECUTABLE} "${CMAKE_SOURCE_DIR}/src/build-scripts/serialize-bc.py"
COMMAND ${Python_EXECUTABLE} "${_THIS_MODULE_BASE_DIR}/../build-scripts/serialize-bc.py"
${linked_src_bc} ${src_bc_cpp} ${output_name}
DEPENDS "${CMAKE_SOURCE_DIR}/src/build-scripts/serialize-bc.py" ${linked_src_bc}
DEPENDS "${_THIS_MODULE_BASE_DIR}/../build-scripts/serialize-bc.py" ${linked_src_bc}
${exec_headers} ${PROJECT_PUBLIC_HEADERS}
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" )

Expand Down
10 changes: 8 additions & 2 deletions src/include/OSL/llvm_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,11 +665,17 @@ class OSLEXECPUBLIC LLVM_Util {
/// Return an llvm::Constant holding the given floating point constant.
llvm::Constant* constant(float f);

/// Return an llvm::Constant holding the given floating point constant.
llvm::Constant* constant64(double f);
AlexMWells marked this conversation as resolved.
Show resolved Hide resolved

/// Return an llvm::Constant holding the given integer constant.
llvm::Constant* constant(int i);
llvm::Constant* constant(int32_t i);
llvm::Constant* constant(uint32_t i);

/// Return an llvm::Constant holding the given integer constant.
llvm::Constant* constant8(int i);
llvm::Constant* constant8(int8_t i);
llvm::Constant* constant8(uint8_t i);
llvm::Constant* constant16(int16_t i);
llvm::Constant* constant16(uint16_t i);
llvm::Constant* constant64(uint64_t i);
llvm::Constant* constant128(uint64_t i);
Expand Down
14 changes: 14 additions & 0 deletions src/include/OSL/oslconfig.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,20 @@ ustringrep_from(string_view s)



using TypeDesc_pod = int64_t;

static_assert(sizeof(TypeDesc_pod) == sizeof(TypeDesc),
"TypeDesc size differs from its POD counterpart");

/// Convenience function to convert to a TypeDesc.
inline TypeDesc
TypeDesc_from(TypeDesc_pod type)
{
return OSL::bitcast<OSL::TypeDesc>(type);
}



// N.B. SymArena is not really "configuration", but we cram it here for
// lack of a better home.

Expand Down
12 changes: 12 additions & 0 deletions src/include/OSL/oslexec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,18 @@ class OSLEXECPUBLIC ShadingSystem {
void add_symlocs(cspan<SymLocationDesc> symlocs);
void add_symlocs(ShaderGroup* group, cspan<SymLocationDesc> symlocs);

// Find the SymLocationDesc for this named param, returning its pointer
// or nullptr if that name is not found.
const SymLocationDesc* find_symloc(ustring name) const;
const SymLocationDesc* find_symloc(ShaderGroup* group, ustring name) const;

// Find the SymLocationDesc for this named param but only if it matches
// the arena type, returning its pointer or nullptr if that name is not
// found.
const SymLocationDesc* find_symloc(ustring name, SymArena arena) const;
const SymLocationDesc* find_symloc(ShaderGroup* group, ustring name,
SymArena arena) const;

Copy link
Contributor Author

@curtisblack curtisblack Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

/// Ensure that the group has been optimized and optionally JITed. The ctx pointer
/// supplies a ShadingContext to use.
void optimize_group(ShaderGroup* group, ShadingContext* ctx,
Expand Down
60 changes: 59 additions & 1 deletion src/include/OSL/rendererservices.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <OSL/encodedtypes.h>
#include <OSL/oslconfig.h>
#include <OSL/variant.h>


OSL_NAMESPACE_ENTER
Expand All @@ -15,6 +16,7 @@ class RendererServices;
template<int WidthT> class BatchedRendererServices;
class ShadingContext;
struct ShaderGlobals;
class ShaderGroup;

// Tags for polymorphic dispatch
template<int SimdWidthT> class WidthOf {
Expand All @@ -30,6 +32,20 @@ typedef const void* TransformationPtr;
typedef void (*PrepareClosureFunc)(RendererServices*, int id, void* data);
typedef void (*SetupClosureFunc)(RendererServices*, int id, void* data);

enum class AttributeSpecBuiltinArg {
ShaderGlobalsPointer, // void* (TODO: ideally ShaderGlobals*)
ShadeIndex, // int
Derivatives, // bool
AlexMWells marked this conversation as resolved.
Show resolved Hide resolved
Type, // TypeDesc_pod
ArrayIndex, // int, Always zero for non-indexed array lookups.
IsArrayLookup, // bool
ObjectName, // const char* (TODO: change to ustringhash)
AttributeName, // const char* (TODO: change to ustringhash)
};

using AttributeSpecArg = ArgVariant<AttributeSpecBuiltinArg>;
using AttributeGetterSpec = FunctionSpec<AttributeSpecArg>;

// Turn off warnings about unused params for this file, since we have lots
// of declarations with stub function bodies.
OSL_PRAGMA_WARNING_PUSH
Expand All @@ -51,7 +67,8 @@ class OSLEXECPUBLIC RendererServices {

/// Given the name of a 'feature', return whether this RendererServices
/// supports it. Feature names include:
/// <none>
/// "OptiX"
/// "build_attribute_getter"
///
/// This allows some customization of JIT generated code based on the
/// facilities and features of a particular renderer. It also allows
Expand Down Expand Up @@ -189,6 +206,47 @@ class OSLEXECPUBLIC RendererServices {
const EncodedType* arg_types, uint32_t arg_values_size,
uint8_t* arg_values);

/// Builds a free function to provide a value for a given attribute.
/// This occurs at shader compile time, not at execution time.
///
/// @param group
/// The shader group currently requesting the attribute.
///
/// @param object_lookup
/// True if an object name was specified, even if the value is not
/// known at compile time.
///
/// @param object_name
/// The object name, or nullptr if the value is not specified or it
/// is not known at compile time.
///
/// @param attribute_name
/// The attribute name, or nullptr if the value is not known at
/// compile time.
///
/// @param array_lookup
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

/// True if the attribute lookup provides an index.
///
/// @param array_index
/// The array index, or nullptr if the value is not specified or it
/// is not known at compile time.
///
/// @param type
/// The type of the value being requested.
///
/// @param derivatives
/// True if derivatives are also being requested.
///
/// @param spec
/// The built attribute getter. An empty function name is interpreted
/// as a missing attribute.
///
virtual void build_attribute_getter(ShaderGroup& group, bool object_lookup,
ustring* object_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

@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);

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

ustring* attribute_name,
bool array_lookup, int* array_index,
TypeDesc type, bool derivatives,
AttributeGetterSpec& spec);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, let's make sense.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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,

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


/// Get the named attribute from the renderer and if found then
/// write it into 'val'. Otherwise, return false. If no object is
Expand Down
Loading
Loading