[ARM][test] Add more tests of two-part immediates

[benshi001]

Summary

The ARM backend breaks some specific immediates to two parts
in binary operations. And this patch adds more tests
for that.

[benshi001]

This patch does two optimization:

1. mov Rx, 0x1ffff

before:

ldr
...
.long ...

after:

mov Rx, # 0x20000
sub Rx, Rx, 1

2. add Rx, Rx, # 0x1ffff

before

ldr
add
...
.long ...

after:

add Rx, # 0x20000
sub Rx, # 1

[benshi001]

The patch depends on https://reviews.llvm.org/D83928, it will show improvements based on it.

[benshi001]

ping

[samparker]

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

3319 ↗

(On Diff #284204)

NewUseOpc2 = NewUseOpc1, same for below.

3365 ↗

(On Diff #284204)

NewUseOpc2 = NewUseOpc1, same for below.

llvm/test/CodeGen/ARM/add-sub-imm.ll

0

We should have a test run for a Thumb2 target as well. It would also be a good idea for some edge-case negative tests to exercise isSOImmTwoPartValSub some more.

[benshi001]

I have added tests for thumb2, however current patch does not apply to thumb2.

I will submit another patch for thumb2 later, since smaller patches are easy to review.

[benshi001]

Also, two edge-case negative tests to exercise isSOImmTwoPartValSub are added.

[samparker]

Thanks for the test changes.

I have added tests for thumb2, however current patch does not apply to thumb2.

I'm a bit confused then why there are Thumb2 changes in FoldImmediate?

[benshi001]

In D84100#2209170, @samparker wrote:

Thanks for the test changes.

I have added tests for thumb2, however current patch does not apply to thumb2.

I'm a bit confused then why there are Thumb2 changes in FoldImmediate?

I do not think there are Thumb2 changes in FoldImmediate. You can see NewUseOpc1 and NewUseOpc2 are identical
for all thumb2 related code. Actually the original NewUseOpc is enough for thumb2. So I should change it back (only) for thumb2 in FoldImmediate?

[samparker]

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

[benshi001]

In D84100#2209387, @samparker wrote:

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

Sorry, I made a mistake, the changes to thumb are necessary, since

• BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc), + BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc1), NewReg) .addReg(Reg1, getKillRegState(isKill)) .addImm(SOImmValV1) .add(predOps(ARMCC::AL)) .add(condCodeOp());
• UseMI.setDesc(get(NewUseOpc)); + UseMI.setDesc(get(NewUseOpc2));

This piece of code is in the critical path, all cases will walk though it.

It is necessary to substitute NewUseOpc with NewUseOpc1 & NewUseOpc2.

I will upload a new patch later, with tests of orr and eor.

[benshi001]

In D84100#2209387, @samparker wrote:

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

Sorry, I made a mistake, the changes to thumb are necessary, since

\-  BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc),
\+  BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc1),
           NewReg)
       .addReg(Reg1, getKillRegState(isKill))
       .addImm(SOImmValV1)
       .add(predOps(ARMCC::AL))
       .add(condCodeOp());
\-  UseMI.setDesc(get(NewUseOpc));
\+  UseMI.setDesc(get(NewUseOpc2));

This piece of code is in the critical path, all cases will walk though it.

It is necessary to substitute NewUseOpc with NewUseOpc1 & NewUseOpc2.

I will upload a new patch later, with tests of orr and eor.

[benshi001]

In D84100#2209387, @samparker wrote:

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

Sorry, I made a mistake, the changes to thumb are necessary, since

BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc),
         NewReg)
     .addReg(Reg1, getKillRegState(isKill))
     .addImm(SOImmValV1)
     .add(predOps(ARMCC::AL))
     .add(condCodeOp());
UseMI.setDesc(get(NewUseOpc));

is changed to

BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc1),
       NewReg)
   .addReg(Reg1, getKillRegState(isKill))
   .addImm(SOImmValV1)
   .add(predOps(ARMCC::AL))
   .add(condCodeOp());
UseMI.setDesc(get(NewUseOpc2));

This piece of code is in the critical path, all cases will walk though it.

It is necessary to substitute NewUseOpc with NewUseOpc1 & NewUseOpc2.

