Refactor/cxx20 modernization - #141
Merged
ajaypanyala merged 22 commits intoJul 5, 2026
Merged
Conversation
…types, CMake - block_span.hpp: Replace raw ptr+vector with std::mdspan (dynamic rank), add make_block_span factory, retain BufKind tag, add [[nodiscard]] - strong_num.hpp: Replace 12 comparison operators with operator<=> (spaceship), add StrongNumeric concept, replace enable_if with requires clauses, add [[nodiscard]] on value(), remove #define DEBUG_STRONGNUM in favor of inline constexpr bool - errors.hpp: Replace __FILE__/__LINE__ macro patchwork with std::source_location, add tamm_expects() inline function, retain macro wrappers for ABI compat, UNREACHABLE() now calls std::unreachable() (C++23 compat path) - boundvec.hpp: Add std::copyable concept constraint, replace manual destructor calls with std::destroy_at, replace placement-style assignment with std::construct_at, add spaceship operator==/<=> - types.hpp: Replace __cplusplus<201703L apply shim (now guaranteed C++20), add inline constexpr maxrank, harden hash_combine with std::bit_cast, add [[nodiscard]] to pure query functions - CMakeLists.txt: Enforce cxx_std_20, add FetchContent fallback for kokkos/mdspan on older stdlibs (GCC<13, Clang<17)
Restores all /** @brief ... @tparam ... @PARAM ... @return */ Doxygen blocks and inline doc-comments that were dropped during the C++20 modernization commit (18f02de). - strong_num.hpp: restore @todo blocks, @brief/@tparam/@param/@return for checked_cast overloads, class-level Doxygen for StrongNum, @brief for value_type/NumType aliases, and /**< */ member doc - errors.hpp: restore @brief Doxygen for NOT_IMPLEMENTED, NOT_ALLOWED, UNREACHABLE, EXPECTS, EXPECTS_STR macros - boundvec.hpp: restore full @brief/@tparam/@param/@return/@pre Doxygen for all methods and the class itself; restore @brief for operator== / operator!= / operator<< - block_span.hpp: restore /// doc-comments on all accessors and constructors to match original intent - types.hpp: restore @param/@return doc-comments on to_ga_eltype and from_ga_eltype; restore inline comments on aliases
- lru_cache.hpp: initialize cycle_ to 0 in-class (was UB on first use); fix access() to return it->second instead of dereferencing the map iterator directly (which yields a pair, not Value) - perm.hpp: silence signed/unsigned comparison warning in perm_map_apply by casting pm to size_t before comparing with input_vec.size(); use std::distance for iterator subtraction in perm_compute/perm_map_compute instead of raw pointer arithmetic - boundvec.hpp: fix resize() EXPECTS guard — size_type is unsigned so sz >= 0 is a tautology and was never firing; replace with a meaningful upper-bound check: EXPECTS(sz <= maxsize) - types.hpp: make make_label() thread-safe by using std::atomic<Label> with memory_order_relaxed for the monotonic counter
block_buffer.hpp: - Copy ctor: initialize allocated=false before the delete guard to avoid reading an uninitialized member (UB on the freshly-constructed object) - Copy assignment: fix typo re=block_buffer.indexedTensor -> re=block_buffer.re - Move ctor: steal the allocated flag from the source and reset it to false so the moved-from dtor does not double-delete the buffer block_mult_plan.hpp: - prep_flat_plan: fix tautological comparison lhs==rhs1 && lhs==rhs1; second condition must be lhs==rhs2 - prep_loop_gemm_plan: fix copy-paste bug where both rhs1_pos and rhs2_pos searched [rhs1_labels_.begin(), rhs2_labels_.end()); each must search its own range - GemmPlan: initialize num_batch_indices_=0 in-class (was uninitialized, causing UB in update_plan/apply on every batched GEMM) - GemmPlan::update_plan: remove local 'int num_batches_' that shadowed the class member; the member is now correctly written so apply() sees the computed batch count - scalar_vec_mult_update/assign: remove spurious N-element loop wrapping a flat_update/flat_assign call that already processes all elements; loop caused O(N^2) work instead of O(N) - GeneralMultPlan: cache the three intermediate buffers as members; resize only when block dimensions change instead of heap-allocating on every apply() call (hot path in CCSD doubles iterations)
types.hpp: - Change IndexVector from std::vector<Index> to BoundVec<Index, maxrank> eliminating one heap allocation per distributed block get/put/add call; IndexVectorHash/Equal updated to work with BoundVec - Add stack-allocated BlockIdVec alias for local use block_buffer.hpp: - Replace raw new[]/delete[] + bool allocated ownership with std::vector<T> storage_ (owner) + std::span<T> buf_span_ (non-owning view) - Rule-of-Zero: copy/move/dtor are all compiler-generated and correct; eliminates the three ownership bugs fixed in the prior commit permanently - release_put/add and release() updated to clear storage_ directly block_span.hpp: - block_dims() now returns std::span<const size_t> view into a cached TensorVec<size_t> extents_ member instead of heap-allocating a new std::vector on every call; old vector-returning overload kept as block_dims_vec() for callers that truly need ownership - All internal uses of block_dims() updated to use the span overload block_mult_plan.hpp: - GeneralMultPlan: cache permuted block dims (linter_dims_, r1inter_dims_, r2inter_dims_) as members; computed once in apply() only when the block shape changes (guarded by extent comparison), eliminating perm_map_apply vector allocation on every contraction block - BlockMultPlan::apply_impl: replace per-call plan reconstruction with a cached std::variant<FlatBlockMultPlan,GemmPlan,TTGTPlan,GeneralMultPlan> member built once in the constructor; apply_impl becomes a trivial std::visit dispatch with zero label-vector copies at call time - All IndexLabelVec constructor parameters changed to std::span<const TiledIndexLabel> to avoid copies at call sites block_assign_plan.hpp: - Cache the complex-type scratch buffer new_rhs_vec_ as a member; resize only on growth (same pattern as GeneralMultPlan buffers) eliminating one heap allocation per complex block assignment
block_mult_plan.hpp:
Bug 1 — GeneralMultPlan: intermediate buffers were hardcoded to
std::vector<double>, causing type-punning / memory corruption for
float and complex tensor types. Changed to std::vector<std::byte>
with byte-count sizing (nelems * sizeof(T)) so the same cached buffer
works correctly for any element type T.
Bug 2 — GemmPlan::update_plan: bufa_/bufb_/bufc_ were stored via
'&lhs.buf()' — taking the address of a T* rvalue returned by buf(),
which is a dangling pointer / UB. Changed to store the data pointer
value directly: bufc_ = lhs.buf() (void* = T*, implicit cast).
Bug 3 — BlockMultPlan::apply() public overload (Scalar lscale/rscale)
had an empty body {}, silently discarding every contraction invoked
through the Scalar API. Replaced with a forwarding call to
apply_impl() after casting Scalar to T1 so all contraction paths
actually execute.
Stabilize the C++20 modernization branch and fix latent bugs found while auditing the surrounding code. cxx20 refactor regressions: - types.hpp: revert IndexVector to std::vector<Index> (BoundVec<Index,8> overflowed for index/tile lists that hold thousands of entries and lacked data()/erase()/range-insert/(count,value) API used at ~15 call sites) - block_span.hpp: remove impossible std::dextents<size_t,dynamic_extent> (dynamic-rank mdspan is ill-formed); model blocks as ptr+extents, add operator[]/data()/flat_span(); block_dims() returns const vector& to match kernel signatures - block_mult_plan.hpp: drop UB reinterpret_cast<BlockSpan<T1>&>; constrain apply() to T1==T2==T3; use Scalar::get<T1>() instead of illegal static_cast - scalar.hpp: add type-safe Scalar::get<T>() variant extractor - boundvec.hpp: restore (count,value) ctor, data(), reverse iterators, range/count insert_back; constrain iterator ctors with std::input_iterator - CMakeLists.txt: drop unused kokkos/mdspan FetchContent Pre-existing bugs: - blockops_cpu.hpp: ipgen_idx off-by-one OOB read; index_permute_update(lscale) lbuf[i] -> lbuf[c] - tensor_impl.hpp: spin ignore used spin_sizes[1] instead of [2] - setop.hpp: writes/reads/accumulates(ec) missing return in else branch - addop.hpp/setop.hpp: remove self-assign in optype ternary - range.hpp / index_space.hpp / tiled_index_space.hpp: initialize previously uninitialized members (Range lo_/hi_/step_, hash_value_) - index_space_interface.hpp: rename inverted has_duplicate() -> has_no_duplicate() - tiled_index_space.hpp: collapse redundant map lookups (single find + if-init)
The cxx20 strong_num refactor broke widespread call sites:
- StrongNum value ctor was made explicit, breaking implicit conversions
relied on everywhere (Proc p = GA_Nodeid(), Offset off = 0,
Size sz = block_size(...), vector<Proc>::resize(n,1)). Make it implicit.
- operator<=> alone does not synthesize ==/!=; add a defaulted
operator==(const NumType&) so same-type == and != work again
(e.g. step_proc_ != Proc{1}).
- errors.hpp: EXPECTS now uses static_cast<bool>(cond) so pointer-like
conditions with an explicit operator bool (shared_ptr/unique_ptr) still
work, e.g. EXPECTS(distribution_).
- blockops_cpu.hpp: include tamm/scalar.hpp (uses Scalar directly).
GCC 14.3 (C++20) eagerly instantiates the defaulted special members of the *IndexSpaceImpl classes during class definition. Those classes hold std::vector<TiledIndexSpace> members, so the element type must be complete at that point; with only the forward declaration from types.hpp the build failed with "arithmetic on a pointer to an incomplete type 'tamm::TiledIndexSpace'". Include tiled_index_space.hpp here. This header is only included by index_space.cpp and tiled_index_space.hpp does not include it back, so no include cycle is introduced.
Remove dead code, collapse duplication, and modernize with C++20; no public API/ABI change (verified against exachem usage) and no UPC++ sections touched. Net ~1050 fewer lines plus one correctness fix. Dead code: - delete op_cost.hpp (unused OpCostTerm/Expr/Order) and its include/CMake entry - delete op_executor_analysis.hpp (dead duplicate of op_executor.hpp) Op layer: - add internal::validate_index_labels(); collapse 5 identical validate() bodies (mult/add/set/scan/map) into one ranges-based helper - clone() -> std::make_shared<InjectedClassName>(*this) across all 7 op types - flatten the 4 near-identical *Plan subclasses (mult/add/set) to a shared base with direct writes()/accumulates()/reads(); add virtual dtor (fixes deleting derived via base shared_ptr); the former local/global split was a scheduling no-op with no external callers - Rule of Zero: drop redundant =default copy-ctors / hand-written specials (op types, LabelMap, ProcGroup) to restore implicit moves Ranges / accessors: - is_dense/is_sparse/is_dense_labels/has_sparse_labels -> std::ranges none_of/any_of - std::iota (MPI-only paths) and vector::insert(count,value) for fill loops - tindices()/Distribution::proc_grid() return const& (+ [[nodiscard]]) - reuse internal::merge_vector for label concatenations Fixes: - block_lambda_plan prep_flat_plan compared labels_list[1] on every iteration instead of [i], skipping size checks for operands >=2; rewritten with std::ranges::equal - remove stray duplicate #pragma once at EOF of label_translator.hpp
abagusetty
marked this pull request as draft
July 5, 2026 16:01
ajaypanyala
marked this pull request as ready for review
July 5, 2026 16:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Is this pull request associated with an issue(s)?
N/A
Description
Adds C++20 feature support with modern CXX practices
TODOs
N/A