179 refactor recursive template implementations with fold expressions to reduce compiler memory usage and compilation times

[morousg]

No description provided.

[Copilot]

Pull request overview

Refactors core compile-time plumbing around TypeList, OperationTuple, and FusedOperation to use fold expressions (reducing recursive template depth), and updates tests/examples accordingly while moving memory_operations.h to algorithms/basic_ops.

Changes:

• Reworks fk::TypeList and TypeListCat_t to reduce recursive instantiation (index-based leaf/getter approach).
• Replaces the recursive OperationTuple construction/access with make_new_operation_tuple(...) + get_opt<Idx>(...) and updates fusing/building code paths.
• Updates many tests/examples/includes to the new operation/fusion tuple APIs and new memory_operations.h include location.

Reviewed changes

Copilot reviewed 38 out of 39 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
utests/core/execution_model/utest_executors.h	Updates include path and adjusts expected fused type alias.
utests/algorithm/image_processing/utest_saturate/utest_saturate_common.h	Simplifies saturate expected-value comparison logic.
tests/operation_test_utils.h	Updates read-type detection trait usage.
tests/operation/test_operation_types.h	Updates operation type lists and type-traits used in compile-time tests.
tests/operation/test_operation_tuple.h	Migrates to make_new_operation_tuple / get_opt-style tuple APIs and adds new tuple tests.
tests/operation/test_operation_fuser.h	Updates complex fused-type composition and simplifies test logic.
tests/operation/test_instantiable_operations.h	Updates type assertions for new operation tuple / instantiable-operation shapes.
tests/operation/test_fused_operation.h	Updates assertions to new tuple access and new fused operation type shapes.
tests/operation/test_filtered_index_sequence.h	Updates restriction/type-list usage to new operation types and restriction naming.
tests/examples/readme_test_code.h	Makes several example constants constexpr and updates include path.
tests/examples/inlining_and_LDL_STL.h	Replaces recursive operate() with fold-expression execution; updates pack indexing helper usage and test sizes.
tests/data/test_tuple.h	Updates operation tuple type extraction to new Operations typelist model.
tests/data/basic_test.h	Migrates operation tuple access to get_opt/TypeAt_t and adds more verbose failure output.
tests/buildAPI/batch_build_compilation_time.h	Updates include path and minor formatting.
tests/algorithm/test_warp.h	Updates include path.
tests/algorithm/test_deinterlace.h	Updates include path.
tests/algorithm/test_crop.h	Updates include path.
tests/algorithm/test_border_reader.h	Updates include path.
tests/algorithm/test_batchresize_build.h	Updates include path.
include/fused_kernel/core/utils/vector_utils.h	Updates typelist concatenation usage and minor trait/constexpr robustness tweaks.
include/fused_kernel/core/utils/type_lists.h	Reimplements TypeList indexing/catenation via fold expressions to reduce recursion depth.
include/fused_kernel/core/utils/parameter_pack_utils.h	Renames parameter-pack accessor to get_arg and updates helpers to use it.
include/fused_kernel/core/execution_model/thread_fusion.h	Updates source type list composition and adds explicit cast for elems_per_thread.
include/fused_kernel/core/execution_model/operation_model/parent_operations.h	Adds new parent types (MidWrite/Open/Closed) and macros to support new fused-operation kinds.
include/fused_kernel/core/execution_model/operation_model/operation_types.h	Extends operation “instance types” (Open/Closed) and introduces generalized OpIs.
include/fused_kernel/core/execution_model/operation_model/operation_tuple.h	Replaces recursive operation tuple structure with filtered tuple-of-instances + get_opt + make_new_operation_tuple.
include/fused_kernel/core/execution_model/operation_model/operation_data.h	Extends which instance types are considered “has params”.
include/fused_kernel/core/execution_model/operation_model/iop_fuser.h	Switches non-batch fusing to go through FusedOperation<>::build(...) and tightens type assertions.
include/fused_kernel/core/execution_model/operation_model/instantiable_operations.h	Updates fold-expression piping mechanics and adds Open/Closed instantiable op wrappers.
include/fused_kernel/core/execution_model/operation_model/fused_operation.h	Reimplements fused-operation execution using fold expressions and introduces Open/Closed semantics.
include/fused_kernel/core/execution_model/executors.h	Updates include path and parameter-pack access helper usage.
include/fused_kernel/core/execution_model/data_parallel_patterns.h	Updates parameter-pack access helper usage.
include/fused_kernel/core/data/tuple.h	Small reference/formatting adjustments for tuple get.
include/fused_kernel/core/data/rawptr.h	Zero-initializes dims members to avoid uninitialized state.
include/fused_kernel/core/data/ptr_nd.h	Value-initializes internal members to avoid uninitialized state.
include/fused_kernel/core/data/circular_tensor.h	Updates include path.
include/fused_kernel/algorithms/image_processing/resize.h	Updates include path.
include/fused_kernel/algorithms/image_processing/deinterlace.h	Updates include path.
include/fused_kernel/algorithms/basic_ops/memory_operations.h	Removes NVRTC guard around <vector> include and updates header content accordingly.

