[ARM] Fix detection of duplicates when parsing reg list operands

[chill]

Summary

For examples of currently failing cases, look at some of the entries in the test.

[ostannard]

LGTM

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

CreateRegList(SmallVectorImpl<std::pair<unsigned, unsigned>> &Regs,
     } else if (ARMMCRegisterClasses[ARM::SPRRegClassID].contains(
                    Regs.front().second)) {
       if (Regs.back().second == ARM::VPR)
         Kind = k_FPSRegisterListWithVPR;
       else
         Kind = k_SPRRegisterList;
     }
 
-    // Sort based on the register encoding values.
-    array_pod_sort(Regs.begin(), Regs.end());
-
     if (Kind == k_RegisterList && Regs.back().second == ARM::APSR)
       Kind = k_RegisterListWithAPSR;
 
+    assert(std::is_sorted(Regs.begin(), Regs.end()) &&
+           "Register list must be sorted by encoding");
+
     auto Op = make_unique<ARMOperand>(Kind);
-    for (SmallVectorImpl<std::pair<unsigned, unsigned>>::const_iterator
-           I = Regs.begin(), E = Regs.end(); I != E; ++I)
-      Op->Registers.push_back(I->second);
+    for (const auto &P : Regs)
+      Op->Registers.push_back(P.second);
 
     Op->StartLoc = StartLoc;
     Op->EndLoc = EndLoc;
     return Op;
   }
 
   static std::unique_ptr<ARMOperand> CreateVectorList(unsigned RegNum,
                                                       unsigned Count,
static unsigned getNextRegister(unsigned Reg) {
   case ARM::R6:  return ARM::R7;  case ARM::R7:  return ARM::R8;
   case ARM::R8:  return ARM::R9;  case ARM::R9:  return ARM::R10;
   case ARM::R10: return ARM::R11; case ARM::R11: return ARM::R12;
   case ARM::R12: return ARM::SP;  case ARM::SP:  return ARM::LR;
   case ARM::LR:  return ARM::PC;  case ARM::PC:  return ARM::R0;
   }
 }
 
+// Insert an <Encoding, Register> pair in an ordered vector. Return true on
+// success, or false, if duplicate encoding found.
+static bool
+insertNoDuplicates(SmallVectorImpl<std::pair<unsigned, unsigned>> &Regs,
+                   unsigned Enc, unsigned Reg) {
+  Regs.emplace_back(Enc, Reg);
+  for (auto I = Regs.rbegin(), J = I + 1, E = Regs.rend(); J != E; ++I, ++J) {
+    if (J->first == Enc) {
+      Regs.erase(J.base());
+      return false;
+    }
+    if (J->first < Enc)
+      break;
+    std::swap(*I, *J);
+  }
+  return true;
+}
+
 /// Parse a register list.
 bool ARMAsmParser::parseRegisterList(OperandVector &Operands,
                                      bool EnforceOrder) {
   MCAsmParser &Parser = getParser();
   if (Parser.getTok().isNot(AsmToken::LCurly))
     return TokError("Token is not a Left Curly Brace");
   SMLoc S = Parser.getTok().getLoc();
   Parser.Lex(); // Eat '{' token.
bool ARMAsmParser::parseRegisterList(OperandVector &Operands,
   // space for that many.
   int EReg = 0;
   SmallVector<std::pair<unsigned, unsigned>, 16> Registers;
 
   // Allow Q regs and just interpret them as the two D sub-registers.
   if (ARMMCRegisterClasses[ARM::QPRRegClassID].contains(Reg)) {
     Reg = getDRegFromQReg(Reg);
     EReg = MRI->getEncodingValue(Reg);
-    Registers.push_back(std::pair<unsigned, unsigned>(EReg, Reg));
+    Registers.emplace_back(EReg, Reg);
     ++Reg;
   }
   const MCRegisterClass *RC;
   if (ARMMCRegisterClasses[ARM::GPRRegClassID].contains(Reg))
     RC = &ARMMCRegisterClasses[ARM::GPRRegClassID];
   else if (ARMMCRegisterClasses[ARM::DPRRegClassID].contains(Reg))
     RC = &ARMMCRegisterClasses[ARM::DPRRegClassID];
   else if (ARMMCRegisterClasses[ARM::SPRRegClassID].contains(Reg))
     RC = &ARMMCRegisterClasses[ARM::SPRRegClassID];
   else if (ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID].contains(Reg))
     RC = &ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID];
   else
     return Error(RegLoc, "invalid register in register list");
 
   // Store the register.
   EReg = MRI->getEncodingValue(Reg);