I will upload a new patch later, with tests of orr and eor.

[benshi001]

In D84100#2209387, @samparker wrote:

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

I have uploaded a new patch with tests of eor and orr.

The changes for Thumb2 are unavoidable due to the piece of code in the critical path.

[benshi001]

I have uploaded a new patch, in which more tests for orr and eor are added.

My patch also affect (improve) eor and orr, which I did not realize.

Currently, 12 positive test cases and 2 negative test cases are introduced, which should cover all corners.

[samparker]

I think the fact that you're having to make some Thumb2 changes to get around how the code is organised, suggests that the code should be reorganised instead. I'd suggest:

• Pre-commit the two-part-imm.ll test.
• Split the arm and thumb paths in an NFC patch, refactoring common code.
• Then make your arm changes.

[benshi001]

In D84100#2212804, @samparker wrote:

I think the fact that you're having to make some Thumb2 changes to get around how the code is organised, suggests that the code should be reorganised instead. I'd suggest:

• Pre-commit the two-part-imm.ll test.
• Split the arm and thumb paths in an NFC patch, refactoring common code.
• Then make your arm changes.

Thanks. This patch now only contains the improved test case two-part-imm.ll. And I will
submit the split of the arm and thumb paths in another NFC patch.

Really appreciate your suggestion.

[benshi001]

More tests are added for testing future patches.

[samparker]

Thanks, but please feel free to commit tests without review.

llvm/test/CodeGen/ARM/add-sub-imm.ll

-; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=arm-eabi %s -o - | FileCheck %s --check-prefix=CHECK
-
-;; Check how immediates are handled in add/sub.
-
-define i32 @sub0(i32 %0) {
-; CHECK-LABEL: sub0:
-; CHECK:       @ %bb.0:
-; CHECK-NEXT:    sub r0, r0, #23
-; CHECK-NEXT:    mov pc, lr
-  %2 = sub i32 %0, 23
-  ret i32 %2
-}
-
-define i32 @sub1(i32 %0) {
-; CHECK-LABEL: sub1:
-; CHECK:       @ %bb.0:
-; CHECK-NEXT:    ldr r1, .LCPI1_0
-; CHECK-NEXT:    add r0, r0, r1
-; CHECK-NEXT:    mov pc, lr
-; CHECK-NEXT:    .p2align 2
-; CHECK-NEXT:  @ %bb.1:
-; CHECK-NEXT:  .LCPI1_0:
-; CHECK-NEXT:    .long 4294836225 @ 0xfffe0001
-  %2 = sub i32 %0, 131071
-  ret i32 %2
-}
-
-define i32 @sub2(i32 %0) {
-; CHECK-LABEL: sub2:
-; CHECK:       @ %bb.0:
-; CHECK-NEXT:    sub r0, r0, #35
-; CHECK-NEXT:    sub r0, r0, #8960
-; CHECK-NEXT:    mov pc, lr
-  %2 = sub i32 %0, 8995
-  ret i32 %2
-}
-
-define i32 @add0(i32 %0) {
-; CHECK-LABEL: add0:
-; CHECK:       @ %bb.0:
-; CHECK-NEXT:    add r0, r0, #23
-; CHECK-NEXT:    mov pc, lr
-  %2 = add i32 %0, 23
-  ret i32 %2
-}
-
-define i32 @add1(i32 %0) {
-; CHECK-LABEL: add1:
-; CHECK:       @ %bb.0:
-; CHECK-NEXT:    ldr r1, .LCPI4_0
-; CHECK-NEXT:    add r0, r0, r1
-; CHECK-NEXT:    mov pc, lr
-; CHECK-NEXT:    .p2align 2
-; CHECK-NEXT:  @ %bb.1:
-; CHECK-NEXT:  .LCPI4_0:
-; CHECK-NEXT:    .long 131071 @ 0x1ffff
-  %2 = add i32 %0, 131071
-  ret i32 %2
-}
-
-define i32 @add2(i32 %0) {
-; CHECK-LABEL: add2:
-; CHECK:       @ %bb.0:
-; CHECK-NEXT:    add r0, r0, #8960
-; CHECK-NEXT:    add r0, r0, #2293760
-; CHECK-NEXT:    mov pc, lr
-  %2 = add i32 %0, 2302720
-  ret i32 %2
-}

