-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix task_alignment
for ARM
#1386
Conversation
Signed-off-by: Valery Matskevich <[email protected]>
Signed-off-by: Valery Matskevich <[email protected]>
Signed-off-by: Valery Matskevich <[email protected]>
Signed-off-by: Valery Matskevich <[email protected]>
@@ -220,7 +224,11 @@ class alignas(task_alignment) task : public task_traits { | |||
virtual task* cancel(execution_data&) = 0; | |||
|
|||
private: | |||
#if __ARM_ARCH_7A__ || __aarch64__ | |||
std::uint64_t m_reserved[14]{}; |
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.
i decided to put #if
here, instead of (task_alignment - sizeof(void*) - sizeof(task_traits)) / sizeof(std::uint64_t)
, because it gets weird on architectures where pointer isn't 8 bytes? idk, this seems cleaner than it was before
This patch might introduce backward compatibility problems so it should be carefully investigated. |
I tried to come up with some benchmarks with different variations of system topology, however, no difference is spotted :( |
Description
task_alignment
seem to be (asgit blame
says) added to _task.h in 2019 when moving from TBB to oneTBB. However, it happens so thattask_alignment
is 64 bytes by default (x86 cacheline), which doesn't make any sense for ARM architectures with 128 bytes cacheline.Fixes # - issue number(s) if exists
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@
to send notificationsOther information