[Openmp-commits] [PATCH] D137827: [openmp] Use z_Linux_asm.S to provide __kmp_invoke_microtask with Clang for Windows/aarch64

Thu Nov 24 05:39:19 PST 2022

mstorsjo added a comment.

In D137827#3949184 <https://reviews.llvm.org/D137827#3949184>, @DavidSpickett wrote:

>> I'm wrapping the .comm directive since the alignment in that directive is interpreted differently between ELF and COFF.
>
> I didn't know this was a thing. Can you cite some documentation for the `.comm` behaviour?

https://sourceware.org/binutils/docs/as/Comm.html - which says:

> When using ELF or (as a GNU extension) PE, the .comm directive takes an optional third argument. This is the desired alignment of the symbol, specified for ELF as a byte boundary (for example, an alignment of 16 means that the least significant 4 bits of the address should be zero), and for PE as a power of two (for example, an alignment of 5 means aligned to a 32-byte boundary).

> I assume this passes whatever OpenMP testing we have. I'd be ok approving it on that basis.

Yes - this, together with D137748 <https://reviews.llvm.org/D137748> and D137772 <https://reviews.llvm.org/D137772>, makes all tests pass in the mingw/aarch64 build configuration.

================
Comment at: openmp/runtime/src/z_Linux_asm.S:153
+	ALIGN 2
+	.globl KMP_PREFIX_UNDERSCORE(\proc)
+KMP_PREFIX_UNDERSCORE(\proc):
----------------
DavidSpickett wrote:
> I guess this is 2 byte function start alignment? I'd have guessed 4 so maybe I'm wrong.
No, the `ALIGN` macro takes a power of two - so this sets 4 byte alignment.

(The `ALIGN` macro is probably used because the `.align` directive is inconsistent across platforms - that's why there are the `.p2align` and `.balign` directives too. But changing the existing code to use them instead would be a different change.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137827/new/

https://reviews.llvm.org/D137827