-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47229: [C++][Arm] Force mimalloc to generate armv8.0 binary #47766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Can you check if this patch is still necessary if we apply #47589 ? |
Looks necessary for mimalloc 3.1.5. The default setting is still armv8.1-a unless this option is set. |
…ble binary Mimalloc default generates LSE atomic instructions only work on armv8.1. This causes illegal instruction on armv8.0 platforms like Raspberry4. This PR sets mimalloc build flag -DMI_NO_OPT_ARCH=ON to disable LSE instruction. Please note even with flag set, compiler and libc will replace the atmoic call with an ifunc that matches hardware best at runtime. That means LSE is used only if the running platform supports it.
@github-actions crossbow submit -g cpp |
@github-actions crossbow submit wheelcp313* |
Co-authored-by: Antoine Pitrou <[email protected]>
Revision: ff8e8c3 Submitted crossbow builds: ursacomputing/crossbow @ actions-7e9ea384fd |
Revision: ff8e8c3 Submitted crossbow builds: ursacomputing/crossbow @ actions-a989831826 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
### Rationale for this change Mimalloc default generates LSE atomic instructions only work on armv8.1. This causes illegal instruction on armv8.0 platforms like Raspberry4. This PR sets mimalloc build flag -DMI_NO_OPT_ARCH=ON to disable LSE instruction. Please note even with flag set, compiler and libc will replace the atmoic call with an ifunc that matches hardware best at runtime. That means LSE is used only if the running platform supports it. ### What changes are included in this PR? Force mimalloc build flag -DMI_NO_OPT_ARCH=ON. ### Are these changes tested? Manually tested. ### Are there any user-facing changes? No. **This PR contains a "Critical Fix".** Fixes crashes on Armv8.0 platform. * GitHub Issue: #47229 Lead-authored-by: Yibo Cai <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 8496d4e. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…pache#47766) ### Rationale for this change Mimalloc default generates LSE atomic instructions only work on armv8.1. This causes illegal instruction on armv8.0 platforms like Raspberry4. This PR sets mimalloc build flag -DMI_NO_OPT_ARCH=ON to disable LSE instruction. Please note even with flag set, compiler and libc will replace the atmoic call with an ifunc that matches hardware best at runtime. That means LSE is used only if the running platform supports it. ### What changes are included in this PR? Force mimalloc build flag -DMI_NO_OPT_ARCH=ON. ### Are these changes tested? Manually tested. ### Are there any user-facing changes? No. **This PR contains a "Critical Fix".** Fixes crashes on Armv8.0 platform. * GitHub Issue: apache#47229 Lead-authored-by: Yibo Cai <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
Rationale for this change
Mimalloc default generates LSE atomic instructions only work on armv8.1. This causes illegal instruction on armv8.0 platforms like Raspberry4. This PR sets mimalloc build flag -DMI_NO_OPT_ARCH=ON to disable LSE instruction.
Please note even with flag set, compiler and libc will replace the atmoic call with an ifunc that matches hardware best at runtime. That means LSE is used only if the running platform supports it.
What changes are included in this PR?
Force mimalloc build flag -DMI_NO_OPT_ARCH=ON.
Are these changes tested?
Manually tested.
Are there any user-facing changes?
No.
This PR contains a "Critical Fix".
Fixes crashes on Armv8.0 platform.