[ARM] Refactor converting Thumb1 from 3 to 2 operand (nfc)

[scott-0]

Summary

Also adds some test cases that already pass.

tryConvertingToTwoOperandForm() is written in a slightly odd style so that the patches that follow are simpler.

[rengolin]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5489 ↗

(On Diff #29308)

This comment should probably be moved down...

[scott-0]

Thanks for the review!

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5489 ↗

(On Diff #29308)

Will do. (I left it here so that more of the this diff was a straight movement of lines.)

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

class ARMAsmParser : public MCTargetAsmParser {
 
   StringRef splitMnemonic(StringRef Mnemonic, unsigned &PredicationCode,
                           bool &CarrySetting, unsigned &ProcessorIMod,
                           StringRef &ITMask);
   void getMnemonicAcceptInfo(StringRef Mnemonic, StringRef FullInst,
                              bool &CanAcceptCarrySet,
                              bool &CanAcceptPredicationCode);
 
+  void tryConvertingToTwoOperandForm(StringRef Mnemonic, bool CarrySetting,
+                                     OperandVector &Operands);
   bool isThumb() const {
     // FIXME: Can tablegen auto-generate this?
     return STI.getFeatureBits()[ARM::ModeThumb];
   }
   bool isThumbOne() const {
     return isThumb() && !STI.getFeatureBits()[ARM::FeatureThumb2];
   }
   bool isThumbTwo() const {
if (Mnemonic == "bkpt" || Mnemonic == "cbnz" || Mnemonic == "setend" ||
     if (hasV6MOps())
       CanAcceptPredicationCode = Mnemonic != "movs";
     else
       CanAcceptPredicationCode = Mnemonic != "nop" && Mnemonic != "movs";
   } else
     CanAcceptPredicationCode = true;
 }
 
+// \brief Some Thumb1 instructions have two operand forms that are not
+// available as three operand, convert to two operand form if possible.
+//
+// FIXME: We would really like to be able to tablegen'erate this.
+void ARMAsmParser::tryConvertingToTwoOperandForm(StringRef Mnemonic,
+                                                 bool CarrySetting,
+                                                 OperandVector &Operands) {
+  if (Operands.size() != 6 || !isThumbOne())
+    return;
+
+  ARMOperand &Op3 = static_cast<ARMOperand &>(*Operands[3]);
+  ARMOperand &Op4 = static_cast<ARMOperand &>(*Operands[4]);
+  if (!Op3.isReg() || !Op4.isReg())
+    return;
+
+  ARMOperand &Op5 = static_cast<ARMOperand &>(*Operands[5]);
+
+  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"))
+    return;
+
+  // 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'
+  bool Transform = Op3.getReg() == Op4.getReg();
+  // If both registers are the same then remove one of them from
+  // the operand list, with certain exceptions.
+  if (Transform) {
+    // Don't transform 'adds Rd, Rd, Rm' or 'sub{s} Rd, Rd, Rm' because the
+    // 2 operand forms don't exist.
+    if (((Mnemonic == "add" && CarrySetting) || Mnemonic == "sub") &&
+        Op5.isReg())
+      Transform = false;
+  }
+
+  if (Transform)
+    Operands.erase(Operands.begin() + 3);
+}
+
 bool ARMAsmParser::shouldOmitCCOutOperand(StringRef Mnemonic,
                                           OperandVector &Operands) {
   // FIXME: This is all horribly hacky. We really need a better way to deal
   // with optional operands like this in the matcher table.
 
   // The 'mov' mnemonic is special. One variant has a cc_out operand, while
   // another does not. Specifically, the MOVW instruction does not. So we
   // special case it here and remove the defaulted (non-setting) cc_out
if (RequireVFPRegisterListCheck) {
     if (AcceptSinglePrecisionOnly && !Op.isSPRRegList())
       return Error(Op.getStartLoc(),
                    "VFP/Neon single precision register expected");
     if (AcceptDoublePrecisionOnly && !Op.isDPRRegList())
       return Error(Op.getStartLoc(),
                    "VFP/Neon double precision register expected");
   }
 
+  tryConvertingToTwoOperandForm(Mnemonic, CarrySetting, Operands);
+
   // Some instructions, mostly Thumb, have forms for the same mnemonic that
   // do and don't have a cc_out optional-def operand. With some spot-checks
   // of the operand list, we can figure out which variant we're trying to
   // parse and adjust accordingly before actually matching. We shouldn't ever
   // try to remove a cc_out operand that was explicitly set on the
   // mnemonic, of course (CarrySetting == true). Reason number #317 the
   // table driven matcher doesn't fit well with the ARM instruction set.
   if (!CarrySetting && shouldOmitCCOutOperand(Mnemonic, Operands))
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(

llvm/trunk/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     r4, sp, r4
+@ CHECK: add    r4, sp, r4          @ encoding: [0x6c,0x44]
+
+    add     r4, r4, sp
+@ CHECK: add    r4, sp              @ encoding: [0x6c,0x44]
+
+    add     sp, sp, #32
+@ FIXME: ARMARM says 'add   sp, sp, #32'
+@ CHECK: add    sp, #32             @ encoding: [0x08,0xb0]
+
+    add     r5, sp, #1016
+@ CHECK: add    r5, sp, #1016       @ encoding: [0xfe,0xad]
+
     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]
 
+    subs    r2, r2, #8
+@ CHECK: subs   r2, #8              @ encoding: [0x08,0x3a]
+
+    sub     sp, sp, #16
+@ CHECK: sub    sp, #16             @ encoding: [0x84,0xb0]
+
     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]