Comments suppressed due to low confidence (1)

include/fused_kernel/algorithms/basic_ops/memory_operations.h:24

• is now included unconditionally. This header previously avoided including the full STL when NVRTC_COMPILER is defined (forward-declaring std::vector instead), which is important because NVRTC typically can’t parse many standard library headers. Reintroduce the NVRTC_COMPILER guard (or otherwise ensure NVRTC builds still work) before including .

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This adds a custom std::type_identity in namespace std when compiling pre-C++20. Adding user-defined names to namespace std is undefined behavior, and some C++17 standard libraries already provide std::type_identity as an extension (which would cause a redefinition error). Prefer defining a fk::type_identity (or detail::type_identity) and using that, or gate the definition on a feature-test macro like __cpp_lib_type_identity.

[morousg]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

DECLARE_OPEN_PARENT defines exec(...) as void, but OpenType operations are expected to return an OutputType (they feed into further computation and fold expressions). The void wrapper will break OpenInstantiableOperation operator| and any OpenType-based fusion. Change the wrapper to return OutputType and forward Parent::exec's return value.

[morousg]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

Pull request overview

Copilot reviewed 38 out of 39 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

utests/algorithm/image_processing/utest_saturate/utest_saturate_common.h:32

• cxp::cmp_greater::f(input, ...) returns a vector-of-bool when input is a CUDA vector type (used by addOneTestAllChannels), which is not usable as the if condition here and can fail to compile or behave incorrectly. The previous fk::get<0>(input) ensured a scalar comparison; consider restoring that (or explicitly comparing a scalar channel / reducing the boolean vector) so this works for both scalar and vector inputs.
  include/fused_kernel/algorithms/basic_ops/memory_operations.h:24
• This header now unconditionally includes <vector> and uses std::vector in APIs like SplitWrite::build. Previously it was guarded for NVRTC_COMPILER, presumably because NVRTC builds don't have full libstdc++/libc++ support. Unconditional inclusion can break NVRTC compilation; consider restoring the #if !defined(NVRTC_COMPILER) guard (or moving std::vector-based helpers behind a host-only guard) so device/NVRTC builds don't depend on <vector>.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Defining std::type_identity inside namespace std is undefined behavior (user code must not add new declarations to std, even conditionally). To avoid UB while keeping C++17 compatibility, define an internal fk::detail::type_identity (or a type_identity_t alias) and use that instead of injecting into std.

[morousg]

@copilot open a new pull request to apply changes based on this feedback

[Copilot]

Pull request overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

include/fused_kernel/algorithms/basic_ops/memory_operations.h:24

