I've gotten around to trying out the exciting new OSL batched execution mode for Gaffer.

For context, Gaffer's internal use of OSL is a bit atypical - we're not a renderer, but we offer OSL for processing images and geometry - it works quite nicely for basic operations, and we're looking at extending it to be more powerful.

The new batched stuff should be an easy win for this application - we have large contiguous buffers to operate on.  A quick preliminary test showed a slightly better than factor of 2 speedup using AVX2 with a naive mandelbrot evaluator written in OSL - not the most realistic test, but fairly promising.

We currently use debug closures with a custom keyword parameter to define the output from the OSL shaders we run on images or geometry - we may not actually need to use keyword parameters, but it's been convenient, and I assume it's intended to still work.

The behaviour I'm seeing is that when I retrieve the value of the parameters from `shaderGlobals.varying.Ci[ i ]` for each `i` in WidthT, the parameter that was set up as a keyword parameter has the same value for every index.  Making things more peculiar, if it's a vector or matrix type, subsequent indices into the vector or matrix parameter return the first component but for a different shading point.  For example, if I shade 4 points with `u` ranging from 0.25 to 1, with a shader that sets a keyword closure parameter to `color( u, u * 10, 0 )`, I would expect the parameter values of the closures stored in `shaderGlobals.varying.Ci[ i ]` to be <0.25, 2.5, 0>, <0.5, 5.0, 0>, <0.75, 7.5, 0> and <1.0, 10.0, 0>.  But instead I get <0.25, 0.5, 0.75> repeated for all Ci pointers within the batch.

I'm definitely not ruling out that I could be doing something wrong - I'm early in prototyping this, and I certainly don't fully understand all the details.  But there isn't really any difference in how I access keyword and non-keyword closure parameters, and it seems like they're set up properly, because they both work in non-batched execution ... I haven't been able to find anything I'm doing wrong, and I know this SIMD support is pretty new, so it seems plausible there could be a bug.

My next step would usually be to try and reproduce this without using any Gaffer code, to try and rule out any mistakes I may have made, but I'm not sure what the easiest way to do that is - I don't see any test code in the OpenShadingLanguage that would exercise this?  I think the easiest test might be to try and hack in support for closures in testshade - doing this properly would probably require a fair bit of work, but maybe it wouldn't take long to hack something up that would be just enough to temporarily demonstrate this issue.  Before I dig into this though, I figured it wouldn't hurt to just ask about this here - maybe someone is already aware of this, or maybe someone can just look at the batched llvm gen, and see if they can identify a bug in there ( I haven't really worked much on LLVM stuff, and I don't really understand that code, but I can see that llvm_gen_keyword_fill for batched execution is a separate code path, so if it's not being exercised by the tests, it's plausible there could be a bug ).

I would appreciate any help in either confirming that this is a bug, or in confirming that varying closure keyword parameters can be correctly evaluated in batched execution, and that the error must be somewhere in my code.

-Daniel Dresser

Since I've now got a clean repro independent of my code, I went ahead and made an issue: https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/issues/1619

-Daniel

Hmm, this is a bit more complex than what you're describing, because the batched_llvm_gen version of llvm_gen_closure doesn't just switch the target of the memcpy, it replaces the memcpy completely with a pair of for loops over num_elements and num_components, with manual loads and stores.  Based on your description, it's unclear whether I need to copy the logic from llvm_gen_closure to llvm_gen_keyword, or whether I should just switch the target of the memcpy.

I flipped a coin and guessed I should do the former, and it does in fact seem to be working in the simplest case.  I'll try out a more complex example tomorrow.

My current diff for batched_llvm_gen is:

7067c7067
<                       ustring clname, llvm::Value* mem_void_ptr, int argsoffset)
---
>                       ustring clname, llvm::Value* mem_void_ptr, int argsoffset, int lane_index)
7091,7094c7091,7127
<                 llvm::Value* dst = rop.ll.offset_ptr(mem_void_ptr, p.offset);
<                 llvm::Value* src = rop.llvm_void_ptr(Value);
<                 rop.ll.op_memcpy(dst, src, (int)p.type.size(),
<                                  4 /* use 4 byte alignment for now */);
---
>
>                 bool arg_is_uniform = Value.is_uniform();
>
>                 TypeDesc simpletype(Value.typespec().simpletype());
>                 int num_elements   = simpletype.numelements();
>                 int num_components = simpletype.aggregate;
>
>                 llvm::Value* dest_base = rop.ll.offset_ptr(mem_void_ptr, p.offset);
>                 dest_base              = rop.llvm_ptr_cast(dest_base, p.type);
>
>                 for (int a = 0; a < num_elements; ++a) {
>                     llvm::Value* arrind = simpletype.arraylen ? rop.ll.constant(a)
>                                                               : NULL;
>                     for (int c = 0; c < num_components; c++) {
>                         llvm::Value* dest_elem;
>                         if (num_components > 1)
>                             dest_elem = rop.ll.GEP(dest_base, a, c);
>                         else
>                             dest_elem = dest_base;
>
>                         // NOTE:  We don't want any uniform arguments to be
>                         // widened, so our typical op_is_uniform doesn't do what we
>                         // want for this when loading.  So just pass arg_is_uniform
>                         // which will avoid widening any uniform arguments.
>                         llvm::Value* loaded
>                             = rop.llvm_load_value(Value, 0, arrind, c,
>                                                   TypeDesc::UNKNOWN,
>                                                   /*op_is_uniform*/ arg_is_uniform,
>                                                   /*index_is_uniform*/ true);
>
>                         if (!arg_is_uniform) {
>                             loaded = rop.ll.op_extract(loaded, lane_index);
>                         }
>                         rop.ll.op_unmasked_store(loaded, dest_elem);
>                     }
>                 }
>
7278c7311
<                               2 + weighted + clentry->nformal);
---
>                               2 + weighted + clentry->nformal, lane_index);