Page MenuHomePhabricator

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

Authored by Michael_Pique on Sep 24 2020, 11:55 AM.

Details

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.

Diff Detail

Event Timeline

Michael_Pique created this revision.Sep 24 2020, 11:55 AM
Michael_Pique requested review of this revision.Sep 24 2020, 11:55 AM

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

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 requested changes to this revision.Sep 25 2020, 9:23 AM

Can we use a macro to guard .size then?

This revision now requires changes to proceed.Sep 25 2020, 9:23 AM

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 planned changes to this revision.Oct 3 2020, 3:01 PM

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 added a comment.EditedOct 3 2020, 3:15 PM

.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 updated this revision to Diff 297341.EditedOct 9 2020, 2:55 PM

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.

jdoerfert resigned from this revision.Oct 10 2020, 8:31 AM
jdoerfert added a reviewer: AndreyChurbanov.
AndreyChurbanov accepted this revision.Oct 23 2020, 11:58 AM

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

This revision is now accepted and ready to land.Oct 23 2020, 11:58 AM

Can this be merged?

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 added a subscriber: kongyi.Nov 23 2020, 6:17 AM

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

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
mpique@icloud.com
+1.858.354.4391 Voice, SMS text, iMessage

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 added a comment.EditedNov 23 2020, 1:09 PM

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 requested changes to this revision.Nov 23 2020, 1:17 PM

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

This revision now requires changes to proceed.Nov 23 2020, 1:17 PM
Michael_Pique updated this revision to Diff 307184.EditedNov 23 2020, 1:20 PM

Replaced

#if __ELF__

with

#ifdef __ELF__
AndreyChurbanov accepted this revision.Nov 24 2020, 2:06 AM

LGTM

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

This revision is now accepted and ready to land.Nov 24 2020, 2:06 AM
kaz7 added a subscriber: kaz7.Nov 24 2020, 4:13 AM

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.

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