-  Registers.push_back(std::pair<unsigned, unsigned>(EReg, Reg));
+  Registers.emplace_back(EReg, Reg);
 
   // This starts immediately after the first register token in the list,
   // so we can see either a comma or a minus (range separator) as a legal
   // next token.
   while (Parser.getTok().is(AsmToken::Comma) ||
          Parser.getTok().is(AsmToken::Minus)) {
     if (Parser.getTok().is(AsmToken::Minus)) {
       Parser.Lex(); // Eat the minus.
if (Parser.getTok().is(AsmToken::Minus)) {
       // Ranges must go from low to high.
       if (MRI->getEncodingValue(Reg) > MRI->getEncodingValue(EndReg))
         return Error(AfterMinusLoc, "bad range in register list");
 
       // Add all the registers in the range to the register list.
       while (Reg != EndReg) {
         Reg = getNextRegister(Reg);
         EReg = MRI->getEncodingValue(Reg);
-        Registers.push_back(std::pair<unsigned, unsigned>(EReg, Reg));
+        if (!insertNoDuplicates(Registers, EReg, Reg)) {
+          Warning(AfterMinusLoc, StringRef("duplicated register (") +
+                                     ARMInstPrinter::getRegisterName(Reg) +
+                                     ") in register list");
+        }
       }
       continue;
     }
     Parser.Lex(); // Eat the comma.
     RegLoc = Parser.getTok().getLoc();
     int OldReg = Reg;
     const AsmToken RegTok = Parser.getTok();
     Reg = tryParseRegister();
     if (Reg == -1)
       return Error(RegLoc, "register expected");
     // Allow Q regs and just interpret them as the two D sub-registers.
     bool isQReg = false;
     if (ARMMCRegisterClasses[ARM::QPRRegClassID].contains(Reg)) {
       Reg = getDRegFromQReg(Reg);
       isQReg = true;
     }
     if (!RC->contains(Reg) &&
         RC->getID() == ARMMCRegisterClasses[ARM::GPRRegClassID].getID() &&
         ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID].contains(Reg)) {
       // switch the register classes, as GPRwithAPSRnospRegClassID is a partial
       // subset of GPRRegClassId except it contains APSR as well.
       RC = &ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID];
     }
-    if (Reg == ARM::VPR && (RC == &ARMMCRegisterClasses[ARM::SPRRegClassID] ||
-                            RC == &ARMMCRegisterClasses[ARM::DPRRegClassID])) {
+    if (Reg == ARM::VPR &&
+        (RC == &ARMMCRegisterClasses[ARM::SPRRegClassID] ||
+         RC == &ARMMCRegisterClasses[ARM::DPRRegClassID] ||
+         RC == &ARMMCRegisterClasses[ARM::FPWithVPRRegClassID])) {
       RC = &ARMMCRegisterClasses[ARM::FPWithVPRRegClassID];
       EReg = MRI->getEncodingValue(Reg);
-      Registers.push_back(std::pair<unsigned, unsigned>(EReg, Reg));
+      if (!insertNoDuplicates(Registers, EReg, Reg)) {
+        Warning(RegLoc, "duplicated register (" + RegTok.getString() +
+                            ") in register list");
+      }
       continue;
     }
     // The register must be in the same register class as the first.
     if (!RC->contains(Reg))
       return Error(RegLoc, "invalid register in register list");
     // In most cases, the list must be monotonically increasing. An
     // exception is CLRM, which is order-independent anyway, so
     // there's no potential for confusion if you write clrm {r2,r1}
     // instead of clrm {r1,r2}.
     if (EnforceOrder &&
         MRI->getEncodingValue(Reg) < MRI->getEncodingValue(OldReg)) {
       if (ARMMCRegisterClasses[ARM::GPRRegClassID].contains(Reg))
         Warning(RegLoc, "register list not in ascending order");
       else if (!ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID].contains(Reg))
         return Error(RegLoc, "register list not in ascending order");
     }