• memory_operations.h now unconditionally includes <vector>. This header previously had NVRTC-specific handling, and the repo still uses NVRTC_COMPILER guards elsewhere. NVRTC typically cannot include the full STL headers, so this risks breaking NVRTC compilation. Consider restoring the #if !defined(NVRTC_COMPILER) include guard and/or using a forward declaration strategy for std::vector when NVRTC_COMPILER is defined.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

include/fused_kernel/algorithms/basic_ops/memory_operations.h:23

• memory_operations.h now unconditionally includes <vector>, but this header is consumed in NVRTC builds elsewhere in the project (there were previously NVRTC-specific guards here). If NVRTC compilation is still a supported configuration, this can break NVRTC because <vector> (and related parts of the standard library) are often unavailable/unsupported there. Consider restoring the #if !defined(NVRTC_COMPILER) conditional include (and the forward-declaration fallback) or moving the std::vector-dependent APIs behind an NVRTC guard.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The GPU specialization guard for TransformDPP<ParArch::GPU_NVIDIA,...> was narrowed to #if defined(__NVCC__) || CLANG_HOST_DEVICE, but surrounding comments (and another #endif comment later in the file) still reference __NVRTC__ / __HIPCC__. If NVRTC or HIP builds are still supported, this guard change will exclude them from the GPU implementation and likely break compilation/behavior. Please either restore the previous condition (including NVRTC/HIP macros as appropriate) or update the stale comments and build matrix to reflect an intentional support drop.

[Copilot]

The conditional inside TransformDPP<ParArch::GPU_NVIDIA>::exec was changed from #if VS2017_COMPILER || CLANG_HOST_DEVICE to #if CLANG_HOST_DEVICE. On VS2017 + nvcc, CLANG_HOST_DEVICE is false and VS2017_COMPILER is true, but cooperative_groups.h (and the cg alias) are explicitly not included for VS2017 at the top of this file. That means the #else branch using cg::thread_block will fail to compile on VS2017. Please restore the VS2017 condition here (or ensure cg is available in that configuration).

[Copilot]

Pull request overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (2)

include/fused_kernel/algorithms/basic_ops/memory_operations.h:36

• The return type declaration has changed from explicitly specifying the ThreadFusionType to using trailing return type syntax with auto. While this is cleaner, ensure that template argument deduction works correctly, especially for the ELEMS_PER_THREAD template parameter which has a default value of 1.
  include/fused_kernel/algorithms/basic_ops/memory_operations.h:75
• The ReadOp and WriteOp build methods now use perfect forwarding with PtrType&& and std::forward. However, the template instantiation uses std::decay_t<PtrType> to extract the type information. This is correct, but verify that the forwarding doesn't cause issues when the ptr is an lvalue reference, as the build method may be called with both lvalues and rvalues.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The FusedOperation implementation has been completely rewritten using fold expressions instead of recursive template instantiation. The specializations have changed from checking the first/last operation types to using std::enable_if_t with type predicates. While this should reduce compilation times, the logic for determining the FusedOperation type (OpenType, ReadType, UnaryType, ClosedType, WriteType, BinaryType) needs careful verification. Specifically, verify that the specialization priority and std::enable_if_t conditions don't create ambiguous matches or miss valid combinations.

[Copilot]

Similarly, the copyright uses "Mediaproduccion S.L.U." instead of the "Grup Mediapro S.L.U." used in other updated files. Ensure consistent naming of the legal entity across all copyright notices.

[Copilot]

The OperationTuple structure has been completely refactored from a recursive linked-list style to a flat structure with filtered indices. The old .next member access pattern has been replaced with get_opt<Index>(). This is a major breaking API change. While the tests have been updated, any external code using OperationTuple will need significant refactoring. Consider providing a migration guide or compatibility layer.

