mitsuba-renderer / drjit

Dr.Jit — A Just-In-Time-Compiler for Differentiable Rendering
BSD 3-Clause "New" or "Revised" License
563 stars 40 forks source link

Fixes to vectorized calls #224

Closed njroussel closed 4 months ago

njroussel commented 4 months ago

This PR fixes various issues in vectorized calls (and some minor orther stuff).

Minor fixes unrelated to calls in:

Everything else is somehow related to vectorized calls, either only in symbolic mode, or evaluated mode, or for getters or even fordr.dispatch/dr.switch. It might be easier review this PR by looking at the individual commits as they all are independent fixes.

Relies on a fix contained in https://github.com/mitsuba-renderer/drjit-core/pull/88

wjakob commented 4 months ago

These fixes almost all look good to me. Just two comments:

njroussel commented 4 months ago

For https://github.com/mitsuba-renderer/drjit/commit/6299a0f2c2e03ac7d745cd8b7f9951b2a45d75ad, I agree that (uint32_t) JitFlag::SymbolicCalls | (uint32_t) JitFlag::SymbolicLoops should be enough. This has to do with the couple places where we make dummy calls to zero-initialize a return value (here and here). For example, in evaluated mode we'd need to make a call to a dr.switch target just to figure out the return type. If that target function has a symbolic loop or an other vectorized call in it, then the tracing would be halted as it's most likely going to try to execute them in evaluated mode even though we expliclitly want them to be symbolic here as we only need to figure out the return type. I'll change the code, such that it only applies to the two cases mentioned above and without the SymbolicConditionals. Does that sound good?

wjakob commented 4 months ago

I'll change the code, such that it only applies to the two cases mentioned above and without the SymbolicConditionals. Does that sound good?

Instead of changing the JIT flags, how about changing the interpretation of the default execution policy? For example, in src/extra/cond.cpp, we would change the following lines:

diff --git a/src/extra/cond.cpp b/src/extra/cond.cpp
index 3b776dd4..1a9ac2d7 100644
--- a/src/extra/cond.cpp
+++ b/src/extra/cond.cpp
@@ -580,8 +580,16 @@ bool ad_cond(JitBackend backend, int symbolic, const char *label, void *payload,
     if (strchr(label, '\n') || strchr(label, '\r'))
         jit_raise("'label' may not contain newline characters.");

-    if (symbolic == -1)
-        symbolic = (int) jit_flag(JitFlag::SymbolicConditionals);
+    if (symbolic == -1) {
+        uint32_t flags = jit_flags();
+        if (flags & (uint32_t) JitFlag::SymbolicScope) {
+            // We're inside some other symbolic operation, cannot use evaluated mode
+            symbolic = 1;
+        } else {
+            symbolic = (int) flags & (uint32_t) JitFlag::SymbolicConditionals;
+        }
+    }

For src/extra/loop.cpp, the change would be near-identical. For src/extra/call.cpp, make sure to rebase on master. Over the weekend, I added a similar symbolic=-1/0/1 argument to the operation.

njroussel commented 4 months ago

@wjakob That makes sense to me, I've made those changes in https://github.com/mitsuba-renderer/drjit/pull/224/commits/eeb2328f064b28fd74367143c62c508aa6cba445.

I do think this is what gives us the most flexibility, but is also a bit confusing. Let me recap:

For some control flow that is nested in a symbolic computation the following things could happen:

(The last item should be pretty rare, I don't think the warning will be too loud.)

I think this gives us the granularity needed to allow user to force "modes" very precisely on certain constructs, have a decent automatic system and also easily have scoped mode changes across a big section of code