Transform 3 operand instructions to 2 operand versions of the same instruction if first 2 register operands are the same for thumb1

[rs]

Summary

This patch makes the ARM backend transform 3 operand instructions such as 'adds' to the 2 operand version of the same instruction if the first two register operands are the same e.g. 'adds r0, r0, #1' will is transformed to 'adds r0, #1'. Currently for some instructions such as 'adds' if you try to assemble 'adds r0, r0, #8' for thumb v6m the assembler would throw an error message because the immediate cannot be encoded using 3 bits, the backend should be smart enough to transform the instruction to 'adds r0, #8'.

[rengolin]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5657

Either call these Reg1, Reg2 or Op3, Op4.

5668

Being pedantic, the ARM ARM refers to the first register as being optional, not the second. :)

5672

why is this necessary?

5688

You could simplify this with a nested if

8196

Can you describe why is this change needed?

[rs]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5657

ok

5672

The 3 operand 'add' is transformed to the 2 operand 'add' and the 2 operand register-register 'add' does not have a CCOut operand, so it must be removed (see line 5236 which says the same thing).

5688

I'm not sure what you mean, would you prefer this 'if' to be put into nested 'if''s, or do you want it to be part of the previous 'if'?

8196

If you try to assemble 'add r1, r2' (echo 'add r1, r2' | llvm-mc --show-encoding --triple thumbv6m) you would get the error message ' error: instruction variant requires Thumb2', this instruction is supported in v6m.

[rengolin]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5672

ok

5688

I mean something like:

f (isThumbOne() && Operands.size() == 5 && Mnemonic == "add" && !CarrySetting) {
    ARMOperand Op2 = static_cast<ARMOperand &>(*Operands[2]);
    ARMOperand Op3 = static_cast<ARMOperand &>(*Operands[3]);
    if (Op2.isReg() && Op3.isReg() && Op1 == Op3)
        Operands.erase(Operands.begin() + 2);
}

8196

oh, ok, I read it backwards.

[rs]

Hi Renato,

I've attached a new patch with changes that you suggested, could you do another review please?

Thanks

[rengolin]

LGTM, thanks!

[rs]

I don't have commit rights yet so could you commit this patch for me?

Thanks

[rengolin]

Committed in r218521. Thanks.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

if (Op1.isReg() && Op2.isReg() && MRC.contains(Op1.getReg()) &&
       unsigned NewReg = MRI->getMatchingSuperReg(Reg1, ARM::gsub_0,
           &(MRI->getRegClass(ARM::GPRPairRegClassID)));
       Operands[Idx] =
           ARMOperand::CreateReg(NewReg, Op1.getStartLoc(), Op2.getEndLoc());
       Operands.erase(Operands.begin() + Idx + 1);
     }
   }
 