Suggested change

	using OperationTuple = OperationTuple_<void, Operations...>;
	using FlatOperationTuple = OperationTuple_<void, Operations...>;
	/**
	* @brief Operation tuple for execution model operations.
	*
	* Migration note:
	* The OperationTuple structure has been refactored from a recursive,
	* linked-list-style representation (accessed via `.next`) to a flat
	* representation with filtered indices.
	*
	* Code that previously relied on patterns such as:
	*
	* auto &first = ops.operation;
	* auto &second = ops.next.operation;
	* auto &third = ops.next.next.operation;
	*
	* must be updated to use index-based accessors on the flat tuple,
	* for example `get_opt<Index>()` (and/or helpers defined in this
	* header) instead of chaining `.next`.
	*
	* The FlatOperationTuple alias names the new, flat implementation
	* explicitly:
	*
	* using Ops = fk::FlatOperationTuple<Op0, Op1, Op2>;
	*
	* while OperationTuple is kept as the primary alias for backward
	* compatibility with existing code that only depends on the type
	* name, not on the old recursive layout.
	*/
	template <typename... Operations>
	using OperationTuple = FlatOperationTuple<Operations...>;

[Copilot]

The VS2017_COMPILER macro check has been removed and only CLANG_HOST_DEVICE is checked. If VS2017 support is being dropped, this should be documented. If VS2017 should still be supported but behaves like Clang in this context, that assumption should be verified.

[Copilot]

The comprehensive tests for deeply nested fused operations (Tests 1-18) are excellent additions that verify the fold expression implementation works correctly at various nesting depths. However, these tests only verify type correctness via static_assert. Consider adding runtime tests that actually execute these operations with sample data to ensure the execution logic is correct, not just the type deduction.

[Copilot]

The Point constructors have been removed, requiring aggregate initialization with braced-init-lists. However, the z coordinate is being set to 0 in many places throughout the codebase. This pattern should be documented or a helper function should be provided to create 2D Points more ergonomically, as Point{x, y, 0} is verbose and error-prone.

Suggested change

	// Helper to ergonomically construct 2D points represented in 3D space
	// by fixing the z coordinate to zero. This centralises the common
	// pattern `Point_{x, y, 0}` in a single, type-safe place.
	template <typename T>
	[[nodiscard]] constexpr Point_<T, ND::_3D> make_point2d(T x, T y) {
	return Point_<T, ND::_3D>{x, y, T{}};
	}

[Copilot]

The exec function implementations in the macros have been changed to call exec(...) directly instead of calling through the Parent. This recursive call pattern could lead to infinite recursion if not properly overloaded. Verify that all Operation types that use these macros provide their own exec implementations that don't rely on the parent's exec.

[Copilot]

The copyright line lists "Mediaproduccion S.L.U." but other files in this PR use "Grup Mediapro S.L.U.". Verify which is the correct legal entity name and ensure consistency across all copyright headers.

[Copilot]

The z dimension initialization comment states "z is 0 because we do not use it going forward, and batch is handled before calling this exec". However, this is an implementation detail that may not be obvious. Consider documenting this more clearly in the function signature or with a more prominent comment explaining why z is always 0 in this context.

Suggested change

	// z is 0 because we do not use it going forward, and batch is handled before calling this exec
	// Note: ResizeComplete operates on a single 2D image slice per invocation.
	// The z dimension is not used by exec_resize (see num_elems_z(), which always returns 1),
	// and any batching or z-dimension iteration is handled by the caller before invoking exec().
	// For this reason we explicitly construct a Point with z = 0 here, after translating x/y
	// into the coordinate system of the cropped destination region.

[Copilot]

Pull request overview

Copilot reviewed 56 out of 56 changed files in this pull request and generated 10 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The TypeListCat implementation now uses a fold expression over operator>> which is defined as a friend function that returns TypeList<Ts..., Us...>. However, the operator>> is declared but not defined in the TypeList class. The implementation relies on template argument deduction from a decltype expression in TypeListCat, which evaluates std::declval() >> .... This should work, but verify that the operator>> overload resolution is unambiguous and correctly deduces the result type.

[Copilot]

