[PATCH] D12089: Implement __aeabi_c{d,f}{cmpeq,cmple,rcmple}.

[Josh Gao via llvm-commits]

jmgao added a comment.

Thanks for the comments, I'll fix them up and have an updated patch shortly


================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:12
@@ +11,3 @@
+
+#ifndef __LITTLE_ENDIAN__
+#error FIXME
----------------
compnerd wrote:
> Is this actually portable?  Can you rewrite this as:
> 
>     __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> 
> or even:
> 
>     __ARMEB__
Fixed

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:13
@@ +12,3 @@
+#ifndef __LITTLE_ENDIAN__
+#error FIXME
+#endif
----------------
compnerd wrote:
> Please replace FIXME with a slightly better message.  How about something like:
> 
>     #error big endian support not implemented
Fixed

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:33
@@ +32,3 @@
+        cmp r0, #1
+        bne 1f
+        msr CPSR_f, #APSR_C
----------------
compnerd wrote:
> Cant you hoist the pop {r0-r3} here?
Yep, done

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:39
@@ +38,3 @@
+        pop {r0, r1, r2, r3}
+        bl __aeabi_cdcmple
+        pop {pc}
----------------
compnerd wrote:
> Indicate that this is safe now since we have ruled out NaNs, so cmple can't trap. 
Fixed

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:46
@@ +45,3 @@
+//   if (__aeabi_dcmplt(a, b)) {
+//     Z = 1; C = 0;
+//   } else if (__aeabi_dcmpeq(a, b)) {
----------------
srhines wrote:
> These comments don't match the implementation below, so something seems wrong.
Oops, the comments are incorrect. Fixed

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:83
@@ +82,3 @@
+        .p2align 2
+DEFINE_COMPILERRT_FUNCTION(__aeabi_cdrcmple)
+        push {lr}
----------------
compnerd wrote:
> Why not just swap and tail call?
I misinterpreted the RTABI doc, I was applying the "3 way status returning functions preserve all registers except ip, lr, cpsr" to the reversed 3 way functions as well.

Fixed

================
Comment at: test/builtins/Unit/arm/call_apsr.S:12
@@ +11,3 @@
+    ldr ip, [sp, #4]
+	blx ip
+	mrs r0, apsr
----------------
indentation broken here


Repository:
  rL LLVM

http://reviews.llvm.org/D12089