Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[OpenMP][test]Flip bit-fields in 'struct flags' for big-endian in test cases #79895

Merged
merged 4 commits into from
Feb 7, 2024
Merged

[OpenMP][test]Flip bit-fields in 'struct flags' for big-endian in test cases #79895

merged 4 commits into from
Feb 7, 2024

Conversation

xingxue-ibm
Copy link
Contributor

This patch flips bit-fields in struct flags for big-endian in test cases to be consistent with the definition of the structure in libomp kmp.h.

@xingxue-ibm xingxue-ibm added the openmp:libomp OpenMP host runtime label Jan 29, 2024
@xingxue-ibm xingxue-ibm self-assigned this Jan 29, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 29, 2024 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@iii-i iii-i requested a review from uweigand January 30, 2024 01:53
@iii-i
Copy link
Member

iii-i commented Jan 30, 2024

The change looks good to me.

One thing though: grepping for 'struct kmp_depend_info' shows two other places where we have these bit fields: openmp/libomptarget/include/OpenMP/InternalTypes.h and openmp/runtime/test/tasking/hidden_helper_task/common.h. Would it make sense to make this change there as well?

@xingxue-ibm
Copy link
Contributor Author

The change looks good to me.

One thing though: grepping for 'struct kmp_depend_info' shows two other places where we have these bit fields: openmp/libomptarget/include/OpenMP/InternalTypes.h and openmp/runtime/test/tasking/hidden_helper_task/common.h. Would it make sense to make this change there as well?

Thanks for checking, @iii-i! Changed openmp/runtime/test/tasking/hidden_helper_task/common.h for big-endian as well. openmp/libomptarget/include/OpenMP/InternalTypes.h seems to be a header for the target runtime. Since I won't be able to test changes, I'd refrain from touching it and will leave it for future if the target runtime needs to support big-endian.

@xingxue-ibm
Copy link
Contributor Author

Gentle ping...Are there any further comments or concerns?

kkwli
kkwli approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@kkwli kkwli left a comment

Choose a reason for hiding this comment

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

LGTM

jprotze
jprotze approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@jprotze jprotze left a comment

Choose a reason for hiding this comment

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

I suggest to add a comment in kmp.h, that provides the hint of explicit copy of the struct in tests and that any changes to the struct should be applied to the tests.
Can you add this comment as part of your patch?
Overall looks good to me.

openmp/runtime/test/tasking/bug_nested_proxy_task.c
unsigned set : 1;
unsigned unused : 3;
unsigned all : 1;
#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
Copy link
Collaborator

@jprotze jprotze Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was wondering whether this test is compiler(clang) specific, or is this portable?
Since kmp.h has the same code, it make sense to replicate the pattern here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering whether this test something that is compiler(clang) specific, or is this portable?

Checked compilers in Compiler Explorer, the test works with gcc, clang, icc, icx, and zig cc. Does not seem to work with msvc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is something that would also need to be addressed in kmp.h.

Does this versioning have a correctness implication, or is this a performance optimization?
In the latter case, from my perspective, a viable approach would be hardcode the endianess, if the two macros are undefined.

It's your choice to fix it as part of this patch or whether you prefer to fix it in a separate pr:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would do it in a separate PR.

@xingxue-ibm
Copy link
Contributor Author

I suggest to add a comment in kmp.h, that provides the hint of explicit copy of the struct in tests and that any changes to the struct should be applied to the tests. Can you add this comment as part of your patch? Overall looks good to me.

Thanks very much, @jprotze! Yeah, it will be helpful to mention that test cases have an explicit copy of the struct. I will add the comment to kmp.h.

@jprotze
Copy link
Collaborator

jprotze commented Feb 6, 2024

I'm not sure whether you have the permissions to merge, if not just leave a comment and Kelvin or I will hit the merge button

@xingxue-ibm
Copy link
Contributor Author

I'm not sure whether you have the permissions to merge, if not just leave a comment and Kelvin or I will hit the merge button

Thanks, I have the permission.

@xingxue-ibm
Addressed comments.
27e7826 
* Add a comment in kmp.h to mention that some test cases contain an
  explicit copy of struct kmp_depend_info.
@xingxue-ibm xingxue-ibm merged commit 7a9b0e4 into llvm:main Feb 7, 2024
4 checks passed
@xingxue-ibm xingxue-ibm deleted the openmp-test-flip-bitfields branch February 7, 2024 20:25
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 14, 2024
@xingxue-ibm @llvmbot
[OpenMP][test]Flip bit-fields in 'struct flags' for big-endian in tes…
3e33697 
…t cases (llvm#79895)

This patch flips bit-fields in `struct flags` for big-endian in test
cases to be consistent with the definition of the structure in libomp
`kmp.h`.

(cherry picked from commit 7a9b0e4)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 16, 2024
@xingxue-ibm @tstellar
[OpenMP][test]Flip bit-fields in 'struct flags' for big-endian in tes…
cf13026 
…t cases (llvm#79895)

This patch flips bit-fields in `struct flags` for big-endian in test
cases to be consistent with the definition of the structure in libomp
`kmp.h`.

(cherry picked from commit 7a9b0e4)
@pointhex pointhex mentioned this pull request May 7, 2024
Closed
@aemerson aemerson mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jprotze jprotze jprotze approved these changes

@kkwli kkwli kkwli approved these changes

@mstorsjo mstorsjo Awaiting requested review from mstorsjo

@brad0 brad0 Awaiting requested review from brad0

@shiltian shiltian Awaiting requested review from shiltian

@iii-i iii-i Awaiting requested review from iii-i

@uweigand uweigand Awaiting requested review from uweigand

Assignees

@xingxue-ibm xingxue-ibm

Labels
openmp:libomp OpenMP host runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xingxue-ibm @iii-i @jprotze @kkwli