Skip to content

[OpenMP] Provide big-endian bitfield definitions #69995

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

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

iii-i
Copy link
Member

@iii-i iii-i commented Oct 24, 2023

structs kmp_depend_info.flags and kmp_tasking_flags contain bitfields, which overlay integer flag values. The current bitfield definitions target little-endian machines. On big-endian machines bitfields are laid out in the opposite order, so the current definitions do not work there.

There are two ways to fix this: either provide big-endian bitfield definitions, or bit-swap integer flag values. Go with the former, since it's localized to one place and therefore is more maintainable.

structs kmp_depend_info.flags and kmp_tasking_flags contain bitfields,
which overlay integer flag values. The current bitfield definitions
target little-endian machines. On big-endian machines bitfields are
laid out in the opposite order, so the current definitions do not work
there.

There are two ways to fix this: either provide big-endian bitfield
definitions, or bit-swap integer flag values. Go with the former,
since it's localized to one place and therefore is more maintainable.
@llvmbot llvmbot added the openmp:libomp OpenMP host runtime label Oct 24, 2023
@shiltian
Copy link
Contributor

shiltian commented Oct 24, 2023

I'm wondering since we use them via x.y, does it really affect the correctness on big-endian systems? I understand given current implementation, unused or reserved will be at the beginning of a buffer.

@iii-i
Copy link
Member Author

iii-i commented Oct 24, 2023

If we were doing just that, it would be totally fine. However, there is code that bitcasts ints to these structs like this:

kmp_task_t *__kmpc_omp_task_alloc(ident_t *loc_ref, kmp_int32 gtid,
                                  kmp_int32 flags, size_t sizeof_kmp_task_t,
                                  size_t sizeof_shareds,
                                  kmp_routine_entry_t task_entry) {
  kmp_task_t *retval;
  kmp_tasking_flags_t *input_flags = (kmp_tasking_flags_t *)&flags;

That's where big- and little-endian differ.

Copy link
Contributor

@shiltian shiltian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Yeah, we need that.

@iii-i iii-i merged commit 34459b7 into llvm:main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openmp:libomp OpenMP host runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants