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

problem with integer geometry attributes and batched shader execution #1929

Open
cmstein opened this issue Jan 22, 2025 · 5 comments
Open

problem with integer geometry attributes and batched shader execution #1929

cmstein opened this issue Jan 22, 2025 · 5 comments
Assignees
Labels
batch shading Specific to the SIMD batch shading back end

Comments

@cmstein
Copy link
Collaborator

cmstein commented Jan 22, 2025

I'm seeing a problem with batched execution that I've managed to distill to the repro case below with testshade. Basically, if I have an integer geometry attribute, then batched OSL execution seems to disregard what the renderer writes to the destination depending on what the default value is specified in the shader. For eample, suppose I have a shader with the following parameter declaration, int face_idx = 0 [[int lockgeom=0]], the value returned by the renderer doesn't make it to the shader. But in this case it does: face_idx = 2 [[int lockgeom=0]]. FWIW, a value of '1' also fails.

This is based on OSL-v1.14.2.0 which goes dates back to September so it's possible this has been fixed?

Here's my example shader:

shader example(int face_idx = 0 [[int lockgeom = 0]],
               output color dst = 0)
{
    dst = color(face_idx * 0.25, 0, 0);
}

A patch to add the face_idx user-data to batched testshade:

diff --git a/src/testshade/batched_simplerend.cpp b/src/testshade/batched_simplerend.cpp
index ea2acbdf..6618fba7 100644
--- a/src/testshade/batched_simplerend.cpp
+++ b/src/testshade/batched_simplerend.cpp
@@ -21,6 +21,7 @@ struct UniqueStringCache {
         , red("red")
         , green("green")
         , blue("blue")
+        , face_idx("face_idx")
         , lookupTable("lookupTable")
         , blahblah("blahblah")
         , options("options")
@@ -51,6 +52,7 @@ struct UniqueStringCache {
     ustring red;
     ustring green;
     ustring blue;
+    ustring face_idx;
     ustring lookupTable;
     ustring blahblah;
     ustring options;
@@ -663,7 +665,17 @@ BatchedSimpleRenderer<WidthT>::get_userdata(ustringhash name,
 
     // For testing of interactions with default values
     // may not provide data for all lanes
-    if (name == ucache().s && Masked<float>::is(val)) {
+    if (name == ucache().face_idx && Masked<int>::is(val)) {
+        printf("found face_idx!\n");
+        Masked<int> out(val);
+        for (int i = 0; i < WidthT; ++i) {
+           if (out[i].is_on()) {
+               out[i] = int(4 * bsg->varying.u[i]);
+           }
+        }
+        return out.mask();
+    }
+    else if (name == ucache().s && Masked<float>::is(val)) {
         Masked<float> out(val);
         for (int i = 0; i < WidthT; ++i) {
             // NOTE: assigning to out[i] will mask by itself

Command line:
env TESTSHADE_BATCH_SIZE=16 testshade --batched --res 256 256 -o dst output.exr example

Here are the images I get:

With face_idx = 0 [[in lockgeom=0]] I get the wrong image:

Image

With face_idx = 2 [[in lockgeom=0]] I get the expected image:

Image

Is anyone able to reproduce this?

Thanks!

@lgritz lgritz added the batch shading Specific to the SIMD batch shading back end label Feb 8, 2025
@AlexMWells
Copy link
Contributor

I am able to reproduce the problem, think the caching of get_userdata might be broken vs. obtaining the value from the renderer.

@AlexMWells
Copy link
Contributor

Adding --options dump_forced_llvm_bool_symbols=1 we can see the results of the Batched Analysis:

face_idx is forced llvm bool.

Which means face_idx can be only be 0 or 1. When an integer is only assigned a 0 or 1, it is allowed to be forced to be a true boolen vs. an integer.

This is why you got correct results when you had initialized face_idx=2, it wasn't allowed to treat the face_idx as a bool. So some extra logic need to be added to disqualify the "force boolean" when a parameter is bound.

@lgritz
Copy link
Collaborator

lgritz commented Feb 25, 2025

I'm not sure I understand how the force boolean comes into play here?

@AlexMWells
Copy link
Contributor

AlexMWells commented Feb 25, 2025

Its a bug in Batched Analysis, it is not properly considering get_userdata could write any integer value to face_idx. It is only looking at the lack of writers to face_idx in the .oso and the default value.

With
shader example(int face_idx = 0 [[int lockgeom = 0]],
face_idx meets criteria of only having 0 or 1 written to it.
It was forced to be boolean, so didn't have enough enough space to even hold a 8x32bit integers from batched version of get_userdata. And incorrect results were observed.

With
shader example(int face_idx = 2 [[int lockgeom = 0]],
face_idx meets DOES NOT meet criteria of only having 0 or 1 written to it.
It was is left as an integer, batched version of get_userdata worked as expected and we see correct results

@AlexMWells
Copy link
Contributor

I have a fix now, just need to clean it up, should be able to post PR tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
batch shading Specific to the SIMD batch shading back end
Projects
None yet
Development

No branches or pull requests

3 participants