[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

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

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

Unindent the label please.

155

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

157

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

178

We didnt update the condition codes before, did we?

179

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

180

No need to align the lr with the second column.

216

Unnecessary newline.

243

Did we set the condition flags previously?

253

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

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

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

194

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!

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
 

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     LOCAL_LABEL(eq_sign)
+    movs    r6,     r0
+    eors    r6,     r1
+LOCAL_LABEL(eq_sign):
+#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     LOCAL_LABEL(eq_identical)
+    subs    r0,     r2, r3
+LOCAL_LABEL(eq_identical):
+#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
+    mrs     r6,         apsr
+    asrs    r0,         r1, #31
+    mvns    r0,         r0
+    msr     apsr,       r6
+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
+    mrs     r6,         apsr
+    asrs    r0,         r1, #31
+    msr     apsr,       r6
+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.
+#if __ARM_ARCH_ISA_THUMB == 1
+    beq     1f
+    movs    r6,     #1
+    orrs    r0,     r6
+1:
+#else
     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
+    movs    r6,         #0xff
+    lsls    r6,         #24
+    cmp     r2,         r6
+    bhi     1f
+    cmp     r3,         r6
+    1:
+    bls     1f
+    movs    r0,         #1
+    1:
+    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 
     // 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
+    lsls    r2,         r0, #1
+    lsls    r3,         r1, #1
+#if __ARM_ARCH_ISA_THUMB == 1
+    push    {r6,    lr}
+    lsrs    r6,     r3, #1
+    orrs    r6,     r2, r6
+    beq     1f
+    movs    r6,     r0
+    eors    r6,     r1
+    1:
+    bmi     1f
+    subs    r0,     r2, r3
+    1:
+    bhs     1f
+    mrs     r6,     apsr
+    asrs    r0,     r1, #31
+    mvns    r0,     r0
+    msr     apsr,   r6
+    1:
+    bls     1f
+    mrs     r6,     apsr
+    asrs    r0,     r1, #31
+    msr     apsr,   r6
+    1:
+    beq     1f
+    movs    r6,     #1
+    orrs    r0,     r6
+    1:
+    movs    r6,     #0xff
+    lsls    r6,     #24
+    cmp     r2,     r6
+    bhi     1f
+    cmp     r3,     r6
+    1:
+    bls     1f
+    movs    r0,      #0
+    mvns    r0,      r0
+    1:
+    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     1f
+    movs    r0,         #1
+    1:
+#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