Details
- Reviewers
AndreyChurbanov jlpeyton
Diff Detail
- Repository
- rOMP OpenMP
Event Timeline
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?
__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.
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?
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.
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:
- Adding a line to LibompGetArchitecture.cmake to detect it.
- Adding an architecture to runtime/CMakeLists.txt (maybe X32, look for "IA32" or "INTEL64" for an example)
- Going through the cmake/* files and making sure the X32 architecture is properly handled (similar to IA32 or INTEL64)
- Adding x32 architecture to tools/lib/Platform.pm (use 32e as an example to copy and paste for x32)
- 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.