[Openmp-commits] [PATCH] D138704: RFC: [openmp] Provide an assembly implementation of __kmp_invoke_microtask on ARM

[Martin Storsjö via Phabricator via Openmp-commits]

mstorsjo added a comment.

In D138704#3956913 <https://reviews.llvm.org/D138704#3956913>, @DavidSpickett wrote:

> Assembly looks good to me.

Thanks for reviewing!

> Now we need some way to tell if earlier Arm needs to be supported. Is there other existing Arm assembly for OpenMP? That would give a clue, might have to go to the git blame to find the intent when it was first added. I'm no expert on the very old architectures but I can help check it for things that look "new" or "old" (for lack of better words).

No, as far as I can see, there's no other existing assembly for ARM here.

AFAIK this implementation should be interworking-safe on armv5 and above, but wouldn't necessarily work as-is for armv4t. It probably won't assemble in thumb1 mode either.



================
Comment at: openmp/runtime/src/z_Linux_asm.S:1447
+	// We strictly need 4*(argc-2) bytes, but allocate 4*argc for
+	// simplicity (to avoid needing to handle the argc<2 cases).
+	// We align the number of bytes allocated to 8 bytes, to keep the
----------------
DavidSpickett wrote:
> mstorsjo wrote:
> > DavidSpickett wrote:
> > > What is the `-2` in `argc-2`? (or rather what would it be)
> > We pass 2 arguments out of `argc` in registers (while the other 2 register parameters are taken by `tid` and `gtid`). In the case of aarch64 with 8 register parameters, they'd need space for `argc-6` arguments on the stack.
> How about:
> ```
> // We strictly need 4*(argc-2) bytes (2 arguments are passed in registers)
> // but allocate 4*argc for....
> ```
Thanks, that sounds good to me, I'll amend the patch accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138704