-
Notifications
You must be signed in to change notification settings - Fork 359
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
base: main
Are you sure you want to change the base?
Conversation
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 <[email protected]>
LGTM for the scalar path. |
rop.llvm_load_arg (P, true /*derivs*/, false /*op_is_uniform*/), | ||
rop.llvm_global_symbol_ptr(Strings::flipHandedness), |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
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 [email protected]