z_Linux_asm.S modifications for arm64 (AARCH64) for Darwin/macOS

[Michael_Pique]

Summary

Added macro calls to adjust external reference for Darwin/AARCH64 link compatibility. Deleted ".size" directive not accepted by llvm/Clang arm64 assembler. I do not know whether the .size directive is accepted, or required, by the PPC64, MIPS64, or RISCV64 assemblers as I have no way to test on those.

[lenary]

.size is accepted, but IIRC not required, in the risc-v assembler.

[arichardson]

For CHERI-extended architectures (RISCV,MIPS,AArch64), we need .size to get sensible bounds on pointers. While we don't support any of this code yet, keeping existing .size directives would be good.

[jdoerfert]

Can we use a macro to guard .size then?

[Michael_Pique]

In D88252#2295244, @jdoerfert wrote:

Can we use a macro to guard .size then?

Yes, that’s how the code in lines 170-189 (for ARCH_X86) and 191-210 (for ARCH_X86_64) handle this, by assuming that “if not DARWIN then must be LINUX*”. Both those blocks are within an “if KMP_GOMP_COMPAT” (“support for unnamed common blocks”) lines 163-212. It isn’t clear to me that the .data (and .size) code fragment we’re discussing (1744-1754) should not also be within a KMP_GOMP_COMPAT guard and merged with 163-212. A cleaner alternative to guarding individual .size directives might be a “SIZE” macro that conditionally emits or ignores .size directives. I’m not hereby proposing such a reorganization, but the “#if” method has its limitations, and keeping the very similar .data creation code together would be more readable.

If we do simply guard the line 1752 .size, would it be “if not DARWIN”, or need we exclude other OS platforms?

[Michael_Pique]

I'm new at this and could use guidance. I have tested that simply guarding the ".size" with "#if ! KMP_OS_DARWIN" is sufficient to build and link on macOS Big Sur Beta. If I should make a fresh "diff" and link it into this discussion, I'm happy to do that. It leaves unanswered, for now, whether the ".size" is or is not compatible with some Linux variants but would certainly be a step forward.

[arichardson]

.size should work on all ELF platforms. I'm not sure which platforms are supported here, so maybe !Darwin is sufficient.

Checking for __ELF__ might also be an option since that should be set by both clang and GCC ( https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html)

[Michael_Pique]

Now guarding the .size directive with #if __ELF__ as suggested. Tested on arm64 Darwin. Unable to test on arm64 Linux as have no such system available.

[AndreyChurbanov]

Accepting as there were no objections.
I cannot check the correctness myself as don't have access to these architectures.

[fxcoudert]

Can this be merged?

[lebedev.ri]

Please upload correct patch - there is no such file in the root of monorepo, so what's the omitted path to the file?
Also, subj could include the projects name tag (i'm guessing, it's [libomp]?)

[kongyi]

@Michael_Pique Can this be merged? We need this change to support ARM64 Darwin.

[Michael_Pique]

Since the proposed change shows as "Accepted" on October 23, I do not know what more I am able to do.
Let me know please if I'm blocking something inadvertently.

Michael Pique
[e-mail]
+1.858.354.4391 Voice, SMS text, iMessage

[arichardson]

This seems fine, but the patch should be uploaded against the monorepo so that it shows up as openmp/runtime/src/z_Linux_asm.S.

z_Linux_asm_ifelf.S

1752 ↗

(On Diff #297341)

This shoud be #ifdef.

[Michael_Pique]

I agree on the #ifdef instead of #if; I can make and upload a new patch if helpful. I confess I'm lost in the process.

[AndreyChurbanov]

Michael,

I am sorry, my accept action was not formal enough.

What you can do now is:

1. clone the monorepo;
2. apply your changes to openmp/runtime/src/z_Linux_asm.S, please also address @arichardson's comment on #if/#ifdef;
3. run diff, e.g: $ git diff -U 999999 > newdiff.diff
4. update the review diff with newdiff.diff (of cause, feel free to chose whatever name you like for the file instead of newdiff).

Once the updated patch is approved, it needs to be landed. Do you have commit permission to the monorepo?
If not, you can ask somebody who has the permission to commit the patch for you (e.g. I can do this).

Regards,
Andrey

[Michael_Pique]

Replaced

#if __ELF__

with

#ifdef __ELF__

[AndreyChurbanov]

LGTM

Patch would still better be updated with correct path and full context (maybe post commit).

[kaz7]

FYI.

openmp/runtime/src/z_Linux_asm.S

1744

I guess several architecture like KMP_ARCH_PPC64, KMP_ARCH_MIPS64, and KMP_ARCH_RISCV64 doesn't have KMP_PREFIX_UNDERSCORE definition here. I may be wrong, though. I'm working on VE architecture which is not merged yet. It shows compile error here after this patch.

[AndreyChurbanov]

openmp/runtime/src/z_Linux_asm.S

1744

I've submitted https://reviews.llvm.org/D92027 with attempt to fix your problem.
Could you please check if it works for you.

Thanks, Andrey

openmp/runtime/src/z_Linux_asm.S

__kmp_unnamed_critical_addr:
     .size __kmp_unnamed_critical_addr,4
 #endif /* KMP_ARCH_ARM */
 
 #if KMP_ARCH_PPC64 || KMP_ARCH_AARCH64 || KMP_ARCH_MIPS64 || KMP_ARCH_RISCV64
     .data
     .comm .gomp_critical_user_,32,8
     .data
     .align 8
-    .global __kmp_unnamed_critical_addr
-__kmp_unnamed_critical_addr:
+    .global KMP_PREFIX_UNDERSCORE(__kmp_unnamed_critical_addr)
+KMP_PREFIX_UNDERSCORE(__kmp_unnamed_critical_addr):
     .8byte .gomp_critical_user_
-    .size __kmp_unnamed_critical_addr,8
+#ifdef __ELF__
+    .size KMP_PREFIX_UNDERSCORE(__kmp_unnamed_critical_addr),8
+#endif
 #endif /* KMP_ARCH_PPC64 || KMP_ARCH_AARCH64 || KMP_ARCH_MIPS64 ||
           KMP_ARCH_RISCV64 */
 
 #if KMP_OS_LINUX
 # if KMP_ARCH_ARM
 .section .note.GNU-stack,"",%progbits
 # else
 .section .note.GNU-stack,"",@progbits
 # endif
 #endif