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

[Josh Gao via llvm-commits]

jmgao added inline comments.

================
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
----------------
compnerd wrote:
> 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.
If I'm reading it correctly, this function is not actually required to preserve the argument registers, only the c?cmple ones are. We just happen to need them after checking for NaNs.

I added the comment to c?cmple.

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:31
@@ +30,3 @@
+        mov r1, r3
+        bl __aeabi_cdcmpeq_helper
+        cmp r0, #1
----------------
rengolin wrote:
> compnerd wrote:
> > Not a big fan of this helper name.  It simply checks for NaNs.
> me neither. If it's small enough, you can either write in asm or move to a separate static function in this file with a more descriptive name.
Renamed it to __aeabi_cdcmpeq_check_nan

================
Comment at: lib/builtins/arm/aeabi_cdcmpeq_helper.c:19
@@ +18,3 @@
+int __aeabi_cdcmpeq_helper(int32_t a, int32_t b) {
+    return isnan_d(a) || isnan_d(b);
+}
----------------
rengolin wrote:
> srhines wrote:
> > Couldn't this be using __builtin_isnan instead of redefining it?
> or even calling it directly from the asm function?
Doesn't __builtin_isnan do overloading via magic and/or generic macros? Is there an actual symbol I can jump to?

================
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;
+}
----------------
compnerd wrote:
> Why the mask?  Don't you need to check if the mantissa to determine if it is a NaN?
Yeah, this is wrong, I replaced it with a __builtin_nan instead


http://reviews.llvm.org/D12089