The OperationTuple refactoring changes from a recursive linked-list structure (with instance and next members) to a flat structure with an instances Tuple member. The get_opt function now uses GetIndex to map from the operation index to the instance index, filtering out Unary operations. However, the implementation is complex and may have edge cases. Specifically, the GetIndexHelper uses a fold expression with side effects in the lambda, which relies on left-to-right evaluation order. While C++17 guarantees this for fold expressions, the logic should be carefully verified for correctness.

[Copilot]

The change in BATCH size from 10 to 100 on line 466 significantly increases the test workload (10x increase). While this may be intentional to provide more thorough testing or benchmarking, it could significantly increase test execution time. Consider whether this increase is necessary or if it should be configurable.

[Copilot]

The Array struct changes its operator[] parameter type from const size_t& index to const int index and then adds separate overloads for both int and size_t. This could cause ambiguity issues when calling with unsigned integer types other than size_t. The const and non-const versions are also inconsistent in their parameter types (int vs size_t). Consider consolidating to a single template operator[] that accepts any integral type, or ensuring consistent parameter types across all overloads.

[Copilot]

The operator| overloads for UnaryInstantiableOperation, BinaryInstantiableOperation, and TernaryInstantiableOperation now have two versions: one that works with InputFoldType for fold expressions, and one that works directly with the input type. The second version (e.g., lines 148-150, 209-212) returns the OutputType directly without wrapping in InputFoldType. This dual behavior should be carefully documented as it affects how these operations can be composed. The non-fold version is simpler but can't be used in the middle of a fold expression chain.

[Copilot]

The exec() functions in the DECLARE_*_PARENT macros now call exec() with unpacked parameters (e.g., exec(input, opData.params)) instead of calling Parent::exec(input, opData). This changes the function being called from a qualified call to an unqualified call, which will use ADL (Argument-Dependent Lookup) and may resolve to a different overload. This could be a subtle bug if there are multiple exec() overloads in scope. Consider making these calls explicitly qualified (e.g., Child::exec() or using CRTP pattern) to avoid ambiguity.

[Copilot]

The removal of VS2017_COMPILER checks and replacement with just CLANG_HOST_DEVICE on lines 246 and 344 indicates dropping support for Visual Studio 2017. This should be documented in the PR description or release notes as it affects supported compilers.

[Copilot]

The OpenInstantiableOperation and ClosedInstantiableOperation classes are newly added (lines 249-284). These represent new operation types in the framework. However:

1. OpenInstantiableOperation supports fusion via operator& but ClosedInstantiableOperation does not
2. ClosedInstantiableOperation only supports operator| with Point, effectively making it a terminal operation
3. The asymmetry between these two (Open can fuse, Closed cannot) should be documented

Verify that this design is intentional and that use cases requiring fusion of Closed operations are properly handled or documented as unsupported.

[Copilot]

The change from using Point(x, y, z) constructor syntax to Point{x, y, z} aggregate initialization is consistent throughout the codebase. However, this requires that Point no longer has user-defined constructors (which was removed in point.h). While this is consistent, it's a breaking API change. All uses of Point construction now must use brace-initialization syntax. This could break external code that uses the old constructor syntax.

[Copilot]

The new InputFoldType specialization (lines 72-77) provides a build() function that constructs an InputFoldType with the concrete input type. However, this creates the type on-the-fly during fold expression evaluation. The original InputFoldType had a constructor that took only a Point. The new design changes how the type is used in fold expressions. Verify that all uses of operator| with Point correctly work with this new build pattern and that the automatic type deduction doesn't cause issues in complex expressions.

Suggested change

	FK_HOST_DEVICE_FUSE auto build(const Point& thread, InputT&& input) {
	FK_HOST_DEVICE_FUSE static auto build(const Point& thread, InputT&& input) {

[Copilot]

Pull request overview

Copilot reviewed 56 out of 56 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 56 out of 56 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.