Fix Issue With Evaluating Varying Closure Keyword Parameters During Batched Evaluation #1620
Conversation
|
|
src/liboslexec/batched_llvm_gen.cpp
Outdated
| : NULL; | ||
| for (int c = 0; c < num_components; c++) { | ||
| llvm::Value* dest_elem; | ||
| if (num_components > 1) |
There was a problem hiding this comment.
What happens for an array of of objects with 1 component?
Looks like maybe the else case is taken which disregards the "a" which is the element index.
Does the else case need to incorporate the arrayind or "a"
There was a problem hiding this comment.
Hmm, that does look like a bug to me too ... which quite possibly means it is also a bug on line 7243 where I copied the code from?
Not that I want to make it harder to get this quick fix I need over the line, but I do wonder if we should add a closure with more types of parameters to testshade so that it's possible to actually write testcases for this code.
There was a problem hiding this comment.
This same comment might apply to the copy loop in llvm_gen_closure of batched_llvm_gen.cpp
There was a problem hiding this comment.
Following the adage of "if its not tested its broken", yes being able to test these combinations would be a great idea
There was a problem hiding this comment.
This started out as a quick fix to a bug that was holding me up ... modifying testshade seems like a somewhat larger change - adding new closure parameters to testshade feels a bit more involved, since they would potentially be visible to other users of OSL. Does someone want to decide what this should look like? Should we add some keyword parameters to debug()? Oh, wait, looking at the list of macros in genclosure.h, it looks like keyword array parameters aren't currently supported anyway? So potentially I could drop the iteration code and use a simple memcpy - though the bug would still potentially exist in the non-keyword array parameter case that already exists. Should we add a special test closure with a parameter of every type in genclosure.h?
There was a problem hiding this comment.
I agree, that looks like a bug. I think the two main purposes of testshade are to provide an example of how to integrate the library and to allow for testing the important features of the library. A special 'test' closure which exercises the types in genclosure.h would be something that accomplishes both, so sounds like a good idea to me -- it'll keep me from accidentally checking things like this in that work for our renderer which doesn't make heavy use of closures but break those that depend on them heavily.
There was a problem hiding this comment.
I think you can still get this fix over the fence without everything else, maybe remove the array index stuff if you know its not needed, replace it with an assert? You have added a test case and modified source to allow it to pass without breaking any other testcases. We just noticed some other issues along the way. Maybe drop a "todo" on the line 7243 to add testcase and fix bug.
danieldresser-ie
force-pushed
the
batchVaryKey
branch
from
be73c33
to
7d20021
Compare
|
As per Alex's comment, I've removed support for array indexing, avoiding the bug he observed with the faulty array handling I copied. I wasn't able to get it working with one big memcpy ... unsure if this is due to not understanding the context well enough, or whether a per-component approach is required in order to get masking and such working. Anyway, I started from the other end, with the more complex code that worked, and removed the array handling but kept the component handling - this seems to have resulted in working code, and I've removed the array part, and added an assert that the arraylen is 0. As for the bug at line 7243, I've added #1621 with a test case that demonstrates that bug - does it make sense to merge this and then continue that part of the conversation over there? |
danieldresser-ie
force-pushed
the
batchVaryKey
branch
from
7d20021
to
60281d1
Compare
) (AcademySoftwareFoundation#1620) When evaluating varying closure keyword parameters in batched execution, the value of the first lane in the batch would be used for the whole batch. Signed-off-by: Daniel Dresser <danield@image-engine.com>
) (AcademySoftwareFoundation#1620) When evaluating varying closure keyword parameters in batched execution, the value of the first lane in the batch would be used for the whole batch. Signed-off-by: Daniel Dresser <danield@image-engine.com>
When evaluating varying closure keyword parameters in batched execution, the value of the first lane in the batch would be used for the whole batch. More details in an issue I made here: #1619
I don't really understand this code at all, this fix was just made by following the suggestion of Stephen on the dev list to copy the code from llvm_gen_closure to llvm_gen_keyword_fill.
It fixes the one simple case which I've added a test for, but the more complex functionality of this code I copied would only be tested if you used array keyword parameters on a closure. This would be more annoying to test, since I don't think testshade declares any closures with array keyword parameters.
Also, while testing, I found a segfault with adding a null closure in batched evaluation - I'll post a repro for that on the dev list, and hopefully we can clean that up soon too.