Windowz#13
Conversation
📝 WalkthroughWalkthroughThis PR overhauls the AVX subsystem (types, span, array, vector), introduces a function-call Counter, replaces the old Type system with a specialization-based implementation, updates public headers/tests to new aliases (Vector_t, make_spans), adds Windows CI, and introduces new benchmark tests and profiles. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inc/mkn/avx/array.hpp (1)
35-37: Duplicate include of span.hpp.Line 37 includes
"span.hpp"which is redundant with line 35's"mkn/avx/span.hpp". Remove the duplicate.🔧 Suggested fix
#include "mkn/avx/def.hpp" #include "mkn/avx/span.hpp" -#include "span.hpp" - #include <array>
🤖 Fix all issues with AI agents
In @inc/mkn/avx/dbg.hpp:
- Around line 72-74: The macro MKN_AVX_FN_COUNTER uses token pasting with
__LINE__ (as in __mkn_avx_count_point##__LINE__) which prevents __LINE__ from
expanding; replace it with a two-step helper macro (e.g., a JOIN/EXPAND pair) so
the line number is expanded before concatenation, then use that helper to
produce the unique identifier for CountPoint and to call Counter::I() with the
correct file/func/line parameters; update the macro definition that creates
__mkn_avx_count_point##__LINE__ to use the helper so CountPoint and Counter::I()
get the actual line number.
In @inc/mkn/avx/url.hpp:
- Around line 1-4: The header currently forces MKN_AVX_FN_COUNTER to be defined
empty, which masks MKN_AVX_COUNT_FNS logic in dbg.hpp; remove the conditional
macro definition for MKN_AVX_FN_COUNTER from this header so dbg.hpp can control
it (or alternatively ensure this header is included only after dbg.hpp); verify
that dbg.hpp still sets MKN_AVX_FN_COUNTER when MKN_AVX_COUNT_FNS is defined and
that no other headers reintroduce the empty definition.
- Around line 48-49: The two operator[] overloads have inconsistent semantics:
the non-const operator[] uses reinterpret_cast<T*>(&array) while the const
operator[] indexes array directly; make the const overload mirror the non-const
behavior by indexing through a pointer-to-const T obtained from
reinterpret_cast<const T*>(&array) so both versions return the same element view
and maintain noexcept and const correctness (update the const operator[]
implementation in the class to use reinterpret_cast to a const T pointer and
index it).
In @test/betterbench_avx.cpp:
- Around line 58-60: Replace the throw with a non-zero process exit and remove
the stray semicolon: where the code currently does `if (d3[arrs - 1][3] != b3)
throw std::runtime_error("FAIL!");` change it to return a non-zero value (e.g.
`return EXIT_FAILURE;` or `return 1;`) so the program exits cleanly with failure
semantics (add `#include <cstdlib>` if using `EXIT_FAILURE`), and delete the
trailing semicolon after the closing brace to remove the extraneous token.
🧹 Nitpick comments (4)
inc/mkn/avx/array.hpp (1)
102-106: Consider removing commented-out code.The commented-out assignment operator adds noise. If it's not planned for future use, consider removing it entirely.
.github/workflows/build_win.yml (1)
30-30: Consider aligning KLOG level with Linux CI and verify -fPIC relevance on Windows.Two minor observations:
KLOG level inconsistency: The Windows workflow uses
KLOG=3while the Linux workflow usesKLOG=5(line 21 in build_nix.yml). Consider usingKLOG=5here as well for consistent verbose logging across platforms, unless lower verbosity on Windows is intentional.-fPIC flag on Windows: The
-fPICflag is typically a Unix/GCC concept for position-independent code. MSVC may ignore it or produce warnings. While likely harmless, you might want to verify it's necessary or consider platform-specific flag handling.test/betterbench_avx.cpp (1)
25-26: Unusedprefetcherlambda.The
prefetcherlambda is defined but never invoked in this benchmark. Consider removing it or adding the prefetch calls if they were intended to be part of the benchmark loop.inc/mkn/avx/types.hpp (1)
34-37: Misleading comment.The comment "one file url include no deps" on line 37 is inaccurate—
url.hppincludes<immintrin.h>,<cstdint>, and<utility>. Consider clarifying or removing this comment.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/build_nix.yml.github/workflows/build_win.ymlREADME.noformatinc/mkn/avx.hppinc/mkn/avx/array.hppinc/mkn/avx/dbg.hppinc/mkn/avx/def.hppinc/mkn/avx/lazy.hppinc/mkn/avx/span.hppinc/mkn/avx/types.hppinc/mkn/avx/url.hppinc/mkn/avx/vector.hppmkn.yamltest/bench.cpptest/betterbench.cpptest/betterbench.hpptest/betterbench_avx.cpptest/test_avx.cpptest/test_lazy.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: mkn/mkn.avx PR: 11
File: inc/mkn/avx/array.hpp:185-190
Timestamp: 2025-03-16T19:40:42.661Z
Learning: In the mkn.avx library, separate type parameters (T0, T1) are used in template functions to handle const/non-const type variations rather than for actual different types. If the types are incompatible, the code will fail to compile.
📚 Learning: 2025-03-16T19:40:42.661Z
Learnt from: PhilipDeegan
Repo: mkn/mkn.avx PR: 11
File: inc/mkn/avx/array.hpp:185-190
Timestamp: 2025-03-16T19:40:42.661Z
Learning: In the mkn.avx library, separate type parameters (T0, T1) are used in template functions to handle const/non-const type variations rather than for actual different types. If the types are incompatible, the code will fail to compile.
Applied to files:
inc/mkn/avx/lazy.hppinc/mkn/avx/types.hppinc/mkn/avx/vector.hpptest/bench.cppinc/mkn/avx/def.hppinc/mkn/avx.hpptest/betterbench.cpptest/betterbench.hpptest/betterbench_avx.cpptest/test_avx.cppinc/mkn/avx/url.hppinc/mkn/avx/span.hppinc/mkn/avx/array.hpp
📚 Learning: 2025-03-16T14:42:12.355Z
Learnt from: PhilipDeegan
Repo: mkn/mkn.avx PR: 11
File: inc/mkn/avx/array.hpp:93-96
Timestamp: 2025-03-16T14:42:12.355Z
Learning: In the mkn::avx Array class, the move constructor is implemented as a call to the copy constructor because the base class _A_ has a deleted move constructor. This design choice was made to ensure the class can be returned from functions while maintaining the constraint that the underlying array cannot be moved.
Applied to files:
inc/mkn/avx/types.hppinc/mkn/avx/vector.hppinc/mkn/avx.hpptest/betterbench.hpptest/test_avx.cppinc/mkn/avx/url.hppinc/mkn/avx/span.hppinc/mkn/avx/array.hpp
🧬 Code graph analysis (5)
test/bench.cpp (3)
test/betterbench.hpp (1)
v0(39-39)test/test_avx.cpp (1)
v0(70-70)inc/mkn/avx/span.hpp (6)
make_spans(561-564)make_spans(561-561)make_spans(568-571)make_spans(568-568)make_spans(575-578)make_spans(575-575)
inc/mkn/avx.hpp (1)
inc/mkn/avx/span.hpp (6)
make_spans(561-564)make_spans(561-561)make_spans(568-571)make_spans(568-568)make_spans(575-578)make_spans(575-575)
test/betterbench.cpp (1)
test/betterbench.hpp (2)
op(68-87)op(68-68)
test/betterbench_avx.cpp (1)
test/betterbench.hpp (4)
op(68-87)op(68-68)KUL_DBG_FUNC_ENTER(90-112)KUL_DBG_FUNC_ENTER(115-134)
inc/mkn/avx/array.hpp (2)
inc/mkn/avx/def.hpp (2)
N(79-94)N(79-79)inc/mkn/avx/span.hpp (41)
N(74-74)N(250-250)arr(72-72)arr(73-73)arr(157-162)arr(157-157)arr(174-179)arr(174-174)arr(190-195)arr(190-190)arr(396-402)arr(396-396)arr(420-426)arr(420-420)arr(439-445)arr(439-439)arr(462-468)arr(462-462)val(403-407)val(403-403)val(446-450)val(446-446)val(469-473)val(469-469)make_span(522-528)make_span(522-522)make_span(530-536)make_span(530-530)make_span(539-542)make_span(539-539)make_span(544-547)make_span(544-544)make_span(550-553)make_span(550-550)make_span(555-558)make_span(555-555)span(248-248)span(249-249)span(255-255)span(256-256)span(504-504)
🪛 Cppcheck (2.19.0)
test/betterbench.cpp
[error] 32-32: Unhandled exception thrown in function that is an entry point.
(throwInEntryPoint)
test/betterbench_avx.cpp
[warning] 48-48: The address of variable 'array' might be accessed at non-zero index.
(objectIndex)
[error] 59-59: Unhandled exception thrown in function that is an entry point.
(throwInEntryPoint)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (35)
test/test_lazy.cpp (2)
16-16: LGTM on the assert migration.The switch from
abort_if_notto standardassertis appropriate for test code. Note thatassertis disabled whenNDEBUGis defined, so ensure test builds don't defineNDEBUG.Also applies to: 27-27, 39-39, 48-48, 60-60, 70-70, 79-79, 94-94, 105-105
108-119: Diagnostic output and test structure look good.The
__FILE__output aids debugging test failures. The test suite covers add, mul, and FMA operations comprehensively.inc/mkn/avx/lazy.hpp (1)
354-366: Unused parameter documentation is appropriate.Commenting out the parameter name
/*in_place*/correctly suppresses unused parameter warnings while preserving the API signature for future implementation.inc/mkn/avx/def.hpp (2)
81-83: Early return pattern with preprocessor is correct but may confuse static analyzers.The preprocessor-based early return works correctly. Consider adding a comment noting that the remaining code is the fallback path when
MKN_AVX_FORCE_Nis not defined.
96-110: NewALIGN()function looks good.The alignment computation correctly maps AVX levels to appropriate alignment values (64/32/16/8). The
MKN_AVX_ALIGN_ASoverride provides useful flexibility for non-standard configurations.inc/mkn/avx/array.hpp (3)
71-82:std::nullopt_tconstructor for uninitialized arrays is a nice addition.Using
std::nullopt_tas a tag type to signal no-initialization is an elegant pattern. The default constructor now initializes to 0 via the Span assignment operator, which is clear and safe.
84-95: Move constructor copies data as expected.Based on learnings, the move constructor intentionally copies data because the underlying
_A_base cannot be moved. The**this = *thatpattern cleanly delegates to Span-based assignment.
204-229: Free operator overloads for Span arithmetic look correct.The operators properly create uninitialized arrays with
std::nullopt, then perform the operations. Using__restricthints to the compiler that the spans don't alias.README.noformat (1)
23-31: Documentation accurately reflects the new alignment behavior.The updated documentation correctly describes the conditional alignment defaults based on AVX level, matching the implementation in
Options::ALIGN().mkn.yaml (1)
17-32: Benchmark profiles restructured for AVX configuration testing.Splitting into
b0andb1profiles enables benchmarking with different AVX configurations (-DMKN_AVX_BENCH_AVX_N). Thegbenchprofile is preserved for Google Benchmark integration.inc/mkn/avx/vector.hpp (1)
42-43: Clean simplification to a type alias.Replacing the complex
Vectorclass with a simple alias tostd::vectorwith an aligned allocator is a good design choice. This leverages standard library functionality while ensuring proper AVX alignment viaOptions::ALIGN(). All usages of the oldVectorclass have been successfully migrated toVector_t, with no remaining legacy patterns in the codebase..github/workflows/build_nix.yml (1)
21-21: LGTM! Enhanced testing configuration.The changes improve CI diagnostics and test coverage by increasing KLOG verbosity, adding KUL tracing, and expanding test targets to include b0 and b1 profiles.
.github/workflows/build_win.yml (2)
3-5: LGTM! Proper concurrency configuration.The concurrency cancellation prevents redundant builds when new commits are pushed to the same PR or branch.
19-21: LGTM! Standard MSVC environment setup.Using the ilammy/msvc-dev-cmd action with amd64 architecture is the correct approach for Windows C++ builds.
test/betterbench.cpp (4)
9-13: LGTM! Flexible type configuration.The conditional compilation allows the benchmark to run with either AVX-optimized Array types or plain scalar types, enabling performance comparison.
23-25: LGTM! Proper baseline establishment.Computing the baseline value once and logging it provides a reference for validating the SoA and AoS implementations.
27-33: LGTM! Proper SoA validation.The test correctly validates the SoA implementation against the baseline. The exception thrown on failure is appropriate for test code, despite the static analysis warning. Test programs commonly throw or abort on failure to signal the test harness.
34-40: LGTM! Proper AoS validation.The AoS test mirrors the SoA validation pattern, ensuring both data layout implementations produce correct results.
test/bench.cpp (3)
97-102: LGTM! Systematic API migration to Vector_t and spans.The refactoring from
Vector<T>toVector_t<T>with span-based operations is clean and consistent. The structured binding patternauto [a, b, c] = mkn::avx::make_spans(v0, v1, v2)improves ergonomics and aligns with the broader architectural changes in this PR.
5-5: LGTM! Include organization improvements.The include reordering and addition of standard headers (
<cmath>,<array>,<cassert>) improve clarity and ensure all dependencies are explicitly listed.Also applies to: 10-12
196-200: Clarify the status of uint32_t support and either enable or remove the deferred benchmarks.The commented uint32_t benchmarks (lines 196-200) correspond to a commented-out
Type_<std::uint32_t, 8>specialization ininc/mkn/avx/url.hpp(lines 194-201). WhileType_<std::int32_t, 8>is active and float/double benchmarks are enabled, uint32_t support appears intentionally deferred. Either complete the uint32_t implementation with a clear tracking issue or remove the commented code to avoid confusion.inc/mkn/avx.hpp (2)
35-35: LGTM! Essential header addition.Including
mkn/avx/span.hppexposes the span utilities to library users, enabling the new span-based API pattern demonstrated in the sample code.
51-52: LGTM! Clear demonstration of the new API.The updated sample code effectively demonstrates the Vector_t and make_spans pattern, providing users with a clear usage example of the refactored API.
test/test_avx.cpp (4)
3-6: LGTM!The include reordering and additions (
mkn/kul/log.hpp,mkn/avx/vector.hpp,<cmath>) align with the new API surface and support the Counter logging added inmain().
17-37: LGTM!The transition from
abort_if_notto standardassertis cleaner. The expanded test data witht2{1}and combined returnt1 + t2correctly validates the copy/move semantics ofArray_t.
45-66: LGTM!The
Vecalias updates tomkn::avx::Vector_t<T>correctly follow the new API convention established in this refactor.
132-141: VerifyKLOGexception safety innoexceptcontext.
main()is markednoexcept, butKLOG(INF)may potentially throw (e.g., during stream operations or formatting failures). IfKLOGcan throw, this would causestd::terminate. Verify that themkn::kullogging implementation is exception-safe or markednoexceptbefore using it in this context.test/betterbench_avx.cpp (1)
28-48: LGTM!The
oplambda correctly demonstrates the newAVX_tfunction-pointer-based API for vectorized arithmetic operations. The structured binding withstd::forward_as_tupleis idiomatic.The static analysis warning about array index access appears to be a false positive related to how structured bindings interact with tuple references.
inc/mkn/avx/dbg.hpp (1)
43-62:Counteris not thread-safe.The singleton
Countermodifiescntswithout synchronization. If the library is used in multi-threaded code withMKN_AVX_COUNT_FNSenabled, this will result in data races.Consider adding a mutex or documenting that function counting is not thread-safe, or using
std::atomic<std::size_t>values if approximate counts are acceptable.test/betterbench.hpp (1)
33-43: LGTM!The
SoAstructure correctly initializes fourVec<V>members with the same size, and theoperator bool()check validates the initialization. The default size of 10 is suitable for a test harness.inc/mkn/avx/url.hpp (1)
149-190: Incomplete integer type specializations.The
Type_<std::int16_t, ...>andType_<std::int32_t, ...>specializations are missing several function pointers (mul_func_ptr,div_func_ptr,set_func_ptr,set_v_func_ptrfor int16_t;sub_func_ptr,div_func_ptr,set_func_ptr,set_v_func_ptrfor int32_t).Code using these types with missing operations will fail to compile. Consider either completing these specializations or adding
static_assertwith clear error messages.inc/mkn/avx/span.hpp (4)
67-80: Union type punning between pointer types.The union
Uholds bothT* arrandmkn::avx::Type<R, N>* i, andavx()provides access to theimember. Reading from a different union member than was written is implementation-defined in C++ (unlike C where it's well-defined).This pattern works in practice for SIMD aliasing but relies on
Type<R, N>having compatible layout withT[N]. Ensure this invariant is documented and tested.
94-104: LGTM!The copy and move constructors both perform shallow copies, which is correct for a non-owning span type. The move constructor intentionally behaves like a copy since there's no ownership to transfer.
560-578: LGTM!The
make_spansoverloads provide convenient tuple construction for multiple spans. The use ofcontainers.data()correctly extracts the underlying data pointers while the forwarding reference syntax enables both lvalue and rvalue container arguments.
211-228: LGTM!The templated
operator=correctly branches onmkn::kul::is_span_like_v<T0>to handle both span-like and scalar assignments. Deleting the rvalue assignment operator prevents dangling references from temporary spans.
| #define MKN_AVX_FN_COUNTER \ | ||
| static mkn::avx::CountPoint __mkn_avx_count_point##__LINE__{__FILE__, __func__, __LINE__}; \ | ||
| mkn::avx::Counter::I()(__FILE__, __func__, __LINE__); |
There was a problem hiding this comment.
Macro __LINE__ token pasting issue.
The ##__LINE__ concatenation won't expand __LINE__ to the actual line number. The preprocessor requires indirection through a helper macro for proper expansion.
Proposed fix
+#define MKN_AVX_CONCAT_(a, b) a##b
+#define MKN_AVX_CONCAT(a, b) MKN_AVX_CONCAT_(a, b)
+
#if defined(MKN_AVX_COUNT_FNS)
#define MKN_AVX_FN_COUNTER \
- static mkn::avx::CountPoint __mkn_avx_count_point##__LINE__{__FILE__, __func__, __LINE__}; \
+ static mkn::avx::CountPoint MKN_AVX_CONCAT(__mkn_avx_count_point, __LINE__){__FILE__, __func__, __LINE__}; \
mkn::avx::Counter::I()(__FILE__, __func__, __LINE__);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @inc/mkn/avx/dbg.hpp around lines 72 - 74, The macro MKN_AVX_FN_COUNTER uses
token pasting with __LINE__ (as in __mkn_avx_count_point##__LINE__) which
prevents __LINE__ from expanding; replace it with a two-step helper macro (e.g.,
a JOIN/EXPAND pair) so the line number is expanded before concatenation, then
use that helper to produce the unique identifier for CountPoint and to call
Counter::I() with the correct file/func/line parameters; update the macro
definition that creates __mkn_avx_count_point##__LINE__ to use the helper so
CountPoint and Counter::I() get the actual line number.
|
|
||
| #if !defined(MKN_AVX_FN_COUNTER) | ||
| #define MKN_AVX_FN_COUNTER // optionally defined in dbg.hpp | ||
| #endif |
There was a problem hiding this comment.
Macro definition order issue.
Defining MKN_AVX_FN_COUNTER as empty before including dbg.hpp will prevent the counter from being enabled even when MKN_AVX_COUNT_FNS is defined, because the #if !defined check on line 2 will always succeed when this header is included first.
This header should be included after dbg.hpp, or the macro definition should be removed here.
Proposed fix
-
-#if !defined(MKN_AVX_FN_COUNTER)
-#define MKN_AVX_FN_COUNTER // optionally defined in dbg.hpp
-#endif
+#include "mkn/avx/dbg.hpp"
#include <cstdint>
#include <utility>
#include <immintrin.h> // avx🤖 Prompt for AI Agents
In @inc/mkn/avx/url.hpp around lines 1 - 4, The header currently forces
MKN_AVX_FN_COUNTER to be defined empty, which masks MKN_AVX_COUNT_FNS logic in
dbg.hpp; remove the conditional macro definition for MKN_AVX_FN_COUNTER from
this header so dbg.hpp can control it (or alternatively ensure this header is
included only after dbg.hpp); verify that dbg.hpp still sets MKN_AVX_FN_COUNTER
when MKN_AVX_COUNT_FNS is defined and that no other headers reintroduce the
empty definition.
| template<typename T, std::size_t SIZE> | ||
| void inline store(T const* __restrict a, Type<T, SIZE> const& __restrict b) noexcept | ||
| { | ||
| MKN_AVX_FN_COUNTER; | ||
| Type<T, SIZE>::set_func_ptr(const_cast<T*>(a), b()); | ||
| } |
There was a problem hiding this comment.
const_cast to write through const pointer.
The store function takes T const* but immediately casts away const to write. This is misleading and potentially undefined behavior if the caller passes a pointer to an actual const object.
Proposed fix
template<typename T, std::size_t SIZE>
-void inline store(T const* __restrict a, Type<T, SIZE> const& __restrict b) noexcept
+void inline store(T* __restrict a, Type<T, SIZE> const& __restrict b) noexcept
{
MKN_AVX_FN_COUNTER;
- Type<T, SIZE>::set_func_ptr(const_cast<T*>(a), b());
+ Type<T, SIZE>::set_func_ptr(a, b());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template<typename T, std::size_t SIZE> | |
| void inline store(T const* __restrict a, Type<T, SIZE> const& __restrict b) noexcept | |
| { | |
| MKN_AVX_FN_COUNTER; | |
| Type<T, SIZE>::set_func_ptr(const_cast<T*>(a), b()); | |
| } | |
| template<typename T, std::size_t SIZE> | |
| void inline store(T* __restrict a, Type<T, SIZE> const& __restrict b) noexcept | |
| { | |
| MKN_AVX_FN_COUNTER; | |
| Type<T, SIZE>::set_func_ptr(a, b()); | |
| } |
| if (d3[arrs - 1][3] != b3) | ||
| throw std::runtime_error("FAIL!"); | ||
| }; |
There was a problem hiding this comment.
Consider returning a non-zero exit code instead of throwing.
Static analysis correctly flags that throwing std::runtime_error in main() results in an unhandled exception, leading to std::terminate. For a benchmark/test executable, returning a non-zero exit code provides cleaner failure semantics.
Proposed fix
if (d3[arrs - 1][3] != b3)
- throw std::runtime_error("FAIL!");
-};
+ {
+ std::cerr << "FAIL!" << std::endl;
+ return 1;
+ }
+ return 0;
+}Also note the trailing semicolon after the closing brace on line 60, which should be removed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (d3[arrs - 1][3] != b3) | |
| throw std::runtime_error("FAIL!"); | |
| }; | |
| if (d3[arrs - 1][3] != b3) | |
| { | |
| std::cerr << "FAIL!" << std::endl; | |
| return 1; | |
| } | |
| return 0; | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 59-59: Unhandled exception thrown in function that is an entry point.
(throwInEntryPoint)
🤖 Prompt for AI Agents
In @test/betterbench_avx.cpp around lines 58 - 60, Replace the throw with a
non-zero process exit and remove the stray semicolon: where the code currently
does `if (d3[arrs - 1][3] != b3) throw std::runtime_error("FAIL!");` change it
to return a non-zero value (e.g. `return EXIT_FAILURE;` or `return 1;`) so the
program exits cleanly with failure semantics (add `#include <cstdlib>` if using
`EXIT_FAILURE`), and delete the trailing semicolon after the closing brace to
remove the extraneous token.
| else | ||
| { | ||
| auto constexpr static N = mkn::avx::Options::N<T>(); | ||
|
|
||
| // NO AVX FOR AOS | ||
| for (std::size_t i = 0; i < size(); i += N) | ||
| { | ||
| for (std::size_t j = 0; j < N; ++j) | ||
| { | ||
| std::size_t off = i + j; | ||
| op<T>(v.data()[off].v0, v.data()[off].v1, v.data()[off].v2, v.data()[off].v3); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential out-of-bounds access when size() is not divisible by N.
In the else branch, the outer loop increments by N and the inner loop iterates j from 0 to N. If size() is not evenly divisible by N, the last iteration will compute off = i + j where off >= size(), causing out-of-bounds access.
Proposed fix
else
{
auto constexpr static N = mkn::avx::Options::N<T>();
// NO AVX FOR AOS
- for (std::size_t i = 0; i < size(); i += N)
+ for (std::size_t i = 0; i < size(); ++i)
{
- for (std::size_t j = 0; j < N; ++j)
- {
- std::size_t off = i + j;
- op<T>(v.data()[off].v0, v.data()[off].v1, v.data()[off].v2, v.data()[off].v3);
- }
+ op<T>(v.data()[i].v0, v.data()[i].v1, v.data()[i].v2, v.data()[i].v3);
}
}Alternatively, if the intent is to process in blocks of N, add a bounds check or ensure size() is always a multiple of N.
| else | ||
| { | ||
| auto constexpr static N = mkn::avx::Options::N<V>(); | ||
|
|
||
| // AVX!! | ||
| for (std::size_t i = 0; i < size(); i += N) | ||
| { | ||
| auto&& [a0, a1, a2, a3] = mkn::avx::make_spans<N>(i, v0(), v1(), v2(), v3()); | ||
| op<T>(a0, a1, a2, a3); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same out-of-bounds risk when size() is not divisible by N.
The AVX path iterates with step N but doesn't handle the remainder when size() is not evenly divisible by N. This will cause make_spans to read past the buffer.
Proposed fix
else
{
auto constexpr static N = mkn::avx::Options::N<V>();
+ auto const aligned_size = size() - (size() % N);
// AVX!!
- for (std::size_t i = 0; i < size(); i += N)
+ for (std::size_t i = 0; i < aligned_size; i += N)
{
auto&& [a0, a1, a2, a3] = mkn::avx::make_spans<N>(i, v0(), v1(), v2(), v3());
op<T>(a0, a1, a2, a3);
}
+ // Handle remainder with scalar operations
+ for (std::size_t i = aligned_size; i < size(); ++i)
+ op<T>(v0().data()[i], v1().data()[i], v2().data()[i], v3().data()[i]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else | |
| { | |
| auto constexpr static N = mkn::avx::Options::N<V>(); | |
| // AVX!! | |
| for (std::size_t i = 0; i < size(); i += N) | |
| { | |
| auto&& [a0, a1, a2, a3] = mkn::avx::make_spans<N>(i, v0(), v1(), v2(), v3()); | |
| op<T>(a0, a1, a2, a3); | |
| } | |
| } | |
| else | |
| { | |
| auto constexpr static N = mkn::avx::Options::N<V>(); | |
| auto const aligned_size = size() - (size() % N); | |
| // AVX!! | |
| for (std::size_t i = 0; i < aligned_size; i += N) | |
| { | |
| auto&& [a0, a1, a2, a3] = mkn::avx::make_spans<N>(i, v0(), v1(), v2(), v3()); | |
| op<T>(a0, a1, a2, a3); | |
| } | |
| // Handle remainder with scalar operations | |
| for (std::size_t i = aligned_size; i < size(); ++i) | |
| op<T>(v0().data()[i], v1().data()[i], v2().data()[i], v3().data()[i]); | |
| } |
4359fb8 to
1f90485
Compare
|
focus on the compile errors in the actions please @CodeRabbit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 39394 @PhilipDeegan I've identified the compile errors in the Windows build. Here's what's happening: Windows Build Failure AnalysisThe Windows build is failing with linker errors (LNK2019: unresolved external symbol), while the Ubuntu builds succeed. The errors are all related to AVX intrinsics: Unresolved symbols:
Root CauseThe issue stems from static constexpr auto add_func_ptr = _mm256_add_pd;
static constexpr auto mul_func_ptr = _mm256_mul_pd;
// etc.On MSVC, AVX intrinsics are inline pseudo-functions recognized by the compiler and replaced directly with machine code. They are not actual callable functions with linkable symbols. When you take their address (assign to function pointer), MSVC expects a real symbol in a library, which doesn't exist. This pattern works fine on GCC/Clang where intrinsics can be treated as regular functions, but fails on MSVC. Solution Options
Would you like me to investigate the codebase further to suggest a specific fix approach? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @inc/mkn/avx/url.hpp:
- Line 84: The static member assignment for fma (auto const static inline fma)
directly binds to the AVX intrinsic _mm256_fmadd_pd which on MSVC produces
linker errors; change the assignment to match the other intrinsic members by
wrapping the call in a lambda that forwards its arguments to _mm256_fmadd_pd
(same pattern used for the other ops around lines 77-82), so fma becomes an
inline lambda taking the intended parameters and returning the intrinsic result,
avoiding creating a non-linkable symbol.
- Around line 48-49: The const overload of operator[] is inconsistent and
invalid because it tries to subscript the AVX register member `array` directly;
change the const `auto& operator[](std::size_t i) const noexcept` to mirror the
non-const version by returning `reinterpret_cast<const T*>(&array)[i]` (i.e.,
cast `&array` to `const T*` and index it) so both overloads return the same
element type and remain noexcept.
🧹 Nitpick comments (2)
inc/mkn/avx/url.hpp (2)
243-269: Consider consistent access pattern for operations.The arithmetic operators use inconsistent access patterns:
operator+andoperator*useType<T, SIZE>::add/mul(lines 247, 261)operator-andoperator/useType<T, SIZE>::Super::impl_type::sub/div(lines 254, 268)Both approaches work, but consistency would improve maintainability.
♻️ Proposed fix for consistency
Type<T, SIZE> inline operator-(Type<T, SIZE> const& a, Type<T, SIZE> const& b) noexcept { MKN_AVX_FN_COUNTER; - return {Type<T, SIZE>::Super::impl_type::sub(a(), b())}; + return {Type<T, SIZE>::sub(a(), b())}; } template<typename T, std::size_t SIZE> Type<T, SIZE> inline operator/(Type<T, SIZE> const& a, Type<T, SIZE> const& b) noexcept { MKN_AVX_FN_COUNTER; - return {Type<T, SIZE>::Super::impl_type::div(a(), b())}; + return {Type<T, SIZE>::div(a(), b())}; }
315-320: Consider adding MKN_AVX_FN_COUNTER to fma function.The
fmafunction lacks theMKN_AVX_FN_COUNTERinstrumentation present in all arithmetic operators (lines 246, 253, 260, 267, 274, 281, 288, 295, 303, 310). If call counting is desired for profiling, this should be consistent.♻️ Proposed addition
template<typename T, std::size_t SIZE> Type<T, SIZE> inline fma(Type<T, SIZE> const& a, Type<T, SIZE> const& b, Type<T, SIZE> const& c) noexcept { + MKN_AVX_FN_COUNTER; return {Type<T, SIZE>::Super::impl_type::fma(a(), b(), c())}; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build_nix.yml.github/workflows/build_win.ymlinc/mkn/avx/lazy.hppinc/mkn/avx/url.hppmkn.yamltest/test_lazy.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- inc/mkn/avx/lazy.hpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: mkn/mkn.avx PR: 12
File: test/bench.cpp:97-99
Timestamp: 2025-03-23T21:51:48.782Z
Learning: Do not comment on pipeline failures or CI build issues for the mkn.avx repository. The user prefers to handle these issues separately from code reviews.
📚 Learning: 2025-03-16T19:40:42.661Z
Learnt from: PhilipDeegan
Repo: mkn/mkn.avx PR: 11
File: inc/mkn/avx/array.hpp:185-190
Timestamp: 2025-03-16T19:40:42.661Z
Learning: In the mkn.avx library, separate type parameters (T0, T1) are used in template functions to handle const/non-const type variations rather than for actual different types. If the types are incompatible, the code will fail to compile.
Applied to files:
inc/mkn/avx/url.hpp
📚 Learning: 2025-03-16T14:42:12.355Z
Learnt from: PhilipDeegan
Repo: mkn/mkn.avx PR: 11
File: inc/mkn/avx/array.hpp:93-96
Timestamp: 2025-03-16T14:42:12.355Z
Learning: In the mkn::avx Array class, the move constructor is implemented as a call to the copy constructor because the base class _A_ has a deleted move constructor. This design choice was made to ensure the class can be returned from functions while maintaining the constraint that the underlying array cannot be moved.
Applied to files:
inc/mkn/avx/url.hpp
🧬 Code graph analysis (1)
inc/mkn/avx/url.hpp (2)
inc/mkn/avx/array.hpp (16)
arr(118-123)arr(118-118)arr(124-129)arr(124-124)arr(132-137)arr(132-132)arr(138-143)arr(138-138)arr(146-151)arr(146-146)arr(152-157)arr(152-152)arr(160-165)arr(160-160)arr(166-171)arr(166-166)inc/mkn/avx/dbg.hpp (1)
i(45-49)
🪛 Clang (14.0.6)
inc/mkn/avx/url.hpp
[error] 6-6: 'cstdint' file not found
(clang-diagnostic-error)
🔇 Additional comments (11)
test/test_lazy.cpp (3)
11-11: LGTM! Good move to centralized alignment configuration.Using
Options::ALIGN()consistently across all vector typedefs improves maintainability and allows compile-time alignment overrides.
16-16: Verify that tests are not built with NDEBUG defined.Switching from
abort_if_nottoassertis cleaner, butassertis a no-op whenNDEBUGis defined. Ensure that test builds do not defineNDEBUG, or test assertions will be silently skipped.
110-110: Useful diagnostic for identifying test output..github/workflows/build_win.yml (1)
1-30: Windows CI workflow looks well-structured.The workflow includes:
- Proper concurrency control to cancel in-progress runs on new commits.
- MSVC toolchain setup with AMD64 architecture.
- Documented workaround for the
/bin/linkconflict..github/workflows/build_nix.yml (1)
21-21: LGTM! Build targets updated to match new profiles.The changes align the CI command with the new test profiles (
test,b0,b1) defined inmkn.yaml, and the added-DKUL_FORCE_TRACEflag is consistent with the Windows workflow.mkn.yaml (2)
12-37: Profile structure is clean and well-organized.The new profiles (
test_lazy,b0,b1) follow the existing pattern, and havingb0/b1share the same source file with different compile-time flags is a sensible approach for benchmark variants.
1-1: Comment on line 1 references undefineddbgprofile in this file.The comment includes
dbgin the directive flags, but no profile withname: dbgis defined in mkn.yaml. The profilestest,test_lazy, andgbenchare all present, butdbgis not. Since mkn.yaml listsmkn.kulas a parent dependency (external), thedbgprofile may be inherited from that parent or be a built-in profile in the mkn build system. Clarify whether this is intentional or if the reference should be removed.inc/mkn/avx/url.hpp (4)
1-29: LGTM: Primary template and fallback operations are well-structured.The primary template correctly provides non-AVX fallback operations for SIZE==1, with appropriate static_assert to catch missing specializations.
104-144: LGTM: Float specializations consistently wrap intrinsics.All float specializations correctly wrap AVX intrinsics in lambdas, avoiding MSVC linker issues.
149-190: LGTM: Integer specializations correctly wrap intrinsics.All integer type specializations consistently wrap AVX intrinsics in lambdas.
220-241: Verify Type wrapper works on MSVC after fixing line 84.The Type wrapper correctly aliases operations from Type_<T, SIZE> specializations. Once the direct intrinsic assignment on line 84 is fixed, verify that this aliasing approach works correctly on MSVC.
| auto& operator[](std::size_t i) noexcept { return reinterpret_cast<T*>(&array)[i]; } | ||
| auto& operator[](std::size_t i) const noexcept { return array[i]; } |
There was a problem hiding this comment.
Fix inconsistent const operator[] implementation.
The non-const operator[] uses reinterpret_cast<T*>(&array)[i] to access elements, but the const version returns array[i]. This inconsistency means the const version attempts to subscript the AVX register type directly, which is invalid.
🐛 Proposed fix
- auto& operator[](std::size_t i) const noexcept { return array[i]; }
+ auto const& operator[](std::size_t i) const noexcept { return reinterpret_cast<T const*>(&array)[i]; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto& operator[](std::size_t i) noexcept { return reinterpret_cast<T*>(&array)[i]; } | |
| auto& operator[](std::size_t i) const noexcept { return array[i]; } | |
| auto& operator[](std::size_t i) noexcept { return reinterpret_cast<T*>(&array)[i]; } | |
| auto const& operator[](std::size_t i) const noexcept { return reinterpret_cast<T const*>(&array)[i]; } |
🤖 Prompt for AI Agents
In @inc/mkn/avx/url.hpp around lines 48 - 49, The const overload of operator[]
is inconsistent and invalid because it tries to subscript the AVX register
member `array` directly; change the const `auto& operator[](std::size_t i) const
noexcept` to mirror the non-const version by returning `reinterpret_cast<const
T*>(&array)[i]` (i.e., cast `&array` to `const T*` and index it) so both
overloads return the same element type and remain noexcept.
| auto const static inline set = [](auto&&... v) { return _mm256_store_pd(v...); }; | ||
| auto const static inline set_v = [](auto&&... v) { return _mm256_set1_pd(v...); }; | ||
|
|
||
| auto const static inline fma = _mm256_fmadd_pd; |
There was a problem hiding this comment.
Fix inconsistent intrinsic assignment causing MSVC linker errors.
Line 84 directly assigns _mm256_fmadd_pd without wrapping it in a lambda, unlike all other intrinsic operations (lines 77-82, and throughout other specializations). On MSVC, AVX intrinsics are inline pseudo-functions without linkable symbols; directly assigning them to static members causes the linker to expect a real symbol, resulting in LNK2019 errors.
This inconsistency is the root cause of the Windows CI failures mentioned in the PR objectives.
🐛 Proposed fix to wrap in lambda
- auto const static inline fma = _mm256_fmadd_pd;
+ auto const static inline fma = [](auto&&... v) { return _mm256_fmadd_pd(v...); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto const static inline fma = _mm256_fmadd_pd; | |
| auto const static inline fma = [](auto&&... v) { return _mm256_fmadd_pd(v...); }; |
🤖 Prompt for AI Agents
In @inc/mkn/avx/url.hpp at line 84, The static member assignment for fma (auto
const static inline fma) directly binds to the AVX intrinsic _mm256_fmadd_pd
which on MSVC produces linker errors; change the assignment to match the other
intrinsic members by wrapping the call in a lambda that forwards its arguments
to _mm256_fmadd_pd (same pattern used for the other ops around lines 77-82), so
fma becomes an inline lambda taking the intended parameters and returning the
intrinsic result, avoiding creating a non-linkable symbol.
1 participant
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.