[PATCH] D28016: [Builtins] [ARM] Adding Thumb1 support for fcmp

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: lib/builtins/arm/comparesf2.S:87
+#if __ARM_ARCH_ISA_THUMB == 1
+    bmi     LOCAL_LABEL(eq_identical)
+    subs    r0,     r2, r3
----------------
rengolin wrote:
> 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.
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.


================
Comment at: lib/builtins/arm/comparesf2.S:154
+    cmp     r3,         r6
+    1:
+    bls     1f
----------------
Unindent the label please.


================
Comment at: lib/builtins/arm/comparesf2.S:155
+    1:
+    bls     1f
+    movs    r0,         #1
----------------
This would need to change to `2` if you rename the next label.


================
Comment at: lib/builtins/arm/comparesf2.S:157
+    movs    r0,         #1
+    1:
+    pop     {r6, pc}
----------------
Same here.  Can you bump this to 2?  Makes it easier to follow


================
Comment at: lib/builtins/arm/comparesf2.S:178
+    lsls    r2,         r0, #1
+    lsls    r3,         r1, #1
+#if __ARM_ARCH_ISA_THUMB == 1
----------------
We didnt update the condition codes before, did we?


================
Comment at: lib/builtins/arm/comparesf2.S:179
+    lsls    r3,         r1, #1
+#if __ARM_ARCH_ISA_THUMB == 1
+    push    {r6,    lr}
----------------
Unindent the labels, and please make them monotonically increasing.  It makes it easier to spot them when jumping through the code.


================
Comment at: lib/builtins/arm/comparesf2.S:180
+#if __ARM_ARCH_ISA_THUMB == 1
+    push    {r6,    lr}
+    lsrs    r6,     r3, #1
----------------
No need to align the lr with the second column.


================
Comment at: lib/builtins/arm/comparesf2.S:216
+    pop     {r6, pc}
+
+#else
----------------
Unnecessary newline.


================
Comment at: lib/builtins/arm/comparesf2.S:243
+    lsls    r3,         r1, #1
+    movs    r0,         #0
+#if __ARM_ARCH_ISA_THUMB == 1
----------------
Did we set the condition flags previously?


================
Comment at: lib/builtins/arm/comparesf2.S:253
+    movs    r0,         #1
+    1:
+#else
----------------
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:



https://reviews.llvm.org/D28016