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

[David Spickett via Phabricator via Openmp-commits]

DavidSpickett added a comment.

Not trying to end up with more comments than assembly, but it might be the easiest way to do it :)



================
Comment at: openmp/runtime/src/z_Linux_asm.S:1429
+
+	// Pushing one extra register to keep the stack aligned
+	push	{r3-r11,lr}
----------------
You might want to add "for when we call pkfn below".

Since AAPCS32 says align to 4, unless at a public interface where it should be 8. Which includes calling a function.
(which evidently you knew but I did not)

Also worth saying which is the extra register.


================
Comment at: openmp/runtime/src/z_Linux_asm.S:1431
+	push	{r3-r11,lr}
+	ldrd    r4, r5, [sp, #10*4]
+
----------------
What does this load? I think p_argv and exit_frame.


================
Comment at: openmp/runtime/src/z_Linux_asm.S:1452
+	sub	sp, sp, r5, lsl #3
+	sub	r3, r3, #1
+
----------------
r3 is the arg count. So I think you are rounding up for the case where the count is an odd number, which would leave sp at 4 byte alignment? But if that's the case why do you then multiply by 8 not 4.

Because you divided by 2 in the first place? Making the calculation produce a number of 8 byte slots not 4?

Tell me where I'm going wrong there.


================
Comment at: openmp/runtime/src/z_Linux_asm.S:1465
+
+	cmp	r5, #0
+	beq	KMP_LABEL(kmp_1)
----------------
This section you doing:
```
while (argc != 0) {
   copy and argument to the stack
   argc--
}

call pkfn
```

Correct?


================
Comment at: openmp/runtime/src/z_Linux_asm.S:1467
+	beq	KMP_LABEL(kmp_1)
+	ldr	r2, [r6]
+
----------------
What does this `ldr r2` and the `ldr r3` below setup?


================
Comment at: openmp/runtime/src/z_Linux_asm.S:2062
     .size __kmp_unnamed_critical_addr,4
+#endif
 #endif /* KMP_ARCH_ARM */
----------------
And this is an elf/coff difference? No need for a equivalent coff line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138704