Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR separates SIMD and scalar handling in unaligned vector-pack load/store operations for the SIMD and EVE backends. It exports ChangesUnaligned vector pack load/store regression fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
|
Can one of the admins verify this patch? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/core/execution/tests/regressions/vector_pack_unaligned_load_store_7287.cpp (1)
36-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd deterministic coverage for the scalar width-one path.
vector_pack_type_t<value_type>selects the default width and skips the width-one scalar fallback in this test. Use an explicitvector_pack_type_t<value_type, 1>case to cover scalar unaligned load/store independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/execution/tests/regressions/vector_pack_unaligned_load_store_7287.cpp` around lines 36 - 39, Update the test’s vector type declaration near value_type and V to explicitly instantiate vector_pack_type_t with width 1, ensuring the unaligned load/store regression exercises the scalar fallback path deterministically while preserving the existing width-based test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@libs/core/execution/include/hpx/execution/traits/detail/eve/vector_pack_load_store.hpp`:
- Around line 43-45: Replace the undocumented eve::is_simd_value predicate in
the affected vector-pack load/store branches with the supported eve::simd_value
trait, matching the neighboring vector_pack_type specialization; apply this
consistently at both referenced locations.
In `@libs/core/execution/tests/regressions/CMakeLists.txt`:
- Around line 11-14: Add regression coverage for both datapar backend paths by
adding or selecting CI configurations that set HPX_WITH_DATAPAR_BACKEND to EVE
and STD_EXPERIMENTAL_SIMD, rather than relying only on HPX_WITH_DATAPAR or
EMULATE. Ensure the existing datapar regression tests, including
lambda_arguments_2403, lambda_return_type_2402, rebind_par_simd, and
vector_pack_unaligned_load_store_7287, execute under each backend.
---
Nitpick comments:
In
`@libs/core/execution/tests/regressions/vector_pack_unaligned_load_store_7287.cpp`:
- Around line 36-39: Update the test’s vector type declaration near value_type
and V to explicitly instantiate vector_pack_type_t with width 1, ensuring the
unaligned load/store regression exercises the scalar fallback path
deterministically while preserving the existing width-based test setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 897cdad7-3603-4e92-be9d-f6e879b47b86
📒 Files selected for processing (5)
libs/core/execution/include/hpx/execution/traits/detail/eve/vector_pack_load_store.hpplibs/core/execution/include/hpx/execution/traits/detail/simd/vector_pack_load_store.hpplibs/core/execution/include/hpx/execution/traits/detail/simd/vector_pack_simd.hpplibs/core/execution/tests/regressions/CMakeLists.txtlibs/core/execution/tests/regressions/vector_pack_unaligned_load_store_7287.cpp
|
Please have a look at the clang-format complaints and address the comments (or mark them as resolved with an explanation). |
vector_pack_load<V, T>::unaligned returned a broadcast of a single scalar across every lane instead of loading the N consecutive elements starting at that address, for both the EVE and std-experimental-simd backends; the symmetric bug existed in vector_pack_store<V, T>::unaligned. Fix both to perform genuine unaligned loads/stores (EVE via the pointer constructor and eve::store, simd via the newly exposed element_aligned tag). V can also be instantiated as a scalar value_type (the width-1 pack used for remainder/tail elements), which neither eve::wide's pointer constructor, eve::store, nor std::experimental::simd's copy_to accept, so both fixes are gated on an is_simd_value/is_simd_v check with the original scalar behavior preserved in the else branch. Add a regression test that loads a non-uniform full-width pack via unaligned(), verifies every lane against the source (a broadcast bug collapses every lane to the first element), negates each lane, stores it back via unaligned(), and verifies each destination element was updated independently rather than overwritten with one repeated value. Confirmed this test fails to compile against the pre-fix code (EVE backend) and passes with the fix. Fixes TheHPXProject#7287 Signed-off-by: adhithyaragavan <[email protected]>
3bc918a to
246fb37
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Fixed the clang-format issue (and the DCO sign-off / C++20 modules build failure that were also flagged), pushed. Replied inline to the two CodeRabbit comments below with my reasoning on why I left those as is, let me know if you'd like it done differently. |
Summary
Fixes #7287.
vector_pack_load<V, T>::unalignedreturned a broadcast of a single scalar (*iter) across every lane of the pack instead of loading the N consecutive elements starting at that address, for both the EVE and std-experimental-simd backends. The symmetric bug existed invector_pack_store<V, T>::unaligned, writing the same broadcast value everywhere instead of storing each lane back to its own position.unaligned()now constructs directly from the pointer (V(std::addressof(*iter))), which resolves toeve::wide's genuine unaligned-load constructor rather than the scalar-broadcast constructor; store useseve::store(value, ptr)with a plain (non-as_aligned) pointer.std::experimental::element_aligned(previously onlyvector_alignedwas re-exported) and used it for both load and store.Vcan also be instantiated as a scalarvalue_type— the width-1 "pack" used by the remainder/tail-element path (seevector_pack_type<T, 1, Abi>) — for which neithereve::wide's pointer constructor /eve::store, norstd::experimental::simd's pointer constructor /copy_to, are valid. Both fixes are gated oneve::is_simd_value<V>{}/datapar::experimental::is_simd_v<V>respectively, falling back to the original plain-assignment behavior for the scalar case.Scope: this only touches the
unalignedload/store fix itself, split out from the larger change in #7288 (which also rewritessearch/search_n) per discussion there.Tests
Added
libs/core/execution/tests/regressions/vector_pack_unaligned_load_store_7287.cpp(gated onHPX_WITH_DATAPAR), following the naming/registration pattern of the other numbered regression tests in that directory (annotated_minmax_2593,chunk_size_4118, etc.). It:unaligned()and checks every lane against the source data (a broadcast bug collapses every lane to the first element).unaligned(), then checks every destination element was updated independently (a broadcast bug overwrites the whole range with one repeated value).Test plan
HPX_WITH_DATAPAR_BACKEND=EVE: new regression test passes.eve::wide's scalar-broadcast constructor isexplicit, soreturn *iter;fails to convert to a full-widthVwhen the function is actually instantiated with a genuine pack (as happens viainvoke_vectorized_in2/invoke_vectorized_in2_indiniterator_helpers.hpp, used by two-iterator algorithms likemismatch/equal_binary/transform_binary). None of the current datapar test suite happens to instantiateunaligned()with a full pack today, so this was previously silent; the new regression test is (as far as I can tell) the first thing in-tree to exercise that path directly.foreach_datapar_test,transform_datapar_test,find_datapar_test,datapar_zip_iterator_guard_test,mismatch_datapar_test,equal_binary_datapar_test,transform_binary_datapar_test,search_datapar_test,search_n_datapar_test,foreach_datapar_zipiter_test) — all still pass with the fix applied.std::experimental::simdfix was verified by inspection/compile reasoning only (this machine's compilers don't expose the same EVE test harness for that backend) — would appreciate a check on CI.