Skip to content

Conversation

@diiigle
Copy link

@diiigle diiigle commented Sep 8, 2025

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
Copy link
Author

diiigle commented Sep 8, 2025

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
Copy link
Member

wjakob commented Sep 15, 2025

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
Copy link
Author

diiigle commented Sep 15, 2025

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
Copy link
Member

wjakob commented Sep 15, 2025

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
Copy link
Author

diiigle commented Sep 15, 2025

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.

Copy link
Member

@wjakob wjakob left a comment

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.

@diiigle
Copy link
Author

diiigle commented Oct 1, 2025

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

}

~Local() = default;
Local(const Local &) = delete;
Copy link
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
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