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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd 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)
----------------
Nice using the shrink wrapping.

Do we want to maintain stack alignment and push the frame pointer as well?


================
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
----------------
Its there a benefit to popping into r3?  Or does the thumb 1 encoding only allow popping into low registers?


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


================
Comment at: lib/builtins/arm/udivsi3.S:103
+#    if __ARM_ARCH_ISA_THUMB == 1
+	.p2align 2 /* adr requires aligned by 4 */
+	mov ip, r0
----------------
Shouldn't the label be aligned, not the code?


================
Comment at: lib/builtins/arm/udivsi3.S:106
+	adr r0, LOCAL_LABEL(div0block)
+	adds r0, #1
+#    else
----------------
Can we use or instead?


================
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.
----------------
Please use defined.


https://reviews.llvm.org/D27309