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

[Saleem Abdulrasool via llvm-commits]

compnerd added inline comments.

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:12
@@ +11,3 @@
+
+#ifndef __LITTLE_ENDIAN__
+#error FIXME
----------------
Is this actually portable?  Can you rewrite this as:

    __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__

or even:

    __ARMEB__

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:13
@@ +12,3 @@
+#ifndef __LITTLE_ENDIAN__
+#error FIXME
+#endif
----------------
Please replace FIXME with a slightly better message.  How about something like:

    #error big endian support not implemented

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:21
@@ +20,3 @@
+//   // __aeabi_cdcmpeq_helper takes the most significant words of a, b
+//   if (__aeabi_cdcmpeq(a, b)) { Z = 0, C = 1 }
+//   else { __aeabi_cdcmple(a, b); }
----------------
Did you mean the _helper here?

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:28
@@ +27,3 @@
+DEFINE_COMPILERRT_FUNCTION(__aeabi_cdcmpeq)
+        push {r0, r1, r2, r3, lr}
+        mov r0, r1
----------------
Please add a comment that as per the RTABI, this function *must* save/restore the core registers except ip, lr, and CPSR.  And a note about lr since its not immediately obvious that it is for compactness.

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:31
@@ +30,3 @@
+        mov r1, r3
+        bl __aeabi_cdcmpeq_helper
+        cmp r0, #1
----------------
Not a big fan of this helper name.  It simply checks for NaNs.

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

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

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:83
@@ +82,3 @@
+        .p2align 2
+DEFINE_COMPILERRT_FUNCTION(__aeabi_cdrcmple)
+        push {lr}
----------------
Why not just swap and tail call?

================
Comment at: lib/builtins/arm/aeabi_cdcmpeq_helper.c:13
@@ +12,3 @@
+static int isnan_d(int32_t a) {
+    return (a & 0x7ff << 20) == 0x7ff << 20;
+}
----------------
Don't you need to check the mantissa to determine if it is a NaN?

================
Comment at: lib/builtins/arm/aeabi_cfcmpeq_helper.c:13
@@ +12,3 @@
+static int isnan_f(int32_t a) {
+    return (a & 0xff << 23) == 0xff << 23;
+}
----------------
Why the mask?  Don't you need to check if the mantissa to determine if it is a NaN?


Repository:
  rL LLVM

http://reviews.llvm.org/D12089