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.

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

Remove direct access to ShaderGlobals members from llvm_ops #1414

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 28, 2021

This is a stepping stone to "shader globals placement".

Turns out that there were only two functions in llvm_ops.cpp that
directly needed to know the layout of the ShaderGlobals struct: the
implementations of osl_calculatenormal, and osl_raytype_bit.

For osl_calculatenormal, just pass the flipHandedness flag directly,
instead of the ShaderGlobals ptr, from which the field was extracted.

For osl_raytype_bit, I changed the function to only retrieve the bit
corresponding to a raytype name (and it's not in llvm_ops any more),
and the "bitwise and" code is just directly generated as bitcode.

I managed to complete the batched shading implementation of
calculatenormal! But for batched raytype_bit, I handled the case with
a constant name, but for the unknown name I got a little confused and
I'm hoping Alex can bail me out with some hand-holding or tell me
what needs to be done exactly.

Signed-off-by: Larry Gritz lg@larrygritz.com

This is a stepping stone to "shader globals placement".

Turns out that there were only two functions in llvm_ops.cpp that
directly needed to know the layout of the ShaderGlobals struct: the
implementations of osl_calculatenormal, and osl_raytype_bit.

For osl_calculatenormal, just pass the flipHandedness flag directly,
instead of the ShaderGlobals ptr, from which the field was extracted.

For osl_raytype_bit, I changed the function to only retrieve the bit
corresponding to a raytype name (and it's not in llvm_ops any more),
and the "bitwise and" code is just directly generated as bitcode.

I managed to complete the batched shading implementation of
calculatenormal! But for batched raytype_bit, I handled the case with
a constant name, but for the unknown name I got a little confused and
I'm hoping Alex can bail me out with some hand-holding or tell me
what needs to be done exactly.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@fpsunflower
Copy link
Contributor

LGTM for the scalar path.

rop.llvm_load_arg (P, true /*derivs*/, false /*op_is_uniform*/),
rop.llvm_global_symbol_ptr(Strings::flipHandedness),
Copy link
Contributor

Choose a reason for hiding this comment

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

As a matter of sanity checking, might consider pulling the global_symbol_ptr out and passing the optional is_uniform parameter and OSL_ASSERT(!is_uniform) as the function being called will interpret the pointer as varying.

{
llvm::Value* bit;
bit = rop.ll.constant(rop.shadingsys().raytype_bit(Name.get_string()));
llvm::Value* raytype = rop.ll.op_load_int(
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if less is more, meaning was op_load_int needed, in that LLVM_Util::ptr_to_cast or LLVM_Util::ptr_cast and LLVM_Util::op_load which could be used to provide the same functionality.

sg,
rop.ll.constant (rop.shadingsys().raytype_bit (name))};

llvm::Value *ret = rop.ll.call_function (rop.build_name("raytype_bit"), args);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove
OSL_BATCHOP int _OSL_OP(raytype_bit)(void* bsg, int bit)
from wide_shadingsys.cpp, and
DECL(__OSL_OP(raytype_bit), "iXi")
from builtindecl_wide_xmacro.h

rop.llvm_store_value (ret, Result);

} else {
FuncSpec func_spec("raytype_name");
// No way to know which name is being asked for
Copy link
Contributor

Choose a reason for hiding this comment

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

For the non-const case suggest following the same pattern you did for llvm_gen, go into wide_shading.cpp and rename the uniform and varying versions of raytype_name to raytype_bit.

For the uniform verson of raytype_name just return the bit by removing the bitwise AND return (bsg->uniform.raytype & bit), and in the batched_llvm_gen add a bitwise AND to the raytype from batchedShaderGlobals (very similar to llvm_gen version).

For the varying version, leave the foreach_unique loop, but again drop the bitwise AND int ray_is_named_type = ((bsg->uniform.raytype & bit) != 0) and just pass the bit variable into assign_all. But back in the batched_llvm_gen you will now need to allocate a varying temporary int to pass to varying version of raytype_bit. IE:

BatchedBackendLLVM::TempScope temp_scope(rop);

// We need wide place to hold the results of raytype_bit 
llvm::Value * loc_raytype_bit = rop.getOrAllocateTemp (TypeSpec(TypeDesc::INT), false /*derivs*/, false /*is_uniform*/, false /*forceBool*/, std::string("raytype_bit"));
            
llvm::Value *args[] = {
                sg,
                rop.llvm_void_ptr (loc_raytype_bit ),
                rop.llvm_void_ptr (Name),
                rop.ll.mask_as_int(rop.ll.current_mask())};
rop.ll.call_function (rop.build_name(func_spec), args);

llvm::Value* raytype= rop.ll.op_load(loc_raytype_bit );
llvm::Value* wide_bit = rop.ll.widen_value(bit);
llvm::Value* bitanded = rop.ll.op_and(raytype, wide_bit );
llvm::Value* ret = rop.ll.op_int_to_bool_to_int(bitanded);
OSL_DASSERT(!result_is_uniform);
rop.llvm_store_value (ret, Result);

To simplify, you can can probably combine some of the code between the uniform and non-uniform branches

@@ -858,6 +861,10 @@ class OSLEXECPUBLIC LLVM_Util {
llvm::Value *op_int_to_bool (llvm::Value *a);
llvm::Value *op_float_to_double (llvm::Value *a);
llvm::Value *op_int_to_longlong (llvm::Value *a);
// int_to_bool_to_int is essentially turning any nonzero -> 1
llvm::Value *op_int_to_bool_to_int (llvm::Value *a) {
return op_bool_to_int(op_int_to_bool(a));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does op_int_to_bool_to_int generate better code than opt_bool_to_int(op_ne(a, constant(0)))?
If so might be useful elsewhere.

DECL (osl_area, "fX")
DECL (osl_filterwidth_fdf, "fX")
DECL (osl_filterwidth_vdv, "xXX")
DECL (osl_raytype_bit, "iXi")
DECL (osl_raytype_bit_from_name, "iXi")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see osl_raytype_bit_from_name referenced anywhere?
Although I do support using that name instead of just osl_raytype_bit.

// Asked if the raytype is a name we can't know until mid-shader.
OSL_SHADEOP int osl_raytype_name (void *sg_, void *name)
// FIXME: We should pass the context, not the sg.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a FIXME, any technical barrier to passing context? For that matter, I've been pondering a ReadOnlyContext or DeviceContext that might be a stripped down version of the full context to answer just these types of questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants