[ARM] Add Thumb2 ADD with SP narrowing from 3 operand to 2

[rengolin]

What about the new immediate range tests? Are they covered somewhere else?

[scott-0]

Thanks for the review!

There is 'add sp, sp, #4' in basic-thumb-instructions.s and 'add sp, sp, #0x1fe0000' in basic-thumb2-instructions.s

I'm also going to make thumb_rewrites.s pass for Thumb2, too; but it requires an additional fix.

[By the way, I considered writing the 'TryTransform' logic as just one 'if':

if (Mnemonic != "add" ||
    !(Op3Reg == ARM::PC || Op4Reg == ARM::PC ||
      (Op5.isReg() && Op5.getReg() == ARM::PC) ||
      ((Op3Reg == ARM::SP || Op4Reg == ARM::SP ||
        (Op5.isReg() && Op5.getReg() == ARM::SP)) &&
       !(Op3Reg == ARM::SP && Op4Reg == ARM::SP &&
         Op5.isImm() && !Op5.isImm0_1020s4()))))
  return;

but I thought it was a bit easier to follow with the intermediate steps.]

[rengolin]

In D11131#203644, @scott-0 wrote:

Thanks for the review!

There is 'add sp, sp, #4' in basic-thumb-instructions.s and 'add sp, sp, #0x1fe0000' in basic-thumb2-instructions.s

Just about the transformation, in this test-file, if you had the allowed imm for SP transformed and the not-allowed not transformed, that'd be enough, I think.

but I thought it was a bit easier to follow with the intermediate steps.

It is. :)

[scott-0]

Yes, that's a good idea; added.

llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

const auto &Op3 = static_cast<ARMOperand &>(*Operands[3]);
         auto &Op4 = static_cast<ARMOperand &>(*Operands[4]);
   if (!Op3.isReg() || !Op4.isReg())
     return;
 
   auto Op3Reg = Op3.getReg();
   auto Op4Reg = Op4.getReg();
 
   // For most Thumb2 cases we just generate the 3 operand form and reduce
-  // it in processInstruction(), but for ADD involving PC the the 3 operand
-  // form won't accept PC so we do the transformation here.
+  // it in processInstruction(), but the 3 operand form of ADD (t2ADDrr)
+  // won't accept SP or PC so we do the transformation here taking care
+  // with immediate range in the 'add sp, sp #imm' case.
   auto &Op5 = static_cast<ARMOperand &>(*Operands[5]);
   if (isThumbTwo()) {
-    if (Mnemonic != "add" ||
-        !(Op3Reg == ARM::PC || Op4Reg == ARM::PC ||
-          (Op5.isReg() && Op5.getReg() == ARM::PC)))
+    if (Mnemonic != "add")
+      return;
+    bool TryTransform = Op3Reg == ARM::PC || Op4Reg == ARM::PC ||
+                        (Op5.isReg() && Op5.getReg() == ARM::PC);
+    if (!TryTransform) {
+      TryTransform = (Op3Reg == ARM::SP || Op4Reg == ARM::SP ||
+                      (Op5.isReg() && Op5.getReg() == ARM::SP)) &&
+                     !(Op3Reg == ARM::SP && Op4Reg == ARM::SP &&
+                       Op5.isImm() && !Op5.isImm0_508s4());
+    }
+    if (!TryTransform)
       return;
   } else if (!isThumbOne())
     return;
 
   if (!(Mnemonic == "add" || Mnemonic == "sub" || Mnemonic == "and" ||
         Mnemonic == "eor" || Mnemonic == "lsl" || Mnemonic == "lsr" ||
         Mnemonic == "asr" || Mnemonic == "adc" || Mnemonic == "sbc" ||
         Mnemonic == "ror" || Mnemonic == "orr" || Mnemonic == "bic"))

llvm/trunk/test/MC/ARM/thumb2-narrow-dp.ll

// CHECK: add  r3, r1               @ encoding: [0x0b,0x44]
     ADD      r4, r4, pc          // T2
 // CHECK: add  r4, pc               @ encoding: [0x7c,0x44]
     ADD      r4, pc, r4          // T2
 // CHECK: add  r4, pc               @ encoding: [0x7c,0x44]
     ADD      pc, pc, r2          // T2
 // CHECK: add  pc, r2               @ encoding: [0x97,0x44]
     ADD      pc, r2, pc          // T2
 // CHECK: add  pc, r2               @ encoding: [0x97,0x44]
+    ADD      pc, pc, sp          // T2
+// CHECK: add   pc, sp              @ encoding: [0xef,0x44]
+    ADD      pc, sp, pc          // T2
+// CHECK: add   pc, sp, pc          @ encoding: [0xef,0x44]
 
 // ADD (SP plus immediate) A8.8.9
     ADD      sp, sp, #20         // T2
 // FIXME: ARMARM says 'add   sp, sp, #20'
-// CHECK: add   sp, #20         @ encoding: [0x05,0xb0]
+// CHECK: add   sp, #20             @ encoding: [0x05,0xb0]
+    ADD      sp, sp, #508        // T2
+// CHECK: add   sp, #508            @ encoding: [0x7f,0xb0]
+    ADD      sp, sp, #512        // T3
+// CHECK: add.w sp, sp, #512        @ encoding: [0x0d,0xf5,0x00,0x7d]
 
 // ADD (SP plus register) A8.8.10 (commutative)
     ADD      r9, sp, r9          // T1
 // CHECK: add   r9, sp, r9          @ encoding: [0xe9,0x44]
+    ADD      r9, r9, sp          // T1
+// FIXME: ARMARM says 'add   r9, sp, r9'
+// CHECK: add   r9, sp              @ encoding: [0xe9,0x44]
     ADD      sp, sp, r10         // T2
 // CHECK: add   sp, r10             @ encoding: [0xd5,0x44]
+    ADD      sp, r10, sp         // T2
+// CHECK: add   sp, r10             @ encoding: [0xd5,0x44]
+    ADD      sp, sp, pc          // T2
+// CHECK: add   sp, pc              @ encoding: [0xfd,0x44]
 
 // AND (commutative)
     ANDS     r0, r2, r1          // Must be wide - 3 distinct registers
     ANDS     r2, r2, r1          // Should choose narrow
     ANDS     r2, r1, r2          // Should choose narrow - commutative
     ANDS.W   r0, r0, r1          // Explicitly wide
     ANDS.W   r3, r1, r3  
     AND      r0, r1, r0          // Must use wide encoding as not flag-setting