Refactor LdgIterator into wrapper function and ADL access#2343
Refactor LdgIterator into wrapper function and ADL access#2343sethrj wants to merge 14 commits intoceleritas-project:developfrom
Conversation
sethrj
added
core
Test summary 5 975 files 9 692 suites 19m 47s ⏱️ Results for commit 963a36e. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2343 +/- ##
=========================================
Coverage 87.24% 87.25%
=========================================
Files 1377 1377
Lines 43848 43867 +19
Branches 13432 13880 +448
=========================================
+ Hits 38257 38276 +19
- Misses 4359 4527 +168
+ Partials 1232 1064 -168
🚀 New features to boost your workflow:
|
Do you have a reference to to rule causing an issue with looking up these operators? I've tried to compile in LdgIterator.test.cc and it is finding operator+ |
|
@esseivaju The issue arose when adjusting the The problem with that is that if {
using DozenDbl = Quantity<DozenUnit, double>;
auto two_dozen = native_value_to<DozenDbl>(24);
auto td_ref = std::ref(two_dozen);
EXPECT_EQ(two_dozen * 2, td_ref * 2);
}will fail on develop with the error: Maybe i should just move all the operator shenanigans to a separate PR, and then have a separate discussion for LDG iterator. |
sethrj
changed the title
Replace the LdgTraits specialization-based customization point with an ADL-based free function ldg_data(T const*) -> U const*. The built-in overloads for arithmetic and enum types live in LdgTraits.hh; the OpaqueId and Quantity overloads now live alongside their respective types in OpaqueId.hh and Quantity.hh. The is_ldg_supported_v trait is re-expressed using void_t SFINAE detection over the ADL call. Update ldg() in LdgRefWrapper.hh to call ldg_data via ADL and derive the arithmetic check from the returned pointer type rather than a traits member. Add a two-argument ldg(obj, mp) convenience overload and a storable LdgMember<Class,T> functor with a CTAD deduction guide for loading a member-pointer field from a struct via __ldg. Add test coverage for both the two-argument ldg and LdgMember callable. Prompt: "Please implement those changes in the code, replacing all use of the old LdgTraits." Assisted-by: GitHub Copilot
Replace the one-line \page stub with a complete reference page that covers: - What __ldg does and when to use it (scalar, struct member, span) - Code examples for each usage pattern including LdgMember and LdgSpan - How to extend ldg support to a custom type via ldg_data ADL overload - Built-in overloads for arithmetic, enum, OpaqueId, and Quantity - Brief descriptions of LdgWrapper and LdgIterator implementation detail Also cleans up the broken placeholder text in the ldg() function doc comment. Prompt: "Combine the main documentation of the ldg-related functions into a doxygen `\page ldg Cached device loading`. Provide examples of usage in user code (data, struct, span), and of overloading the ldg_data. Briefly mention and summarize the under-the-hood LdgWrapper and LdgIterator." Assisted-by: GitHub Copilot
sethrj
changed the title
sethrj
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR refactors LDG (“cached device loading”) support by replacing the old LdgTraits/LdgIterator approach with an ADL-based ldg_data customization point and a proxy LdgWrapper used by LdgSpan, improving extensibility (including member loads) and simplifying includes as part of the broader Span/OpaqueId refactor.
Changes:
- Introduces
corecel/data/Ldg.hhwithldg+ ADL-dispatchedldg_data, plusLdgMemberand 2-argldg(obj, member_ptr)support. - Reintroduces
corecel/cont/LdgSpan.hhand implements LDG span behavior viadetail::LdgWrapper/detail::LdgIterator. - Updates call sites, docs, and tests; removes the legacy
LdgTraits.hhandLdgIterator.hh.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/corecel/OpaqueIdUtils.hh | Switches tests/utilities to include and use LdgSpan/ldg headers. |
| test/corecel/data/Ldg.test.cc | Adds coverage for 2-argument ldg and LdgMember callable behavior. |
| test/corecel/cont/Span.test.cc | Removes LDG span tests from the generic Span test file (migrated). |
| test/corecel/cont/LdgSpan.test.cc | Updates/expands LDG tests for wrapper + iterator + span behavior. |
| test/corecel/CMakeLists.txt | Registers new/renamed LDG-related tests. |
| src/orange/univ/detail/LogicEvaluator.hh | Updates includes to use LdgSpan.hh rather than the removed iterator header. |
| src/orange/univ/detail/InfixEvaluator.hh | Updates includes to use LdgSpan.hh rather than the removed iterator header. |
| src/geocel/VolumeView.hh | Updates include for LDG access (currently mismatched with actual LdgSpan usage). |
| src/corecel/OpaqueId.hh | Replaces LdgTraits specialization with ldg_data overload for ADL. |
| src/corecel/math/Quantity.hh | Replaces LdgTraits specialization with ldg_data overload for ADL. |
| src/corecel/data/LdgTraits.hh | Deletes the old LDG traits mechanism. |
| src/corecel/data/LdgIterator.hh | Deletes the old LDG iterator/value-marker implementation. |
| src/corecel/data/Ldg.hh | Adds new LDG API + documentation page (\\page ldg). |
| src/corecel/data/detail/CollectionImpl.hh | Uses is_ldg_supported_v to return LdgSpan for device const_reference collections. |
| src/corecel/data/Collection.hh | Minor doc fix; removes unused include. |
| src/corecel/cont/Span.hh | Updates LDG-related wording (but still mentions legacy LdgValue elsewhere). |
| src/corecel/cont/LdgSpan.hh | Reintroduces LdgSpan API, to_array overload, and make_span helper. |
| src/corecel/cont/detail/SpanImpl.hh | Updates span traits to use LdgWrapper rather than the removed LdgValue. |
| src/corecel/cont/detail/LdgSpanImpl.hh | Adds LdgWrapper/LdgIterator implementation and LDG support detection. |
| src/celeritas/grid/GridIdFinder.hh | Simplifies includes to rely on LdgSpan types. |
| doc/implementation/data-model.rst | Adds documentation references for the new LDG doxygen page and LdgSpan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@amandalund I've got this working the CI and a separate CUDA 12.9 build, but nvcc initially gave some weird compiler warnings that I didn't see on any of the normal compilers. Since you saw something on your own CUDA build that I couldn't reproduce, could you check out this branch to see if it builds? |
|
@sethrj checked and it builds with no issues |
Add operator==(LdgWrapper, LdgWrapper) and operator!=(LdgWrapper, LdgWrapper) overloads to LdgWrapper to resolve ambiguity when std::equal iterates over two LdgSpan ranges. Previously, comparing two LdgWrapper instances matched both operator==(LdgWrapper, type) and operator==(type, LdgWrapper) after implicit conversion, causing a hard compile error via libc++ __equal_to. Add a LdgWrapperTest.equality test that exercises LdgWrapper == LdgWrapper comparisons directly and via std::equal over two LdgSpans. Prompt: "Fix the build errors due to LdgWrapper's ambiguous types. Add new tests for LdgWrapper equality that demonstrates the build failure and its fix." Assisted-by: GitHub Copilot
|
@esseivaju Could you review this update? Thanks! |
This rethinks the LDG wrapping as part of a core Span/OpaqueId refactor before separating it out in #2338. Before,
ldgwas called as part ofLdgIterator. Now,LdgIteratoris simply a proxy iterator: it returns aLdgWrapper<T>, modeled after std::reference_wrapper, that implicitly casts to the type being wrapped, and loads on that access.The refactored ADL lookup allows a simpler override for loading data via ldg, and a two-argument override even allows you to
ldgdata members from inside a struct:This also reinstitutes
cont/LdgSpan.hh, which is once again the correct file to include when returning LdgSpan data via collections as part of a View.NOTE: I moved the friend operator changes to #2347 . That PR is required for this one to compile.