llvm/test/CodeGen/ARM/two-part-imm.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=arm-eabi %s -o - | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=thumb-eabi -mcpu=arm1156t2-s -mattr=+thumb2 %s -o - \
+; RUN:   | FileCheck %s --check-prefix=CHECK-THUMB2
+
+;; Check how immediates are handled in add/sub.
+
+define i32 @sub0(i32 %0) {
+; CHECK-ARM-LABEL: sub0:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         sub r0, r0, #23
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: sub0:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         subs r0, #23
+; CHECK-THUMB2:         bx lr
+  %2 = sub i32 %0, 23
+  ret i32 %2
+}
+
+define i32 @sub1(i32 %0) {
+; CHECK-ARM-LABEL: sub1:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         ldr r1, .LCPI1_0
+; CHECK-ARM:         add r0, r0, r1
+; CHECK-ARM:         mov pc, lr
+; CHECK-ARM:         .p2align 2
+; CHECK-ARM:       @ %bb.1:
+; CHECK-ARM:         .LCPI1_0:
+; CHECK-ARM:         .long 4294836225 @ 0xfffe0001
+;
+; CHECK-THUMB2-LABEL: sub1:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2          movs r1, #1
+; CHECK-THUMB2          movt r1, #65534
+; CHECK-THUMB2          add r0, r1
+; CHECK-THUMB2          bx lr
+  %2 = sub i32 %0, 131071
+  ret i32 %2
+}
+
+define i32 @sub2(i32 %0) {
+; CHECK-ARM-LABEL: sub2:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         sub r0, r0, #35
+; CHECK-ARM:         sub r0, r0, #8960
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: sub2:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         movw r1, #8995
+; CHECK-THUMB2:         subs r0, r0, r1
+; CHECK-THUMB2:         bx lr
+  %2 = sub i32 %0, 8995
+  ret i32 %2
+}
+
+define i32 @add0(i32 %0) {
+; CHECK-ARM-LABEL: add0:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         add r0, r0, #23
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: add0:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         adds r0, #23
+; CHECK-THUMB2:         bx lr
+  %2 = add i32 %0, 23
+  ret i32 %2
+}
+
+define i32 @add1(i32 %0) {
+; CHECK-ARM-LABEL: add1:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM          ldr r1, .LCPI4_0
+; CHECK-ARM          add r0, r0, r1
+; CHECK-ARM          mov pc, lr
+; CHECK-ARM          .p2align 2
+; CHECK-ARM        @ %bb.1:
+; CHECK-ARM          .LCPI4_0:
+; CHECK-ARM          .long 131071 @ 0x1ffff
+;
+; CHECK-THUMB2-LABEL: add1:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         movw r1, #65535
+; CHECK-THUMB2:         movt r1, #1
+; CHECK-THUMB2:         add r0, r1
+; CHECK-THUMB2:         bx lr
+  %2 = add i32 %0, 131071
+  ret i32 %2
+}
+
+define i32 @add2(i32 %0) {
+; CHECK-ARM-LABEL: add2:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         add r0, r0, #8960
+; CHECK-ARM:         add r0, r0, #2293760
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: add2:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         add.w r0, r0, #2293760
+; CHECK-THUMB2:         add.w r0, r0, #8960
+; CHECK-THUMB2:         bx lr
+  %2 = add i32 %0, 2302720
+  ret i32 %2
+}
+
+define i32 @add3(i32 %0) {
+; CHECK-ARM-LABEL: add3:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         ldr r1, .LCPI6_0
+; CHECK-ARM:         add r0, r0, r1
+; CHECK-ARM:         mov pc, lr
+; CHECK-ARM:         .p2align 2
+; CHECK-ARM:       @ %bb.1:
+; CHECK-ARM:         .LCPI6_0:
+; CHECK-ARM:         .long 2096725 @ 0x1ffe55
+;
+; CHECK-THUMB2-LABEL: add3:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         movw r1, #65109
+; CHECK-THUMB2:         movt r1, #31
+; CHECK-THUMB2:         add r0, r1
+; CHECK-THUMB2:         bx lr
+  %2 = add i32 %0, 2096725
+  ret i32 %2
+}
+
+define i32 @add4(i32 %0) {
+; CHECK-ARM-LABEL: add4:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         ldr r1, .LCPI7_0
+; CHECK-ARM:         add r0, r0, r1
+; CHECK-ARM:         mov pc, lr
+; CHECK-ARM:         .p2align 2
+; CHECK-ARM:       @ %bb.1:
+; CHECK-ARM:         .LCPI7_0:
+; CHECK-ARM:         .long 8462149 @ 0x811f45
+;
+; CHECK-THUMB2-LABEL: add4:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         movw r1, #8005
+; CHECK-THUMB2:         movt r1, #129
+; CHECK-THUMB2:         add r0, r1
+; CHECK-THUMB2:         bx lr
+  %2 = add i32 %0, 8462149
+  ret i32 %2
+}
+
+define i32 @orr0(i32 %0) {
+; CHECK-ARM-LABEL: orr0:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         orr r0, r0, #8960
+; CHECK-ARM:         orr r0, r0, #2293760
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: orr0:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         orr r0, r0, #2293760
+; CHECK-THUMB2:         orr r0, r0, #8960
+; CHECK-THUMB2:         bx lr
+  %2 = or i32 %0, 2302720
+  ret i32 %2
+}
+
+define i32 @orr1(i32 %0) {
+; CHECK-ARM-LABEL: orr1:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         orr r0, r0, #23
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: orr1:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         orr r0, r0, #23
+; CHECK-THUMB2:         bx lr
+  %2 = or i32 %0, 23
+  ret i32 %2
+}
+
+define i32 @orr2(i32 %0) {
+; CHECK-ARM-LABEL: orr2:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         ldr r1, .LCPI10_0
+; CHECK-ARM:         orr r0, r0, r1
+; CHECK-ARM:         mov pc, lr
+; CHECK-ARM:         .p2align 2
+; CHECK-ARM:       @ %bb.1:
+; CHECK-ARM:         .LCPI10_0:
+; CHECK-ARM:         .long 131071 @ 0x1ffff
+;
+; CHECK-THUMB2-LABEL: orr2:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2          movs r1, #1
+; CHECK-THUMB2          movt r1, #65534
+; CHECK-THUMB2          orr r0, r1
+; CHECK-THUMB2          bx lr
+  %2 = or i32 %0, 131071
+  ret i32 %2
+}
+
+define i32 @eor0(i32 %0) {
+; CHECK-ARM-LABEL: eor0:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         eor r0, r0, #8960
+; CHECK-ARM:         eor r0, r0, #2293760
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: eor0:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         eor r0, r0, #2293760
+; CHECK-THUMB2:         eor r0, r0, #8960
+; CHECK-THUMB2:         bx lr
+  %2 = xor i32 %0, 2302720
+  ret i32 %2
+}
+
+define i32 @eor1(i32 %0) {
+; CHECK-ARM-LABEL: eor1:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         eor r0, r0, #23
+; CHECK-ARM:         mov pc, lr
+;
+; CHECK-THUMB2-LABEL: eor1:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2:         eor r0, r0, #23
+; CHECK-THUMB2:         bx lr
+  %2 = xor i32 %0, 23
+  ret i32 %2
+}
+
+define i32 @eor2(i32 %0) {
+; CHECK-ARM-LABEL: eor2:
+; CHECK-ARM:       @ %bb.0:
+; CHECK-ARM:         ldr r1, .LCPI13_0
+; CHECK-ARM:         eor r0, r0, r1
+; CHECK-ARM:         mov pc, lr
+; CHECK-ARM:         .p2align 2
+; CHECK-ARM:       @ %bb.1:
+; CHECK-ARM:         .LCPI13_0:
+; CHECK-ARM:         .long 131071 @ 0x1ffff
+;
+; CHECK-THUMB2-LABEL: eor2:
+; CHECK-THUMB2:       @ %bb.0:
+; CHECK-THUMB2          movs r1, #1
+; CHECK-THUMB2          movt r1, #65534
+; CHECK-THUMB2          eor r0, r1
+; CHECK-THUMB2          bx lr
+  %2 = xor i32 %0, 131071
+  ret i32 %2
+}