Skip to content

Feature completion for C++ drjit::Local - #442

Open
diiigle wants to merge 5 commits into
mitsuba-renderer:masterfrom
diiigle:local_cpp
Open

Feature completion for C++ drjit::Local#442
diiigle wants to merge 5 commits into
mitsuba-renderer:masterfrom
diiigle:local_cpp

Conversation

@diiigle

@diiigle diiigle commented Sep 8, 2025

Copy link
Copy Markdown

Closes #426

Adding multiple features to the C++ version of drjit::Local:

  • Dynamic width of arrays
  • Dynamic size of local memory (for JIT variants)
  • Support for arbitrarily nested DRJIT_STRUCT data structures

All while retaining the stack-allocated scalar variant.

TODO before merging:

  • Tests for DRJIT_STRUCT support

@diiigle

diiigle commented Sep 8, 2025

Copy link
Copy Markdown
Author

Formatting

What settings for clang-format are you using? Unfortunately there is no .clang-format file that unifies settings across the project.

Naming

Please comment on the naming of functions and variables. I tried to keep it consistent with existing drjit nomenclature, but might have diverged accidentally. This code has seen a few iterations so there might be organically-grown inconsistency.

@wjakob

wjakob commented Sep 15, 2025

Copy link
Copy Markdown
Member

Hello @diiigle,

the formatting looks fine. The codebase usually skips opening/closing braces for conditionals/loops if the body is a simple 1-liner.
We use the \param / \brief Doxygen syntax instead of the Java-like @param / @brief.

Please let us know once you've tested DRJIT_STRUCT support, and then we can ship this with the next release.

@diiigle

diiigle commented Sep 15, 2025

Copy link
Copy Markdown
Author

Hi @wjakob,

Thanks for the feedback. I adjusted the formatting in 2559c66.

Struct support is now fully tested on JIT and scalar variants with the addition of 48f58ee. I had to add the Index type as a template parameter because I couldn't figure out how to access the underlying array type from a DRJIT_STRUCT. I believe there are no type traits to do so.

Everything can be squashed before merging

@diiigle
diiigle marked this pull request as ready for review September 15, 2025 13:35
@wjakob

wjakob commented Sep 15, 2025

Copy link
Copy Markdown
Member

Two more minor requests: can you move the lambda functions out into top-level (template) functions that take counter as reference? e.g.

First:

    template <typename T>
    Value read_impl(T &result, uint32_t offset, uint32_t active, size_t &counter) const {
        if constexpr (is_jit_v<T> && depth_v<T> == 1) {
            if (counter >= m_arrays.size())
                jit_raise("Local::read(): internal error, ran out of "
                          "variable arrays!");
            result = T::steal(jit_array_read(m_arrays[counter++], offset, active));
        } else if constexpr (is_traversable_v<T>) {
            /// Recurse and try again if the object is traversable
            traverse_1(fields(result), [&](auto &r) {
                read_impl(r, offset, active, counter);
            });
        }
    }

Some compilers (MSVC cough) are not very good about compiling lambda functions, and nesting them two levels deep is just asking for trouble.

Second: Both files have whitespace errors (trailing whitespace). I usually set my editor to mark those in red so that it's easily noticable.

@diiigle

diiigle commented Sep 15, 2025

Copy link
Copy Markdown
Author

Yeah I had been following the ideas in this post to get the neatest assembly out of nested lambdas, but I understand a normal templated function should be much simpler to compile.

@wjakob wjakob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more minor things, then this is ready to go in.

Comment thread include/drjit/local.h Outdated
Comment thread include/drjit/local.h Outdated
@diiigle

diiigle commented Oct 1, 2025

Copy link
Copy Markdown
Author

Squashed any temporary commits and rebased onto master. Ready to merge from my side.

Comment thread include/drjit/local.h Outdated
@@ -29,15 +39,9 @@ template <typename Value, size_t Size, typename SFINAE = int> struct Local {

~Local() = default;
Local(const Local &) = delete;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjakob What is the reasoning for disallowing copies? I assume it's performance motivated?

That prevents the use of dr::Local as a member in a DRJIT_STRUCT higher up, as they require

Name &operator=(const Name &) = default;                                   \

Meaning it can be wrapped in a DRJIT_STRUCT itself
@njroussel
njroussel force-pushed the master branch 2 times, most recently from fc79b70 to bdc17f6 Compare February 13, 2026 15:57
@wjakob
wjakob force-pushed the master branch 3 times, most recently from 3912746 to 45500e3 Compare June 18, 2026 22:11
@wjakob
wjakob force-pushed the master branch 4 times, most recently from 4fc980f to 32bcaa6 Compare June 29, 2026 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dr::Local in C++ roadmap

2 participants