This is an archive of the discontinued LLVM Phabricator instance.

make libomp work on amd64 x32 ABI
Needs ReviewPublic

Authored by The_Bishop on Feb 1 2018, 6:58 AM.

Details

Diff Detail

Repository
rOMP OpenMP

Event Timeline

The_Bishop created this revision.Feb 1 2018, 6:58 AM
Hahnfeld added a subscriber: Hahnfeld.

Hi, please add context to patches you upload as documented in https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface and format your change with clang-format if possible.

Also, I'm not sure how __ILP32__ can be set if KMP_ARCH_X86_64 is true. Maybe you need to pass -DLIBOMP_ARCH=i386 to CMake?

The_Bishop added a comment.EditedFeb 1 2018, 9:09 AM

__ILP32__ is set by the compiler when the code is to be compiled for AMD64 in X32 mode. This means that the code generator may use the 64bit features but addresses are 32bit.

I'm not sure about the cmake flag because the X32 mode is neither i386 nor amd64. And the proposed patch will still be necessary.

I will add more context next time, sorry (my first patch). But i can not see where i violated the clang formatting style.

__ILP32__ is set by the compiler when the code is to be compiled for AMD64 in X32 mode. This means that the code generator may use the 64bit features but addresses are 32bit.

Ah, now I got you!

I will add more context next time, sorry (my first patch). But i can not see where i violated the clang formatting style.

No problem, we've all been there ;-) I'd guess that clang-format would remove the spaces in || ( KMP_ARCH_X86_64 && __ILP32__ )...

Well, i found a major flaw in my patch - i should have made X32 an arch on it's own.
Did this now, can i update my patch or do i have to abandon and start over?

Well, i found a major flaw in my patch - i should have made X32 an arch on it's own.
Did this now, can i update my patch or do i have to abandon and start over?

No, you can just "Update Diff" (second button in the right menu)

Complete rework to make X32 an arch on it's own.

The_Bishop added a comment.EditedFeb 1 2018, 11:41 AM

Don't know how cmake handles this __ILP32__ thing, so the patch to runtime/cmake/LibompMicroTests.cmake looks a bit crude because the dependency filter is more broad than strictly necessary.

The_Bishop updated this revision to Diff 132461.Feb 1 2018, 1:07 PM
The_Bishop edited the summary of this revision. (Show Details)

removed a stray c&p line ^^

I guess first and foremost, we need to make sure that we want KMP_ARCH_X86, KMP_ARCH_X86_64, and KMP_ARCH_X86_X32 to be mutually exclusive macros i.e., different architectures as far as CMake is concerned. If we want them to be mutually exclusive, then there is more work to be done including but not limited to:

  1. Adding a line to LibompGetArchitecture.cmake to detect it.
  2. Adding an architecture to runtime/CMakeLists.txt (maybe X32, look for "IA32" or "INTEL64" for an example)
  3. Going through the cmake/* files and making sure the X32 architecture is properly handled (similar to IA32 or INTEL64)
  4. Adding x32 architecture to tools/lib/Platform.pm (use 32e as an example to copy and paste for x32)
  5. Go through the source code again and make sure each KMP_ARCH_X86 or KMP_ARCH_X86_64 section makes sense for KMP_ARCH_X86_X32. For example, in kmp_os.h, there are TCR_* macros which depend on the pointer size (similar for KMP_COMPARE_AND_STORE_PTR()). Since it appears that X32 uses four byte pointers it should be paired with KMP_ARCH_X86 in these cases, but it uses elements of the X86_64 ABI in other situations as well.

you are right, i will stop spammin' until i got this stuff fixed. thx for the hints.