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

[Renato Golin via llvm-commits]

rengolin added a comment.

I agree with Stephen and Saleem on their comments, and I have some additional ones.

Also, avoid using push lr / pop pc, since this behaviour doesn't work across all ARM cores. There's a macro for returning that should be used.

cheers,
--renato


================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:20
@@ +19,3 @@
+// void __aeabi_cdcmpeq(double a, double b) {
+//   // __aeabi_cdcmpeq_helper takes the most significant words of a, b
+//   if (__aeabi_cdcmpeq(a, b)) { Z = 0, C = 1 }
----------------
srhines wrote:
> Double comment on this line, and the rest of the lines are formatted strangely.
you can move the comment up one line. Though, you may want to encode the logic in the example and avoid a comment.

================
Comment at: lib/builtins/arm/aeabi_cdcmp.S:31
@@ +30,3 @@
+        mov r1, r3
+        bl __aeabi_cdcmpeq_helper
+        cmp r0, #1
----------------
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.

================
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);
+}
----------------
srhines wrote:
> Couldn't this be using __builtin_isnan instead of redefining it?
or even calling it directly from the asm function?


Repository:
  rL LLVM

http://reviews.llvm.org/D12089