+  // If first 2 operands of a 3 operand instruction are the same
+  // then transform to 2 operand version of the same instruction
+  // e.g. 'adds r0, r0, #1' transforms to 'adds r0, #1'
+  // FIXME: We would really like to be able to tablegen'erate this.
+  if (isThumbOne() && Operands.size() == 6 &&
+       (Mnemonic == "add" || Mnemonic == "sub" || Mnemonic == "and" ||
+        Mnemonic == "eor" || Mnemonic == "lsl" || Mnemonic == "lsr" ||
+        Mnemonic == "asr" || Mnemonic == "adc" || Mnemonic == "sbc" ||
+        Mnemonic == "ror" || Mnemonic == "orr" || Mnemonic == "bic")) {
+      ARMOperand &Op3 = static_cast<ARMOperand &>(*Operands[3]);
+      ARMOperand &Op4 = static_cast<ARMOperand &>(*Operands[4]);
+      ARMOperand &Op5 = static_cast<ARMOperand &>(*Operands[5]);
+
+      // If both registers are the same then remove one of them from
+      // the operand list.
+      if (Op3.isReg() && Op4.isReg() && Op3.getReg() == Op4.getReg()) {
+          // If 3rd operand (variable Op5) is a register and the instruction is adds/sub
+          // then do not transform as the backend already handles this instruction
+          // correctly.
+          if (!Op5.isReg() || !((Mnemonic == "add" && CarrySetting) || Mnemonic == "sub")) {
+              Operands.erase(Operands.begin() + 3);
+              if (Mnemonic == "add" && !CarrySetting) {
+                  // Special case for 'add' (not 'adds') instruction must 
+                  // remove the CCOut operand as well.
+                  Operands.erase(Operands.begin() + 1);
+              }
+          }
+      }
+  }
+
+  // If instruction is 'add' and first two register operands
+  // use SP register, then remove one of the SP registers from
+  // the instruction.
+  // FIXME: We would really like to be able to tablegen'erate this.
+  if (isThumbOne() && Operands.size() == 5 && Mnemonic == "add" && !CarrySetting) {
+      ARMOperand &Op2 = static_cast<ARMOperand &>(*Operands[2]);
+      ARMOperand &Op3 = static_cast<ARMOperand &>(*Operands[3]);
+      if (Op2.isReg() && Op3.isReg() && Op2.getReg() == ARM::SP && Op3.getReg() == ARM::SP) {
+          Operands.erase(Operands.begin() + 2);
+      }
+  }
+
   // GNU Assembler extension (compatibility)
   if ((Mnemonic == "ldrd" || Mnemonic == "strd")) {
     ARMOperand &Op2 = static_cast<ARMOperand &>(*Operands[2]);
     ARMOperand &Op3 = static_cast<ARMOperand &>(*Operands[3]);
     if (Op3.isMem()) {
       assert(Op2.isReg() && "expected register argument");
 
       unsigned SuperReg = MRI->getMatchingSuperReg(
if (isThumbTwo() && Inst.getOperand(OpNo).getReg() != ARM::CPSR &&
         !inITBlock())
       return Match_RequiresITBlock;
     if (isThumbTwo() && Inst.getOperand(OpNo).getReg() == ARM::CPSR &&
         inITBlock())
       return Match_RequiresNotITBlock;
   }
   // Some high-register supporting Thumb1 encodings only allow both registers
   // to be from r0-r7 when in Thumb2.
-  else if (Opc == ARM::tADDhirr && isThumbOne() &&
+  else if (Opc == ARM::tADDhirr && isThumbOne() && !hasV6MOps() &&
            isARMLowRegister(Inst.getOperand(1).getReg()) &&
            isARMLowRegister(Inst.getOperand(2).getReg()))
     return Match_RequiresThumb2;
   // Others only require ARMv6 or later.
   else if (Opc == ARM::tMOVr && isThumbOne() && !hasV6Ops() &&
            isARMLowRegister(Inst.getOperand(0).getReg()) &&
            isARMLowRegister(Inst.getOperand(1).getReg()))
     return Match_RequiresV6;

test/MC/ARM/thumb_rewrites.s

+@ RUN: llvm-mc -triple thumbv6m -show-encoding < %s | FileCheck %s
+
+    adds    r0, r0, #8
+@ CHECK: adds   r0, #8              @ encoding: [0x08,0x30]
+
+    adds    r0, r0, r0
+@ CHECK: adds   r0, r0, r0          @ encoding: [0x00,0x18]
+
+    add     r0, r0, r8
+@ CHECK: add    r0, r8              @ encoding: [0x40,0x44]
+
+    add     sp, sp, r0
+@ CHECK: add    sp, r0              @ encoding: [0x85,0x44]
+
+    add     r0, r0, r1
+@ CHECK: add    r0, r1              @ encoding: [0x08,0x44]
+
+    add     r2, r2, r3
+@ CHECK: add    r2, r3              @ encoding: [0x1a,0x44]
+
+    subs    r0, r0, r0
+@ CHECK: subs   r0, r0, r0          @ encoding: [0x00,0x1a]
+
+    ands    r0, r0, r1
+@ CHECK: ands   r0, r1              @ encoding: [0x08,0x40]
+
+    eors    r0, r0, r1
+@ CHECK: eors   r0, r1              @ encoding: [0x48,0x40]
+
+    lsls    r0, r0, r1
+@ CHECK: lsls   r0, r1              @ encoding: [0x88,0x40]
+
+    lsrs    r0, r0, r1
+@ CHECK: lsrs   r0, r1              @ encoding: [0xc8,0x40]
+
+    asrs    r0, r0, r1
+@ CHECK: asrs   r0, r1              @ encoding: [0x08,0x41]
+
+    adcs    r0, r0, r1
+@ CHECK: adcs   r0, r1              @ encoding: [0x48,0x41]
+
+    sbcs    r0, r0, r1
+@ CHECK: sbcs   r0, r1              @ encoding: [0x88,0x41]
+
+    rors    r0, r0, r1
+@ CHECK: rors   r0, r1              @ encoding: [0xc8,0x41]
+
+    orrs    r0, r0, r1
+@ CHECK: orrs   r0, r1              @ encoding: [0x08,0x43]
+
+    bics    r0, r0, r1
+@ CHECK: bics   r0, r1              @ encoding: [0x88,0x43]