build: Allow exporting Eigen3 alignment values #4098
Conversation
WalkthroughIntegrated into the build process, new configurations are. A CMake option, Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
stephenswat
force-pushed
the
build/public_eigen3_alignment
branch
from
f829e04 to
3d70e96
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmake/ActsConfig.cmake.in (1)
120-146: Use generator expressions instead of deprecatedLOCATIONproperty, you should.Hmmm. Reading the object file, a fragile approach it is. Preferred, a generator expression
$<TARGET_FILE:ActsCore>might be, to locate the compiled artifact across various build configurations. In multi-config environments, break this code could, yes.A potential revision:
- get_target_property(_ACTS_CORE_LOCATION ActsCore LOCATION) + set(_ACTS_CORE_LOCATION "$<TARGET_FILE:ActsCore>")Then adapt your file-reading logic accordingly, you must, hmm?
CMakeLists.txt (1)
131-132: Option naming consistent, it is.Good addition, the new alignment option is. Possibly wish you may to expand its documentation or usage examples for clarity, ensure new builders do.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Core/src/Utilities/Eigen3Alignment.cpp(1 hunks)cmake/ActsConfig.cmake.in(1 hunks)docs/getting_started.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Core/src/Utilities/CMakeLists.txt
- Core/src/Utilities/Eigen3Alignment.cpp
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
🔇 Additional comments (1)
docs/getting_started.md (1)
319-319: Documentation aligned with new option, it is.Clear and consistent, hmm. The mention of
ACTS_ENFORCE_EIGEN3_ALIGNMENTin the build options section is good. Enough, it appears. Approved, this is.
stephenswat
force-pushed
the
build/public_eigen3_alignment
branch
from
3d70e96 to
e27cb0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Core/src/Utilities/Eigen3Alignment.cpp(1 hunks)cmake/ActsConfig.cmake.in(1 hunks)docs/getting_started.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Core/src/Utilities/CMakeLists.txt
- docs/getting_started.md
- CMakeLists.txt
- Core/src/Utilities/Eigen3Alignment.cpp
🔇 Additional comments (2)
cmake/ActsConfig.cmake.in (2)
121-124: Wise validation of prerequisites, this is!Clear and necessary, the target validation is. Prevent configuration failures early, it does.
135-145:❓ Verification inconclusive
Hmm, process the alignment value wisely, you do.
Proper handling of edge cases and clear messaging, I sense. Yet verify the alignment value's impact on dependent projects, we must.
Run this script to check alignment value propagation:
🏁 Script executed:
#!/bin/bash # Description: Verify alignment value propagation to dependent targets # Test: Check if the compile definition is properly propagated cmake --build . --target help | grep ActsCore cmake --trace-expand --find-package=Acts . 2>&1 | grep -A 5 "Setting ACTS Eigen3 alignment"Length of output: 232
Manual verification required, it is.
The code handles edge cases and messaging well, yes. However, the test script failed to run due to missing cmake (command not found), making the propagation of the compile definition into dependent projects uncertain. I advise you to run the verification script in an environment where cmake is available, or manually check that the
ActsCoretarget correctly receives the-DEIGEN_MAX_ALIGN_BYTESdefinition.
- Ensure cmake is properly installed and in your PATH.
- Re-run tests to confirm that "Setting ACTS Eigen3 alignment" is printed and the value correctly propagates.
- Check the
target_compile_definitions(ActsCore ...)usage when building dependent targets.
| set(_ACTS_CORE_EIGEN3_ALIGNMENT_REGEX "ACTS::EigenAlignment=([0-9]+)") | ||
| get_target_property(_ACTS_CORE_LOCATION ActsCore LOCATION) | ||
|
|
||
| file( | ||
| STRINGS ${_ACTS_CORE_LOCATION} _ACTS_CORE_EIGEN3_ALIGNMENT_VALUE_STRING | ||
| LIMIT_COUNT 1 | ||
| REGEX ${_ACTS_CORE_EIGEN3_ALIGNMENT_REGEX} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case of missing target location, we must.
Missing, the error handling for target location property is. If get_target_property fails, undefined behavior we may face.
Apply this change, you should:
get_target_property(_ACTS_CORE_LOCATION ActsCore LOCATION)
+if (NOT _ACTS_CORE_LOCATION)
+ message(FATAL_ERROR "Failed to get ActsCore target location.")
+endif()📝 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.
| set(_ACTS_CORE_EIGEN3_ALIGNMENT_REGEX "ACTS::EigenAlignment=([0-9]+)") | |
| get_target_property(_ACTS_CORE_LOCATION ActsCore LOCATION) | |
| file( | |
| STRINGS ${_ACTS_CORE_LOCATION} _ACTS_CORE_EIGEN3_ALIGNMENT_VALUE_STRING | |
| LIMIT_COUNT 1 | |
| REGEX ${_ACTS_CORE_EIGEN3_ALIGNMENT_REGEX} | |
| ) | |
| set(_ACTS_CORE_EIGEN3_ALIGNMENT_REGEX "ACTS::EigenAlignment=([0-9]+)") | |
| get_target_property(_ACTS_CORE_LOCATION ActsCore LOCATION) | |
| if (NOT _ACTS_CORE_LOCATION) | |
| message(FATAL_ERROR "Failed to get ActsCore target location.") | |
| endif() | |
| file( | |
| STRINGS ${_ACTS_CORE_LOCATION} _ACTS_CORE_EIGEN3_ALIGNMENT_VALUE_STRING | |
| LIMIT_COUNT 1 | |
| REGEX ${_ACTS_CORE_EIGEN3_ALIGNMENT_REGEX} | |
| ) |
As I discovered in acts-project#4049, having Eigen3 types on ABI boundaries invites a variety of compiler issues. In cases where ACTS and the depending project are compiled with different vectorization instruction sets, the ABIs become incompatible with each other, because the matrices are differently aligned. This commit adds the `ACTS_ENFORCE_EIGEN3_ALIGNMENT` flag which, if enabled, will encode the Eigen3 alignment in the ACTS shared object, and the ACTS `find_package` code will then attempt to read this and set it appropriately on any dependent targets. Closes acts-project#4049.
stephenswat
force-pushed
the
build/public_eigen3_alignment
branch
from
e27cb0e to
e917b5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Core/CMakeLists.txt (1)
89-91: Regular expression, more strict it should be.The pattern
[[:digit:]]{4}, any four digits it matches. More specific format for alignment value, enforce we should.Improve pattern to match exact format:
- \"ACTS::EigenAlignment=[[:digit:]]{4}\" || + \"ACTS::EigenAlignment=[0-9][0-9][0-9][0-9]$\" ||
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)Core/CMakeLists.txt(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Core/src/Utilities/Eigen3Alignment.cpp(1 hunks)cmake/ActsConfig.cmake.in(1 hunks)docs/getting_started.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Core/src/Utilities/CMakeLists.txt
- docs/getting_started.md
- cmake/ActsConfig.cmake.in
- CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
|
Thinking about this a bit more I believe this is the best solution. I wondered if it would also make more sense to have a CMake flag to set the alignment explicitly from outside which would be a bit more direct but requires manual intervention. I have a few more questions
|
There was a problem hiding this comment.
This is compiled into ACTS so we can later extract it from the binary right?
Wouldn't it be easier to put it in a text file and install it along ACTS and then read it from the text file to enforce the compiler flag?
This is actually an important point: in Athena we require the Athena-based Eigen headers to be included before ours, because they add their plugins. Not all packages link against I suspect we do, and if I'm correct, we cannot do it this way or at least we can't turn it on on Athena. |
|
Potentially we could have a target which just introduces this definition so we do not risk incompatibilities with other clients? Something like |
As far as I know, no, there is no standard mechanism for this. The common wisdom is not to expose Eigen types on your ABI boundary at all, but I think this ship has failed. Rightfully, nobody saw this monumental ass-pain coming. Anyway, there is a way to enforce universal Eigen code, namely to either 1) set
I don't think so; Eigen is header-only so it does not have any objects of its own, and any object using both Eigen and ACTS would get the That being said, there is another pathological case here, namely if we have library A and B, where A depends on B, A depends on Acts, and B depends on Eigen, and A and B use Eigen3 types over their ABI boundary. Because then B will not inherit the Eigen flag from Acts, while A does. I didn't think of that. 😢
The problem is that providing the same |
Not a header include issue, but as I wrote above I do think this is (unfortunately) an issue with dependent libraries... |
But that we can never treat right? It is annoying but I would not worry too much about it. If we ever encounter it we have to either compile with consistent flags or somehow tweak the alignment for both libraries to be the same. |
|
@andiwand But if this setup here doesn't avoid that problem, I don't necessarily think we should add this to the CMake config to begin with. |
|
Yeah I think we need to rethink this... And unfortunately I don't think there is any good (=easy and foolproof) solution to this problem. |
|
We discussed this in the morning and there are two options
|
As I discovered in #4049, having Eigen3 types on ABI boundaries invites a variety of compiler issues. In cases where ACTS and the depending project are compiled with different vectorization instruction sets, the ABIs become incompatible with each other, because the matrices are differently aligned.
This commit adds the
ACTS_ENFORCE_EIGEN3_ALIGNMENTflag which, if enabled, will encode the Eigen3 alignment in the ACTS shared object, and the ACTSfind_packagecode will then attempt to read this and set it appropriately on any dependent targets.Closes #4049.
Summary by CodeRabbit
New Features
Documentation