-    if (MRI->getEncodingValue(Reg) == MRI->getEncodingValue(OldReg)) {
-      Warning(RegLoc, "duplicated register (" + RegTok.getString() +
-              ") in register list");
-      continue;
-    }
     // VFP register lists must also be contiguous.
     if (RC != &ARMMCRegisterClasses[ARM::GPRRegClassID] &&
         RC != &ARMMCRegisterClasses[ARM::GPRwithAPSRnospRegClassID] &&
         Reg != OldReg + 1)
       return Error(RegLoc, "non-contiguous register range");
     EReg = MRI->getEncodingValue(Reg);
-    Registers.push_back(std::pair<unsigned, unsigned>(EReg, Reg));
+    if (!insertNoDuplicates(Registers, EReg, Reg)) {
+      Warning(RegLoc, "duplicated register (" + RegTok.getString() +
+                          ") in register list");
+    }
     if (isQReg) {
       EReg = MRI->getEncodingValue(++Reg);
-      Registers.push_back(std::pair<unsigned, unsigned>(EReg, Reg));
+      Registers.emplace_back(EReg, Reg);
     }
   }
 
   if (Parser.getTok().isNot(AsmToken::RCurly))
     return Error(Parser.getTok().getLoc(), "'}' expected");
   SMLoc E = Parser.getTok().getEndLoc();
   Parser.Lex(); // Eat '}' token.
 

llvm/trunk/test/MC/ARM/register-list-dup.s

+// RUN: not llvm-mc -triple=thumbv8.1m.main-none-eabi -show-encoding < %s 2>&1 | FileCheck -strict-whitespace %s
+
+clrm {r0, r0}
+// CHECK: warning: duplicated register (r0) in register list
+// CHECK-NEXT: {{^clrm {r0, r0}}}
+// CHECK-NEXT: {{^          \^}}
+
+clrm {r0, r0, r1}
+// CHECK: warning: duplicated register (r0) in register list
+// CHECK-NEXT: {{^clrm {r0, r0, r1}}}
+// CHECK-NEXT: {{^          \^}}
+
+clrm {r0, r1, r0}
+// CHECK: warning: duplicated register (r0) in register list
+// CHECK-NEXT: {{^clrm {r0, r1, r0}}}
+// CHECK-NEXT: {{^              \^}}
+
+clrm {r0, r1, r1}
+// CHECK: warning: duplicated register (r1) in register list
+// CHECK-NEXT: {{^clrm {r0, r1, r1}}}
+// CHECK-NEXT: {{^              \^}}
+
+clrm {r1, r0, r1}
+// CHECK: warning: duplicated register (r1) in register list
+// CHECK-NEXT: {{^clrm {r1, r0, r1}}}
+// CHECK-NEXT: {{^              \^}}
+
+clrm {r1, r1, r0}
+// CHECK: warning: duplicated register (r1) in register list
+// CHECK-NEXT: {{^clrm {r1, r1, r0}}}
+// CHECK-NEXT: {{^          \^}}
+
+clrm {r0-r3, r0}
+// CHECK: warning: duplicated register (r0) in register list
+// CHECK-NEXT: {{^clrm {r0-r3, r0}}}
+// CHECK-NEXT: {{^             \^}}
+
+clrm {r2, r0-r3}
+// CHECK: warning: duplicated register (r2) in register list
+// CHECK-NEXT: {{^clrm {r2, r0-r3}}}
+// CHECK-NEXT: {{^             \^}}
+
+vscclrm {s0, s0, s1, vpr}
+// CHECK: error: non-contiguous register range
+// CHECK: {{^vscclrm {s0, s0, s1, vpr}}}
+// CHECK: {{^             \^}}
+
+vscclrm {s0-s3, vpr, s4}
+// CHECK: error: register list not in ascending order
+// CHECK-NEXT: {{^vscclrm {s0-s3, vpr, s4}}}
+// CHECK-NEXT: {{^                     \^}}
+
+vscclrm {s0-s3, vpr, vpr}
+// CHECK: warning: duplicated register (vpr) in register list
+// CHECK-NEXT: {{^vscclrm {s0-s3, vpr, vpr}}}
+// CHECK-NEXT: {{^                     \^}}
+
+vscclrm {q2, d4, vpr}
+// CHECK: error: register list not in ascending order
+// CHECK-NEXT: {{^vscclrm {q2, d4, vpr}}}
+// CHECK-NEXT: {{^             \^}}
+
+vscclrm {q2, d5, vpr}
+// CHECK: error: non-contiguous register range
+// CHECK-NEXT: {{^vscclrm {q2, d5, vpr}}}
+// CHECK-NEXT: {{^             \^}}