[SPH] reduce alloc in ghost zone find #1465
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
Summary of ChangesHello @tdavidcl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Workflow reportworkflow report corresponding to commit 0dc5d22 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportSome failures were detected in base source checks checks. ❌ Check doxygen headersThe pre-commit checks have found some files without or with a wrong doxygen file header A properly formed doxygen header should be like and placed just below This test is triggered if the doxygen header is missing or with the wrong file name At some point we will refer to a guide in the doc about this List of files with errors :
Suggested changesDetailed changes :diff --git a/src/shamalgs/include/shamalgs/primitives/stream_compact.hpp b/src/shamalgs/include/shamalgs/primitives/stream_compact.hpp
index b0814898..7d779e62 100644
--- a/src/shamalgs/include/shamalgs/primitives/stream_compact.hpp
+++ b/src/shamalgs/include/shamalgs/primitives/stream_compact.hpp
@@ -10,7 +10,7 @@
#pragma once
/**
- * @file sort_by_keys.hpp
+ * @file stream_compact.hpp
* @author Timothée David--Cléris (tim.shamrock@proton.me)
* @brief Sort by keys algorithms
*
@@ -21,6 +21,6 @@
namespace shamalgs::primitives {
sham::DeviceBuffer<u32> stream_compact(
- const sham::DeviceScheduler_ptr &sched, sham::DeviceBuffer<u32> && buf_flags, u32 len);
+ const sham::DeviceScheduler_ptr &sched, sham::DeviceBuffer<u32> &&buf_flags, u32 len);
} // namespace shamalgs::primitives
diff --git a/src/shamalgs/src/primitives/stream_compact.cpp b/src/shamalgs/src/primitives/stream_compact.cpp
index a3b39d6e..e2e13f28 100644
--- a/src/shamalgs/src/primitives/stream_compact.cpp
+++ b/src/shamalgs/src/primitives/stream_compact.cpp
@@ -8,7 +8,7 @@
// -------------------------------------------------------//
/**
- * @file sort_by_keys.hpp
+ * @file stream_compact.cpp
* @author Timothée David--Cléris (tim.shamrock@proton.me)
* @brief Sort by keys algorithms
*
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ghost zone finding logic to improve performance by restructuring kernel launches. A new stream_compact primitive is introduced. The changes in BasicSPHGhosts.cpp batch the mask generation kernels, which is a good optimization pattern for GPUs.
My review has identified a critical out-of-bounds memory access in the new stream_compact implementation, which needs to be fixed. I've also found some minor documentation issues (copy-paste errors) in the new files. Please see the detailed comments.
| sched->get_queue(), | ||
| sham::MultiRef{buf_flags}, | ||
| sham::MultiRef{index_map}, | ||
| len + 1, |
There was a problem hiding this comment.
The kernel is launched with len + 1 work items, which can lead to an out-of-bounds memory access. The loop variable idx will range from 0 to len. When idx is len, the expression sum_vals[idx + 1] on line 48 will attempt to access sum_vals[len + 1]. However, buf_flags (which sum_vals points to) is guaranteed to have a size of at least len + 1, meaning valid indices are from 0 to len. Accessing index len + 1 is out of bounds.
The kernel is intended to process len flags, so it should be launched with len work items. This will make idx range from 0 to len - 1, and the maximum index accessed will be sum_vals[len], which is within the buffer's bounds.
| len + 1, | |
| len, |
| * @file sort_by_keys.hpp | ||
| * @author Timothée David--Cléris (tim.shamrock@proton.me) | ||
| * @brief Sort by keys algorithms |
There was a problem hiding this comment.
The documentation in this file header appears to be copied from sort_by_keys.hpp. Please update it to accurately describe the contents of stream_compact.hpp and the stream_compact function.
* @file stream_compact.hpp
* @author Timothée David--Cléris (tim.shamrock@proton.me)
* @brief Stream compaction primitive| * @file sort_by_keys.hpp | ||
| * @author Timothée David--Cléris (tim.shamrock@proton.me) | ||
| * @brief Sort by keys algorithms |
There was a problem hiding this comment.
The documentation in this file header seems to be copied from sort_by_keys.hpp. Please update it to describe the stream_compact implementation in this file.
| * @file sort_by_keys.hpp | |
| * @author Timothée David--Cléris (tim.shamrock@proton.me) | |
| * @brief Sort by keys algorithms | |
| * @file stream_compact.cpp | |
| * @author Timothée David--Cléris (tim.shamrock@proton.me) | |
| * @brief Implementation of the stream compaction primitive |
1 participant
No description provided.