[Builtins] [ARM] Adding Thumb1 support for fcmp

[weimingz]

Summary

Mainly translate IT block into cmp/branch for functions in comparesf2.S

[rengolin]

Hi Weiming,

I don't think you can use MSR/MRS like that. They're not available anywhere Thumb1 is available (AFAIK, only M0), so this will only work on M0, not on anything else where __ARM_ARCH_ISA_THUMB == 1 will be true.

Also, MRS/MSR is an additional 4 cycles each, and by the number of uses you have here, this function will be several times slower than it should.

cheers,
--renato

lib/builtins/arm/comparesf2.S

87 ↗

(On Diff #82203)

Why do you use named labels here and not below? I'm fine with either standard, would just be good to be consistent, I think.

My personal preference is to use 1f for everything, as it's clear what you want from the local context and the notation is slightly less cluttered.

[weimingz]

Hi Renato,

I agree with your comments on msr/mrs. It's ugly and may only work for armv6m.
The nasty thing is I can't find a way to convert "mvn r0, r1, asr #31" to Thumb1 without clobbering the apsr.

Any ideas? Otherwise, I think we have to just use the C version.

[rengolin]

In D28016#630146, @weimingz wrote:

I agree with your comments on msr/mrs. It's ugly and may only work for armv6m.
The nasty thing is I can't find a way to convert "mvn r0, r1, asr #31" to Thumb1 without clobbering the apsr.

Any ideas? Otherwise, I think we have to just use the C version.

I think the problem here is trying to fit a Thumb1 flow around a Thumb2 one. Maybe if we had a completely separate version in pure Thumb1, and the ifdef just chooses it, we could have a cleaner implementation.

The way I do this is to write in C, compile to assembly and then optimise by hand, add comments, nice labels, etc.

Given the number of alternative we could have (T1/T2/ARM vs, soft/hard), it could be better if we had different files with each one and the main file (this one) defines the function alias depending on the defined pre-processor symbols. Though, this change can be on a separate patch, later.

cheers,
--renato

[compnerd]

lib/builtins/arm/comparesf2.S

87 ↗

(On Diff #82203)

I tend to prefer the temporary local labels (e.g. the 1) that you are suggesting as well. However, I think I would rather see a switch to that across all the various T1 versions that have been added recently rather than just this one.

154 ↗

(On Diff #82203)

Unindent the label please.

155 ↗

(On Diff #82203)

This would need to change to 2 if you rename the next label.

157 ↗

(On Diff #82203)

Same here. Can you bump this to 2? Makes it easier to follow

178 ↗

(On Diff #82203)

We didnt update the condition codes before, did we?

179 ↗

(On Diff #82203)

Unindent the labels, and please make them monotonically increasing. It makes it easier to spot them when jumping through the code.

180 ↗

(On Diff #82203)

No need to align the lr with the second column.

216 ↗

(On Diff #82203)

Unnecessary newline.

243 ↗

(On Diff #82203)

Did we set the condition flags previously?

253 ↗

(On Diff #82203)

Hmm, won't this possibly execute both sides of the ITE which the T2 path doesnt do IIRC. Shouldnt we write this as:

  bhi 1f
  cmp r3, r1
  b 2f
1:
  movs r0, #1
  @ b 2f @ for symmetry
2:

[weimingz]

By tracking the original implementation, I think we can avoid the msr/mrs. Also address some formatting issues.

[weimingz]

lib/builtins/arm/comparesf2.S

243 ↗

(On Diff #82203)

thumb1 has to use the "s" suffixed instructions. The flags will be reset by the "cmp" anyway, so won't affect the correctness.

[weimingz]

lib/builtins/arm/comparesf2.S

253 ↗

(On Diff #82203)

The original code is not an "if-else". the "cmpls" will update the flags as well. It's a short-circuited comparison like " r0 = (r2 < 0xff0000000 || r3 < 0xff000000) ? r0 : 1

[weimingz]

Missed a format issue in previous upload. Sorry~

[weimingz]

Manually ran the unit test and passed

[rengolin]

I'm happy if @compnerd is. :)

[compnerd]

I think that the inline comments are slightly too dense to ready comfortably, especially with the amount of whitespace the rest of the file uses. But thats a minor thing.

[weimingz]

I can fix the comments

lib/builtins/arm/comparesf2.S

196 ↗

(On Diff #82585)

do you mean comments like this and line 214 too?

[compnerd]

Yeah, those felt a slight bit dense while I was trying to read through the assembly. (BTW, no need to update the patch if you adjust the comments before committing).

[compnerd]

Ugh, I missed the removal of the two lines in __gtsf2 in the !T1 case. Ill adjust it as Im working on adding support for *hf targets.

[weimingz]

Ugh, I missed them in during clean up in diff 2. Very sorry!

compiler-rt/trunk/lib/builtins/arm/aeabi_fcmp.S

 #define DEFINE_AEABI_FCMP(cond)                            \
         .syntax unified                          SEPARATOR \
         .p2align 2                               SEPARATOR \
 DEFINE_COMPILERRT_FUNCTION(__aeabi_fcmp ## cond)           \
         push      { r4, lr }                     SEPARATOR \
         bl        SYMBOL_NAME(__ ## cond ## sf2) SEPARATOR \
         cmp       r0, #0                         SEPARATOR \
         b ## cond 1f                             SEPARATOR \
-        mov       r0, #0                         SEPARATOR \
+        movs      r0, #0                         SEPARATOR \
         pop       { r4, pc }                     SEPARATOR \
 1:                                               SEPARATOR \
-        mov       r0, #1                         SEPARATOR \
+        movs      r0, #1                         SEPARATOR \
         pop       { r4, pc }                     SEPARATOR \
 END_COMPILERRT_FUNCTION(__aeabi_fcmp ## cond)
 
 DEFINE_AEABI_FCMP(eq)
 DEFINE_AEABI_FCMP(lt)
 DEFINE_AEABI_FCMP(le)
 DEFINE_AEABI_FCMP(ge)
 DEFINE_AEABI_FCMP(gt)
 
 NO_EXEC_STACK_DIRECTIVE
 

compiler-rt/trunk/lib/builtins/arm/comparesf2.S

 #if __ARM_ARCH_ISA_THUMB == 2
 .thumb
 #endif
 
 .p2align 2
 DEFINE_COMPILERRT_FUNCTION(__eqsf2)
     // Make copies of a and b with the sign bit shifted off the top.  These will
     // be used to detect zeros and NaNs.
+#if __ARM_ARCH_ISA_THUMB == 1
+    push    {r6, lr}
+    lsls    r2,         r0, #1
+    lsls    r3,         r1, #1
+#else
     mov     r2,         r0, lsl #1
     mov     r3,         r1, lsl #1
+#endif
 
     // We do the comparison in three stages (ignoring NaN values for the time
     // being).  First, we orr the absolute values of a and b; this sets the Z
     // flag if both a and b are zero (of either sign).  The shift of r3 doesn't
     // effect this at all, but it *does* make sure that the C flag is clear for
     // the subsequent operations.
+#if __ARM_ARCH_ISA_THUMB == 1
+    lsrs    r6,     r3, #1
+    orrs    r6,     r2, r6
+#else
     orrs    r12,    r2, r3, lsr #1
-
+#endif
     // Next, we check if a and b have the same or different signs.  If they have
     // opposite signs, this eor will set the N flag.
+#if __ARM_ARCH_ISA_THUMB == 1
+    beq     1f
+    movs    r6,     r0
+    eors    r6,     r1
+1:
+#else
     it ne
     eorsne  r12,    r0, r1
+#endif
 
     // If a and b are equal (either both zeros or bit identical; again, we're
     // ignoring NaNs for now), this subtract will zero out r0.  If they have the
     // same sign, the flags are updated as they would be for a comparison of the
     // absolute values of a and b.
+#if __ARM_ARCH_ISA_THUMB == 1
+    bmi     1f
+    subs    r0,     r2, r3
+1:
+#else
     it pl
     subspl  r0,     r2, r3
+#endif
 
     // If a is smaller in magnitude than b and both have the same sign, place
     // the negation of the sign of b in r0.  Thus, if both are negative and
     // a > b, this sets r0 to 0; if both are positive and a < b, this sets
     // r0 to -1.
     //
     // This is also done if a and b have opposite signs and are not both zero,
     // because in that case the subtract was not performed and the C flag is
     // still clear from the shift argument in orrs; if a is positive and b
     // negative, this places 0 in r0; if a is negative and b positive, -1 is
     // placed in r0.
+#if __ARM_ARCH_ISA_THUMB == 1
+    bhs     1f
+    // Here if a and b have the same sign and absA < absB, the result is thus
+    // b < 0 ? 1 : -1. Same if a and b have the opposite sign (ignoring Nan).
+    movs    r0,         #1
+    lsrs    r1,         #31
+    bne     LOCAL_LABEL(CHECK_NAN)
+    negs    r0,         r0
+    b       LOCAL_LABEL(CHECK_NAN)
+1:
+#else
     it lo
     mvnlo   r0,         r1, asr #31
+#endif
 
     // If a is greater in magnitude than b and both have the same sign, place
     // the sign of b in r0.  Thus, if both are negative and a < b, -1 is placed
     // in r0, which is the desired result.  Conversely, if both are positive
     // and a > b, zero is placed in r0.
+#if __ARM_ARCH_ISA_THUMB == 1
+    bls     1f
+    // Here both have the same sign and absA > absB.
+    movs    r0,         #1
+    lsrs    r1,         #31
+    beq     LOCAL_LABEL(CHECK_NAN)
+    negs    r0, r0
+1:
+#else
     it hi
     movhi   r0,         r1, asr #31
+#endif
 
     // If you've been keeping track, at this point r0 contains -1 if a < b and
     // 0 if a >= b.  All that remains to be done is to set it to 1 if a > b.
     // If a == b, then the Z flag is set, so we can get the correct final value
     // into r0 by simply or'ing with 1 if Z is clear.
+    // For Thumb-1, r0 contains -1 if a < b, 0 if a > b and 0 if a == b.
+#if __ARM_ARCH_ISA_THUMB != 1
     it ne
     orrne   r0,     r0, #1
+#endif
 
     // Finally, we need to deal with NaNs.  If either argument is NaN, replace
     // the value in r0 with 1.
+#if __ARM_ARCH_ISA_THUMB == 1
+LOCAL_LABEL(CHECK_NAN):
+    movs    r6,         #0xff
+    lsls    r6,         #24
+    cmp     r2,         r6
+    bhi     1f
+    cmp     r3,         r6
+1:
+    bls     2f
+    movs    r0,         #1
+2:
+    pop     {r6, pc}
+#else
     cmp     r2,         #0xff000000
     ite ls
     cmpls   r3,         #0xff000000
     movhi   r0,         #1
     JMP(lr)
+#endif
 END_COMPILERRT_FUNCTION(__eqsf2)
 DEFINE_COMPILERRT_FUNCTION_ALIAS(__lesf2, __eqsf2)
 DEFINE_COMPILERRT_FUNCTION_ALIAS(__ltsf2, __eqsf2)
 DEFINE_COMPILERRT_FUNCTION_ALIAS(__nesf2, __eqsf2)
 
 .p2align 2
 DEFINE_COMPILERRT_FUNCTION(__gtsf2)
     // Identical to the preceding except in that we return -1 for NaN values.
-    // Given that the two paths share so much code, one might be tempted to 
+    // Given that the two paths share so much code, one might be tempted to
     // unify them; however, the extra code needed to do so makes the code size
     // to performance tradeoff very hard to justify for such small functions.
-    mov     r2,         r0, lsl #1
-    mov     r3,         r1, lsl #1
+#if __ARM_ARCH_ISA_THUMB == 1
+    push    {r6, lr}
+    lsls    r2,        r0, #1
+    lsls    r3,        r1, #1
+    lsrs    r6,        r3, #1
+    orrs    r6,        r2, r6
+    beq     1f
+    movs    r6,        r0
+    eors    r6,        r1
+1:
+    bmi     2f
+    subs    r0,        r2, r3
+2:
+    bhs     3f
+    movs    r0,        #1
+    lsrs    r1,        #31
+    bne     LOCAL_LABEL(CHECK_NAN_2)
+    negs    r0, r0
+    b       LOCAL_LABEL(CHECK_NAN_2)
+3:
+    bls     4f
+    movs    r0,         #1
+    lsrs    r1,         #31
+    beq     LOCAL_LABEL(CHECK_NAN_2)
+    negs    r0, r0
+4:
+LOCAL_LABEL(CHECK_NAN_2):
+    movs    r6,         #0xff
+    lsls    r6,         #24
+    cmp     r2,         r6
+    bhi     5f
+    cmp     r3,         r6
+5:
+    bls     6f
+    movs    r0,         #1
+    negs    r0,         r0
+6:
+    pop     {r6, pc}
+#else
     orrs    r12,    r2, r3, lsr #1
     it ne
     eorsne  r12,    r0, r1
     it pl
     subspl  r0,     r2, r3
     it lo
     mvnlo   r0,         r1, asr #31
     it hi
     movhi   r0,         r1, asr #31
     it ne
     orrne   r0,     r0, #1
     cmp     r2,         #0xff000000
     ite ls
     cmpls   r3,         #0xff000000
     movhi   r0,         #-1
     JMP(lr)
+#endif
 END_COMPILERRT_FUNCTION(__gtsf2)
 DEFINE_COMPILERRT_FUNCTION_ALIAS(__gesf2, __gtsf2)
 
 .p2align 2
 DEFINE_COMPILERRT_FUNCTION(__unordsf2)
     // Return 1 for NaN values, 0 otherwise.
-    mov     r2,         r0, lsl #1
-    mov     r3,         r1, lsl #1
-    mov     r0,         #0
+    lsls    r2,         r0, #1
+    lsls    r3,         r1, #1
+    movs    r0,         #0
+#if __ARM_ARCH_ISA_THUMB == 1
+    movs    r1,         #0xff
+    lsls    r1,         #24
+    cmp     r2,         r1
+    bhi     1f
+    cmp     r3,         r1
+1:
+    bls     2f
+    movs    r0,         #1
+2:
+#else
     cmp     r2,         #0xff000000
     ite ls
     cmpls   r3,         #0xff000000
     movhi   r0,         #1
+#endif
     JMP(lr)
 END_COMPILERRT_FUNCTION(__unordsf2)
 
 DEFINE_AEABI_FUNCTION_ALIAS(__aeabi_fcmpun, __unordsf2)
 
 NO_EXEC_STACK_DIRECTIVE