Implement template helper functions to reduce macro depencencies in SoA backend #48607
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48607/45574
|
|
A new Pull Request was created by @sbaldu for master. It involves the following packages:
@cmsbuild, @fwyzard, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48607/45580
|
|
enable gpu |
|
@cmsbuild, please test |
|
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
CUDA Comparison SummarySummary:
ROCM Comparison SummarySummary:
|
|
The changes look OK to me, from what I can gleam of the old macros. @sbaldu @leobeltra could you post the fully expanded code of a sample SoA before and after these changes, to have a better look at what is the impact ? |
I don't have further comments |
|
OK, thanks. |
| // Helper functions for accumulating column elements | ||
| template <typename ColumnType> | ||
| struct AccumulateSoAElements; |
There was a problem hiding this comment.
If I understand correctly the implementation, these do not accumulate "column elements" but "column size in bytes" ?
Then I would suggest a more accurate name, like AccumulateByteSize ?
There was a problem hiding this comment.
Also, could you move these and any other names that users should not be aware of to cms::soa::impl or something similar ?
There was a problem hiding this comment.
Just a curiosity - why computePitch are functions and AccumulateSoAElements are structs ?
There was a problem hiding this comment.
Just a curiosity - why
computePitchare functions andAccumulateSoAElementsare structs ?
That is because the computation is called inside computeDataSize, which is a static method, so we couldn't call a non-static function inside of it.
There was a problem hiding this comment.
I though that AccumulateSoAElements are structs and computePitch are free functions - none of them are methods ?
There was a problem hiding this comment.
Yes they are free functions, what I meant is that when I call functions like computePitch I pass the column member to resolve the overload, but inside computeDataSize I couldn't do this because I couldn't access the data members of the Layout through this.
So I had to diversify the behaviour for the different types of columns based on their type, and since I can't partially specialize a free function I used a struct.
There was a problem hiding this comment.
OK, I admit to being confused...
Still, can you
- rename
AccumulateSoAElementstoComputeSoAElementByteSize - make the
operator()methodstatic(maybe rename it for convenience) - add a free function interface like
computeSoAElementByteSize(...)that callsComputeSoAElementByteSize::operator()(...)
?
Would that be doable ?
Use template functions instead of macro `SWITCH_ON_TYPE`s Put template utilities inside `cms::soa::detail` namespace
sbaldu
force-pushed
the
feature_SoA_template_meta
branch
from
45654ca to
3e41c72
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48607/45906
|
|
please test |
|
+1 Size: This PR adds an extra 40KB to repository Comparison SummarySummary:
CUDA Comparison SummarySummary:
ROCM Comparison SummarySummary:
|
|
+heterogeneous |
cmsbuild
added
fully-signed
heterogeneous-approved
and removed
pending-signatures
heterogeneous-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @mandrenguyen, @ftenchini (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
5 participants
PR description:
This PR implements helper functions and type traits to handle the behaviours required by the different column types without the need to use boost conditions, thus reducing the use of macros and improving readability.
There are some helper functions that are marked as TODOs, as they are waiting for further developments in the backend of the SoA Layout class.
PR validation:
All the tests still pass, as the behaviour is unchanged.
FYI @leobeltra @fwyzard @felicepantaleo