[PATCH] D120234: [ARM] Make i32 ISD::ABS Legal instead of pattern matching during isel.

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @craig.topper, I'll be honest I haven't reviewed the code changes, but just looking at the tests it's not immediately obvious (to me at least!) what the benefit of this change is? Of course I could be missing something subtle here!



================
Comment at: llvm/test/CodeGen/ARM/iabs.ll:50
+
+define i32 @test4(i32 %a) {
+; CHECK-LABEL: test4:
----------------
I tested this out without your patch and I got this:

        eor     r1, r0, r0, asr #31
        rsb     r0, r1, r0, asr #31
        bx      lr

which has the same number of instructions, but without any control flow.


================
Comment at: llvm/test/CodeGen/Thumb2/abs.ll:44
 ; CHECKT2:       @ %bb.0:
-; CHECKT2-NEXT:    eor.w r1, r0, r0, asr #31
-; CHECKT2-NEXT:    rsb r0, r1, r0, asr #31
+; CHECKT2-NEXT:    cmp r0, #0
+; CHECKT2-NEXT:    it gt
----------------
It feels like this is actually now worse than before due to the presence of hardware control flow? Previously the eor.w and rsb instructions had no dependency on flags and also didn't set any.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120234/new/

https://reviews.llvm.org/D120234