[OpenMP][AIX]Define struct kmp_base_tas_lock with the order of two members swapped for big-endian #79188
Conversation
xingxue-ibm
requested review from
brad0,
jdoerfert,
shiltian,
kkwli and
iii-i
openmp/runtime/src/kmp_lock.h
Outdated
| @@ -120,8 +120,15 @@ extern void __kmp_validate_locks(void); | |||
|
|
|||
| struct kmp_base_tas_lock { | |||
| // KMP_LOCK_FREE(tas) => unlocked; locked: (gtid+1) of owning thread | |||
| #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ && KMP_ARCH_PPC64 | |||
There was a problem hiding this comment.
I question whether KMP_ARCH_PPC64 should be there.
There was a problem hiding this comment.
I thought to use KMP_ARCH_PPC64 to limit the scope. This issue may also affect s390x which I believe is also big-endian. I've changed to guard the order flipping with #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ && __LP64__. @iii-i please let me know if it does not work for s390x. The reason for guarding it with __LP64__ is the original ordering works as designed in 32-bit mode both little- and big-endian. If these members are flipped for 32-bit big-endian, tags of a direct lock will take the second word while the address of the entry allocated from the indirect lock table will take the first word.
There was a problem hiding this comment.
I thought to use
KMP_ARCH_PPC64to limit the scope. This issue may also affect s390x which I believe is also big-endian.
Ya, I was also thinking about other big-endian processors that might be affected like mips64 or if sparc support is eventually added sparc64.
|
How did you find this? I would like to check if this affects s390x. If the repro is simple enough, maybe consider adding a test? |
…__ instead of KMP_ARCH_PPC64.
xingxue-ibm
force-pushed
the
openmp-lock-big-endian
branch
from
52fa82c to
d47954a
Compare
There were 36 |
|
I think this currently works on s390x by accident, since heap and executable addresses look roughly like this: The changes look good to me. I tested them on s390x and there are no test failures. |
|
Gentle ping...Are there any further comments or concerns? |
…mbers swapped for big-endian (llvm#79188) The direct lock data structure has bit `0` (the least significant bit) of the first 32-bit word set to `1` to indicate it is a direct lock. On the other hand, the first word (in 32-bit mode) or first two words (in 64-bit mode) of an indirect lock are the address of the entry allocated from the indirect lock table. The runtime checks bit `0` of the first 32-bit word to tell if this is a direct or an indirect lock. This works fine for 32-bit and 64-bit little-endian because its memory layout of a 64-bit address is (`low word`, `high word`). However, this causes problems for big-endian where the memory layout of a 64-bit address is (`high word`, `low word`). If an address of the indirect lock table entry is something like `0x110035300`, i.e., (`0x1`, `0x10035300`), it is treated as a direct lock. This patch defines `struct kmp_base_tas_lock` with the ordering of the two 32-bit members flipped for big-endian PPC64 so that when checking/setting tags in member `poll`, the second word (the low word) is used. This patch also changes places where `poll` is not already explicitly specified for checking/setting tags. (cherry picked from commit ac97562)
…mbers swapped for big-endian (llvm#79188) The direct lock data structure has bit `0` (the least significant bit) of the first 32-bit word set to `1` to indicate it is a direct lock. On the other hand, the first word (in 32-bit mode) or first two words (in 64-bit mode) of an indirect lock are the address of the entry allocated from the indirect lock table. The runtime checks bit `0` of the first 32-bit word to tell if this is a direct or an indirect lock. This works fine for 32-bit and 64-bit little-endian because its memory layout of a 64-bit address is (`low word`, `high word`). However, this causes problems for big-endian where the memory layout of a 64-bit address is (`high word`, `low word`). If an address of the indirect lock table entry is something like `0x110035300`, i.e., (`0x1`, `0x10035300`), it is treated as a direct lock. This patch defines `struct kmp_base_tas_lock` with the ordering of the two 32-bit members flipped for big-endian PPC64 so that when checking/setting tags in member `poll`, the second word (the low word) is used. This patch also changes places where `poll` is not already explicitly specified for checking/setting tags. (cherry picked from commit ac97562)
The direct lock data structure has bit
0(the least significant bit) of the first 32-bit word set to1to indicate it is a direct lock. On the other hand, the first word (in 32-bit mode) or first two words (in 64-bit mode) of an indirect lock are the address of the entry allocated from the indirect lock table. The runtime checks bit0of the first 32-bit word to tell if this is a direct or an indirect lock. This works fine for 32-bit and 64-bit little-endian because its memory layout of a 64-bit address is (low word,high word). However, this causes problems for big-endian where the memory layout of a 64-bit address is (high word,low word). If an address of the indirect lock table entry is something like0x110035300, i.e., (0x1,0x10035300), it is treated as a direct lock. This patch definesstruct kmp_base_tas_lockwith the ordering of the two 32-bit members flipped for big-endian PPC64 so that when checking/setting tags in memberpoll, the second word (the low word) is used. This patch also changes places wherepollis not already explicitly specified for checking/setting tags.