[PATCH] D27309: builtins: Add ARM Thumb1 implementation for uidiv and uidivmod

[Weiming Zhao via Phabricator via llvm-commits]

weimingz added inline comments.


================
Comment at: lib/builtins/arm/aeabi_uidivmod.S:29
+        bcc     LOCAL_LABEL(case_denom_larger)
+        push    {r0, r1, lr}
+        bl      SYMBOL_NAME(__aeabi_uidiv)
----------------
compnerd wrote:
> Nice using the shrink wrapping.
> 
> Do we want to maintain stack alignment and push the frame pointer as well?
Since thumb1 has no ldrd/strd, I think it's OK. And I see existing code like __aeabi_idivmod.S, __aeabi_cdcmp.S pushes odd number of regs.  


================
Comment at: lib/builtins/arm/aeabi_uidivmod.S:31
+        bl      SYMBOL_NAME(__aeabi_uidiv)
+        pop     {r1, r2, r3}
+        muls    r2, r2, r0 // r2 = quot * denom
----------------
compnerd wrote:
> Its there a benefit to popping into r3?  Or does the thumb 1 encoding only allow popping into low registers?
right. can only pop to R0-R7 and PC


================
Comment at: lib/builtins/arm/udivsi3.S:80
+#define BLOCK_SIZE 10
+#endif
 
----------------
compnerd wrote:
> Not your change, but do you mind creating a macro for the thumb2 case too?
yes. I did plan to do.


================
Comment at: lib/builtins/arm/udivsi3.S:103
+#    if __ARM_ARCH_ISA_THUMB == 1
+	.p2align 2 /* adr requires aligned by 4 */
+	mov ip, r0
----------------
compnerd wrote:
> Shouldn't the label be aligned, not the code?
yes, I thought the same. But I tried and didn't work. Will do more digging.


================
Comment at: lib/builtins/arm/udivsi3.S:106
+	adr r0, LOCAL_LABEL(div0block)
+	adds r0, #1
+#    else
----------------
compnerd wrote:
> Can we use or instead?
in Thumb1, ORR doesn't support immediate values.


================
Comment at: lib/builtins/arm/udivsi3.S:174
+	movs	r0, #0
+#      ifdef __ARM_EABI__
+	bl	__aeabi_idiv0 // due to relocation limit, can't use b.
----------------
compnerd wrote:
> Please use defined.
will do


https://reviews.llvm.org/D27309