Change strategy to avoid converting to narrow add

I've reverted the part of the previous commit that introduced and used
`revalidateInstruction`, replacing it with a more careful check in
`processInstruction` of whether to transform `t2ADDri` → `tADDi8` in
the first place.

I've kept the index fixes in the existing `validateInstruction`.

Also expanded the testing considerably, including making two test
files to deal with the fact that some of these unemittable
instructions aren't detected until a later phase of assembly, which
won't be reached if there are any errors in the first phase.

Author: statham-arm

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

@@ -659,8 +659,6 @@ class ARMAsmParser : public MCTargetAsmParser {
 
   bool validateInstruction(MCInst &Inst, const OperandVector &Ops,
                            unsigned MnemonicOpsEndInd);
-  bool revalidateInstruction(MCInst &Inst, const OperandVector &Ops,
-                             unsigned MnemonicOpsEndInd, unsigned OrigOpcode);
   bool processInstruction(MCInst &Inst, const OperandVector &Ops,
                           unsigned MnemonicOpsEndInd, MCStreamer &Out);
   bool shouldOmitVectorPredicateOperand(StringRef Mnemonic,
@@ -8657,41 +8655,6 @@ bool ARMAsmParser::validateInstruction(MCInst &Inst,
   return false;
 }
 
-// After processInstruction has transformed an instruction being assembled into
-// a different opcode, do any further validity checks that the new opcode
-// depends on.
-//
-// `Inst` contains the final modified form of the instruction, but `Operands`
-// contains the parsed operands from the _original_ instruction, because
-// nothing has updated them (`processInstruction` received them as const).
-// `OrigOpcode` contains the original value of `Inst.getOpcode()`, which should
-// give enough context to know how to understand the original operands list.
-bool ARMAsmParser::revalidateInstruction(MCInst &Inst,
-                                         const OperandVector &Operands,
-                                         unsigned MnemonicOpsEndInd,
-                                         unsigned OrigOpcode) {
-  const unsigned NewOpcode = Inst.getOpcode();
-
-  if (OrigOpcode == ARM::t2ADDri && NewOpcode == ARM::tADDi8) {
-    // t2ADDri is the Thumb 32-bit immediate add instruction, for example
-    // 'add[s] r0,r1,#0xff00'. If its immediate argument isn't a constant
-    // requiring shifting, then processInstruction can turn it into tADDi8, the
-    // simpler 16-bit Thumb immediate add (provided all the other conditions
-    // for that transformation are met). That makes it too late for
-    // validateInstruction to do this check, which it would have done if it had
-    // known from the start that the instruction was tADDi8.
-    int i = (Operands[MnemonicOpsEndInd + 1]->isImm()) ? MnemonicOpsEndInd + 1
-                                                       : MnemonicOpsEndInd + 2;
-    MCParsedAsmOperand &Op = *Operands[i];
-    if (isARMMCExpr(Op) && !isThumbI8Relocation(Op))
-      return Error(Op.getStartLoc(),
-                   "Immediate expression for Thumb adds requires :lower0_7:,"
-                   " :lower8_15:, :upper0_7: or :upper8_15:");
-  }
-
-  return false;
-}
-
 static unsigned getRealVSTOpcode(unsigned Opc, unsigned &Spacing) {
   switch(Opc) {
   default: llvm_unreachable("unexpected opcode!");
@@ -10748,14 +10711,25 @@ bool ARMAsmParser::processInstruction(MCInst &Inst,
     // the flags are compatible with the current IT status, use encoding T2
     // instead of T3. For compatibility with the system 'as'. Make sure the
     // wide encoding wasn't explicit.
-    if (Inst.getOperand(0).getReg() != Inst.getOperand(1).getReg() ||
-        !isARMLowRegister(Inst.getOperand(0).getReg()) ||
-        (Inst.getOperand(2).isImm() &&
-         (unsigned)Inst.getOperand(2).getImm() > 255) ||
-        Inst.getOperand(5).getReg() !=
-            (inITBlock() ? ARM::NoRegister : ARM::CPSR) ||
-        HasWideQualifier)
-      break;
+    if (HasWideQualifier)
+      break; // source code has asked for the 32-bit instruction
+    if (Inst.getOperand(0).getReg() != Inst.getOperand(1).getReg())
+      break; // tADDi8 can't take different input and output registers
+    if (!isARMLowRegister(Inst.getOperand(0).getReg()))
+      break; // high register that tADDi8 can't access
+    if (Inst.getOperand(5).getReg() !=
+        (inITBlock() ? ARM::NoRegister : ARM::CPSR))
+      break; // flag-modification would require overriding the IT state
+    if (Inst.getOperand(2).isImm()) {
+      if ((unsigned)Inst.getOperand(2).getImm() > 255)
+        break; // large immediate that tADDi8 can't contain
+    } else {
+      int i = (Operands[MnemonicOpsEndInd + 1]->isImm()) ? MnemonicOpsEndInd + 1
+        : MnemonicOpsEndInd + 2;
+      MCParsedAsmOperand &Op = *Operands[i];
+      if (isARMMCExpr(Op) && !isThumbI8Relocation(Op))
+        break; // a type of non-immediate that tADDi8 can't represent
+    }
     MCInst TmpInst;
     TmpInst.setOpcode(Inst.getOpcode() == ARM::t2ADDri ?
                       ARM::tADDi8 : ARM::tSUBi8);
@@ -11438,7 +11412,7 @@ bool ARMAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
   unsigned MnemonicOpsEndInd = getMnemonicOpsEndInd(Operands);
 
   switch (MatchResult) {
-  case Match_Success: {
+  case Match_Success:
     LLVM_DEBUG(dbgs() << "Parsed as: ";
                Inst.dump_pretty(dbgs(), MII.getName(Inst.getOpcode()));
                dbgs() << "\n");
@@ -11453,31 +11427,15 @@ bool ARMAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
       return true;
     }
 
-    // Some instructions need post-processing to, for example, tweak which
-    // encoding is selected. Loop on it while changes happen so the individual
-    // transformations can chain off each other. E.g.,
-    // tPOP(r8)->t2LDMIA_UPD(sp,r8)->t2STR_POST(sp,r8)
-    //
-    // This is written as a do-while inside an if, instead of the more obvious
-    // while loop, so that after postprocessing has completed the revised
-    // instruction can be revalidated, but (to save time) only if any changes
-    // had to be made at all.
-    unsigned OrigOpcode = Inst.getOpcode();
-    if (processInstruction(Inst, Operands, MnemonicOpsEndInd, Out)) {
-      do {
+    {
+      // Some instructions need post-processing to, for example, tweak which
+      // encoding is selected. Loop on it while changes happen so the
+      // individual transformations can chain off each other. E.g.,
+      // tPOP(r8)->t2LDMIA_UPD(sp,r8)->t2STR_POST(sp,r8)
+      while (processInstruction(Inst, Operands, MnemonicOpsEndInd, Out))
         LLVM_DEBUG(dbgs() << "Changed to: ";
                    Inst.dump_pretty(dbgs(), MII.getName(Inst.getOpcode()));
                    dbgs() << "\n");
-      } while (processInstruction(Inst, Operands, MnemonicOpsEndInd, Out));
-
-      MnemonicOpsEndInd = getMnemonicOpsEndInd(Operands);
-      if (revalidateInstruction(Inst, Operands, MnemonicOpsEndInd,
-                                OrigOpcode)) {
-        // As above, advance IT/VPT positions if we're exiting early.
-        forwardITPosition();
-        forwardVPTPosition();
-        return true;
-      }
     }
 
     // Only move forward at the very end so that everything in validate
@@ -11500,7 +11458,6 @@ bool ARMAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
       Out.emitInstruction(Inst, getSTI());
     }
     return false;
-  }
   case Match_NearMisses:
     ReportNearMisses(NearMisses, IDLoc, Operands);
     return true;

llvm/test/MC/ARM/lower-upper-errors-2.s

@@ -0,0 +1,18 @@
+// RUN: not llvm-mc --triple thumbv7m -filetype=obj -o /dev/null %s 2>&1 | FileCheck %s
+
+// This test checks reporting of errors of the form "you should have
+// used :lower16: in this immediate field", when the errors are
+// discovered at the object-file output stage by checking the set of
+// available relocations.
+//
+// For errors that are reported earlier, when initially reading the
+// instructions, see lower-upper-errors.s.
+
+// CHECK: [[@LINE+1]]:1: error: unsupported relocation
+adds r0, r0, #foo
+
+// CHECK: [[@LINE+1]]:1: error: unsupported relocation
+add r9, r0, #foo
+
+// CHECK: [[@LINE+1]]:1: error: expected relocatable expression
+movs r11, :upper8_15:#foo

llvm/test/MC/ARM/lower-upper-errors.s

@@ -1,20 +1,51 @@
-// RUN: not llvm-mc --triple thumbv6m -show-encoding %s 2>&1 | FileCheck %s
-// RUN: not llvm-mc --triple thumbv7m -show-encoding %s 2>&1 | FileCheck %s --check-prefixes=CHECK,THUMB2
+// RUN: not llvm-mc --triple thumbv6m %s 2>&1 | FileCheck %s --check-prefixes=CHECK,THUMB1
+// RUN: not llvm-mc --triple thumbv7m %s 2>&1 | FileCheck %s --check-prefixes=CHECK,THUMB2
 
-// Check reporting of errors of the form "you should have used
-// :lower16: in this immediate field".
+// This test checks reporting of errors of the form "you should have
+// used :lower16: in this immediate field", during initial reading of
+// the source file.
+//
+// For errors that are reported at object-file output time, see
+// lower-upper-errors-2.s.
 
 // CHECK: :[[@LINE+1]]:10: error: Immediate expression for Thumb movs requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
 movs r0, #foo
 
 // CHECK: :[[@LINE+1]]:10: error: Immediate expression for Thumb adds requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
 adds r0, #foo
 
-// CHECK: :[[@LINE+1]]:14: error: Immediate expression for Thumb adds requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
-adds r0, r0, #foo
-
 // THUMB2: :[[@LINE+1]]:10: error: immediate expression for mov requires :lower16: or :upper16
 movw r0, #foo
 
 // THUMB2: :[[@LINE+1]]:10: error: immediate expression for mov requires :lower16: or :upper16
 movt r0, #foo
+
+// With a Thumb2 wide add instruction available, this case isn't an error
+// while reading the source file. It only causes a problem when an object
+// file is output, and it turns out there's no suitable relocation to ask
+// for the value of an external symbol to be turned into a Thumb shifted
+// immediate field. And in this case the other errors in this source file
+// cause assembly to terminate before reaching the object-file output stage
+// (even if a test run had had -filetype=obj).
+
+// THUMB1: :[[@LINE+2]]:14: error: Immediate expression for Thumb adds requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
+// THUMB2-NOT: :[[@LINE+1]]:{{[0-9]+}}: error:
+adds r0, r0, #foo
+
+// Similarly for this version, which _must_ be the wide encoding due
+// to the use of a high register and the lack of flag-setting.
+
+// THUMB1: :[[@LINE+2]]:1: error: invalid instruction
+// THUMB2-NOT: :[[@LINE+1]]:{{[0-9]+}}: error:
+add r9, r0, #foo
+
+// CHECK: :[[@LINE+1]]:10: error: Immediate expression for Thumb movs requires :lower0_7:, :lower8_15:, :upper0_7: or :upper8_15:
+movs r0, :lower16:#foo
+
+// This is invalid in either architecture: in Thumb1 it can't use a
+// high register, and in Thumb2 it can't use :upper8_15:. But the
+// Thumb2 case won't cause an error until output.
+
+// THUMB1: :[[@LINE+2]]:1: error: invalid instruction
+// THUMB2-NOT: :[[@LINE+1]]:{{[0-9]+}}: error:
+movs r11, :upper8_15:#foo