[Thumb/Thumb2] Added restrictions on PC, LR, SP in the register list for PUSH/POP/LDM/STM

[jyoti.allur]

Summary

This patch implements the following from ARM ARM for v7m

PUSH The SP and PC cannot be in the list
POP The SP cannot be in the list. If the PC is in the list, the LR must not be in the list

Apart this following is added,
LDM, LDMIA, LDMDB The SP cannot be in the list. If the PC is in the list, the LR must not be in the list, not in ITBLOCK
STM, STMIA, STMDB The SP and PC cannot be in the list

[t.p.northover]

Hi Jyoti,

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

5788 ↗

(On Diff #15707)

I don't think there's any need to abbreviate "Special" here, even if it does make for overflowing lines in the callers.

The bool return also seems a little weird now, particularly after seeing the uses. I'd probably change it to "void findSpecialRegsInList(...)".

6003 ↗

(On Diff #15707)

Where do you get that this depends on the IT block status? It looks like the T1 encoding simply cannot handle PC or LR, and the T2 encoding makes no mention of IT on that line:

if n == 15 || BitCount(registers) < 2 || (P == '1' && M == '1') then UNPREDICTABLE;

6028–6029 ↗

(On Diff #15707)

I wonder if we should completely refactor this handling now. We're getting an awful lot of duplicated and complex logic. Could we refactor it into a table-driven function call "checkRegList" or something?

I'm thinking along the lines of

bool validateRegListOperands(unsigned Opcode, ...) {
  struct {
    unsigned Opcode;
    bool AllowSP;
    bool AllowPCAndLR;
    [...]
  } AllowedCombinations[] = {
    { ARM::t2LDMIA, false, false, [...] },
    [...] 
  };
  auto Entry =std::find(std::begin(AllowedCombinations), std::end(AllowedCombinations), Opcode);
  [...]
}

I suppose it depends on just how uniform the constraints are. It seems like they split reasonably sensibly among ARM/Thumb, load/store, writeback/not and the the various modes though. Could be worth a try.

[jyoti.allur]

Hi Tim,

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

6003 ↗

(On Diff #15707)

Hi Tim,
I referred to ARM®v7-M Architecture Reference Manual Errata markup
https://silver.arm.com/download/ARM_and_AMBA_Architecture/AR580-DC-11001-r0p0-02rel0/DDI0403D_arm_architecture_v7m_reference_manual_errata_markup_1_0.pdf

A7.7.40 (A7-284 page)
for <registers> ie., register list following is stated.

" The SP cannot be in the list.
If the PC is in the list, the LR must not be in the list and the instruction must either
be outside an IT block or the last instruction in an IT block. "

the same is found in A7.7.41 (A7-286 page)

if registers<15> == ‘1’ && InITBlock() && !LastInITBlock() then UNPREDICTABLE;

6028–6029 ↗

(On Diff #15707)

Yes, i was also not happy with this duplicated & complex code. I will try to table generate these checks as you suggested.

[t.p.northover]

Hi Jyoti,

Just one quick reply:

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

6003 ↗

(On Diff #15707)

I still don't think that translates to the condition you added. It looks more like

if (PC && LR)
  Error
else if (PC && inITBlock() && !lastInITBlock())
  Error

[jyoti.allur]

Hi Tim,
Yes, you are right. sorry my understanding of the statement was incorrect. I will implement accordingly.
Need a small clarification about lastInITBlock.

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

6003 ↗

(On Diff #15707)

The last stmt in ITBlock is defined as
boolean LastInITBlock()
return (ITSTATE.IT<3:0> == ‘1000’);

Will it be correct to define this function as follows since 4-TZ gives the size of instructions in ITblock & CurPosition points to current instruction in IT block?
bool lastInITBlock()
{

unsigned TZ = countTrailingZeros(ITState.Mask);
return (ITState.CurPosition == 4 - TZ);

}

[t.p.northover]

Hi Jyoti,

[jyoti.allur]

This comment was removed by jyoti.allur.

[jyoti.allur]

Hi Tim,
I have applied the review comments in this lastest diff version.
Thanks.

[jyoti.allur]

Hi Tim,
I have made some more improvement to the previous patch. Here is a summary of what this patch contains.
Moved all of the register list validation to validateRegListOperands.
We check the conditions for throwing appropriate diagnostic only if the rules in table are rendered false.
listContainsReg removed as we have added findSpecialRegsInList.
Testcases for reglist validation related with SP, PC, LR in /thumb-diagnostics.s
Testcase for ITBlock related validation for LDM/lDMIA,LDMDB in thumb2-diagnostics.s
A testcase in v8_IT_manual.s needed correction after addding this patch due to SP restriction in thumb mode which is valid for v8 as well.

Could you please review it ?

[jyoti.allur]

Hi Tim, Compnerd, Tilman,
Gentle ping.

[t.p.northover]

I think this is splitting up the logic along confusing lines. Some checks are still in the parent switch, but others have been moved to the new validateRegListOperands function.

It looks like that split might be because they use subtarget predicates. If so, just make validateRegListOperands a member of ARMAsmParser.

Cheers.

Tim.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5816–5819

Isn't this the default?

[jyoti.allur]

Hi Tim,
It looks like I was too focused on special registers in register list violations, other diagnostic related with thumb mode alone somehow got overlooked by me.

Updated the patch accordingly in the latest Diff.
Sorry for the trouble of having to review again.

[jyoti.allur]

Hi Tim,
I have updated the patch as follows.
-> Moved all register list validations to validateRegListOperands (member of ARMAsmParser)
-> Split the table AllowedCombinations to AllowedCombinationsSTM and AllowedCombinationsLDM so that we can search based on specific applicable flags only for those set of instructions.

Thanks a lot for reviewing.

[jyoti.allur]

Hi Tim,
gentle ping :)

[t.p.northover]

Hi Jyoti,

Sorry about the delay here. I still find the logic fairly impenetrable, I'm afraid.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5802

It's usually capitalised as "Opcode" elsewhere in LLVM.

5810–5816

Isn't this already the default implementation?

5819–5822

This is only used once, so would probably be best as a lambda.

5834–5835

I don't think these are making the logic clearer. They're such special cases "allow tLDMIA if there's no SP, there is a PC, there is no LR, it's not in an IT block, the base register is not in the list and only low registers are in the list".

It neither covers all valid nor all invalid tLDMIA uses, just some random subset (that's really difficult to think about, at least for me).

Part of it may be that the variables are phrased as "AllowSP" but really get interpreted as "MustHaveSP" by the search. But I'm not convinced fixing that would actually clear things up.

[jyoti.allur]

Hi Tim,
i have replied inline. Let me know your opinion.

-Jyoti

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5810–5816

I am not sure i follow. Could you help me understand what you mean by default implementation here?

We consider it an error when any of these flags being compared is other than what are set in AllowedCombinationsLDM table, so i am comparing each of them here. If any one flag fails, then std::find will return std::end and we throw appropriate diagnostic after checking the flags status, because diagnostic is different based on each incorrect field corresponding to that flag.

For Ex: Consider this restriction, "If the PC is in the list, the LR must not be in the list" applicable to LDM variants, POP.
This is indicated by AllowPCAndLR field in the tables. This is 4th field in the tables ie., AllowedCombinationsLDM[][3]
and AllowedCombinationsSTM[][3]. So all 4th field entries are all "false" in AllowedCombinationsLDM as it is not allowed on LDM set instructions, and "true" in AllowedCombinationsSTM as there is no such restriction on them.

When result of std::find is std::end, we know that some error occurred, but we don't exactly know which, so we check
the flags (bool SP, PC, LR, BaseReg, LowReg, listContainsBase) updated by findSpecialRegsInList and checkLowRegisterList, and throw appropriate diagnostic.

5834–5835

The table encodes the rules on reglist and the way to interpret is as follows :-

{ARM::tLDMIA, false, true, false, false, true, true},
If instruction is ARM::tLDMIA, SP is not allowed, PC is allowed, LR is not allowed, inst should not be in IT block, base register is allowed in register list, only low registers are allowed in register list.
The above mentioned is the rule for ARM::tLDMIA. If any flags are other than above said, we should not allow ARM::tLDMIA.

I should probably add,
{ARM::tLDMIA, false, false, true, false, true, true},

since PC & LR are exclusive as per rule.

Covering all valid or all invalid case for each instruction would result in a very big table 2 to the power n where n is 6.
If current logic is not easy to interpret or code readability is affected, it would be better to remove the table method and just check the flags updated by findSpecialRegsInList and checkLowRegisterList and print diagnostic like it existed originally in the patch before.

[t.p.northover]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5810–5816

Ah, sorry. For some reason I thought classes got a default implementation of "operator ==".

5834–5835

I think that would probably be best. In its current form I think it's practically impenetrable.

[jyoti.allur]

Hi Tim,
Sorry for the delay in updating the patch.
I think this looks okay now.
Thanks.

[t.p.northover]

Hi Jyoti,

Thanks for the updated patch. I think that's fine now (sorry for suggesting the table thing earlier, it didn't work out that well).

Cheers.

Tim.

[jyoti.allur]

Hi Tim,
No problem. Commited with rL223356

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

     if (++ITState.CurPosition == 5 - TZ)
       ITState.CurPosition = ~0U; // Done with the IT block after this.
   }
+  bool lastInITBlock() {
+    unsigned TZ = countTrailingZeros(ITState.Mask);
+    return (ITState.CurPosition == 4 - TZ);
+  }
 
-
   MCAsmParser &getParser() const { return Parser; }
   MCAsmLexer &getLexer() const { return Parser.getLexer(); }
 

   bool processInstruction(MCInst &Inst, const OperandVector &Ops);
   bool shouldOmitCCOutOperand(StringRef Mnemonic, OperandVector &Operands);
   bool shouldOmitPredicateOperand(StringRef Mnemonic, OperandVector &Operands);
+  bool validateRegListOperands(MCInst &Inst, const OperandVector &Operands,
+                               unsigned OpNo);
 
 public:
   enum ARMMatchResultTy {

   return false;
 }
 
-// Check if the specified regisgter is in the register list of the inst,
+// Check if there are any special registers in the register list of the inst,
 // starting at the indicated operand number.
-static bool listContainsReg(MCInst &Inst, unsigned OpNo, unsigned Reg) {
+static void findSpecialRegsInList(MCInst &Inst, unsigned OpNo, bool &SP,
+                                  bool &PC, bool &LR, bool &BaseReg) {
+  SP = PC = LR = BaseReg = false;
+  unsigned Rn = Inst.getOperand(0).getReg();
   for (unsigned i = OpNo; i < Inst.getNumOperands(); ++i) {
     unsigned OpReg = Inst.getOperand(i).getReg();
-    if (OpReg == Reg)
-      return true;
+    if (OpReg == ARM::SP)
+      SP = true;
+    else if (OpReg == ARM::PC)
+      PC = true;
+    else if (OpReg == ARM::LR)
+      LR = true;
+    if (OpReg == Rn)
+      BaseReg = true;
   }
+
+  return;
+}
+
+struct RegListOperandRules {
+  unsigned OPcode;
+  bool AllowSP;
+  bool AllowPC;
+  bool AllowPCAndLR;
+  bool AllowInITBlock;
+  bool AllowBaseRegister;
+  bool AllowLowRegisterOnly;
+
+  bool operator==(const RegListOperandRules &rhs) const {
+    return (OPcode == rhs.OPcode && AllowSP == rhs.AllowSP &&
+            AllowPC == rhs.AllowPC && AllowPCAndLR == rhs.AllowPCAndLR &&
+            AllowInITBlock == rhs.AllowInITBlock &&
+            AllowBaseRegister == rhs.AllowBaseRegister &&
+            AllowLowRegisterOnly == rhs.AllowLowRegisterOnly);
+  }
+};
+
+struct RegListOperandRulesForSTM {
+  RegListOperandRules RegListOperand;
+  RegListOperandRulesForSTM(const RegListOperandRules &X) : RegListOperand(X) {}
+  bool operator()(const RegListOperandRules &regListOperand) const {
+    return (RegListOperand.OPcode == regListOperand.OPcode &&
+            RegListOperand.AllowSP == regListOperand.AllowSP &&
+            RegListOperand.AllowPC == regListOperand.AllowPC &&
+            RegListOperand.AllowBaseRegister ==
+                regListOperand.AllowBaseRegister &&
+            RegListOperand.AllowLowRegisterOnly ==
+                regListOperand.AllowLowRegisterOnly);
+  }
+};
+
+static const RegListOperandRules AllowedCombinationsLDM[] = {
+    {ARM::tLDMIA, false, true, false, false, true,
+     true}, /// Notionally handles ARM::tLDMIA_UPD too
+    {ARM::tLDMIA_UPD, false, true, false, false, false, true},
+    {ARM::t2LDMIA, false, true, false, false, true, false},
+    {ARM::t2LDMDB, false, true, false, false, true, false},
+    {ARM::t2LDMIA_UPD, false, true, false, false, false, false},
+    {ARM::t2LDMDB_UPD, false, true, false, false, false, false}};
+
+static const RegListOperandRules AllowedCombinationsSTM[] = {
+    {ARM::tSTMIA_UPD, false, false, true, true, false, true},
+    {ARM::t2STMIA, false, false, true, true, true, false},
+    {ARM::t2STMDB, false, false, true, true, true, false},
+    {ARM::t2STMIA_UPD, false, false, true, true, false, false},
+    {ARM::t2STMDB_UPD, false, false, true, true, false, false}};
+
+bool ARMAsmParser::validateRegListOperands(MCInst &Inst,
+                                           const OperandVector &Operands,
+                                           unsigned OpNo) {
+  bool SP, PC, LR, BaseReg, LowReg, listContainsBase;
+  const unsigned Opcode = Inst.getOpcode();
+  unsigned Rn = Inst.getOperand(0).getReg();
+  findSpecialRegsInList(Inst, OpNo, SP, PC, LR, BaseReg);
+  LowReg = checkLowRegisterList(Inst, OpNo, Rn, 0, listContainsBase);
+  RegListOperandRules regListOp;
+  regListOp.OPcode = Opcode;
+  regListOp.AllowSP = SP;
+  regListOp.AllowPC = PC;
+  regListOp.AllowPCAndLR = (PC && LR);
+  regListOp.AllowInITBlock = inITBlock();
+  regListOp.AllowBaseRegister = BaseReg;
+  regListOp.AllowLowRegisterOnly = !LowReg;
+
+  auto EntrySTM = std::find_if(
+      std::begin(AllowedCombinationsSTM), std::end(AllowedCombinationsSTM),
+      RegListOperandRulesForSTM(RegListOperandRules(regListOp)));
+  auto EntryLDM = std::find(std::begin(AllowedCombinationsLDM),
+                            std::end(AllowedCombinationsLDM), regListOp);
+
+  if (EntrySTM == std::end(AllowedCombinationsSTM)) {
+
+    switch (Opcode) {
+    case ARM::t2STMIA_UPD:
+    case ARM::t2STMDB_UPD: {
+      if (BaseReg)
+        return Error(Operands.back()->getStartLoc(),
+                     "writeback register not allowed in register list");
+    }
+    case ARM::tSTMIA_UPD: {
+      if (LowReg && !isThumbTwo())
+        return Error(Operands[OpNo]->getStartLoc(),
+                     "registers must be in range r0-r7");
+
+      // This would be converted to a 32-bit stm, but that's not valid if the
+      // writeback register is in the list.
+      if (LowReg && BaseReg)
+        return Error(Operands[OpNo]->getStartLoc(),
+                     "writeback operator '!' not allowed when base register "
+                     "in register list");
+    }
+    case ARM::tPUSH: {
+      if (LowReg && !LR && !isThumbTwo())
+        return Error(Operands[2]->getStartLoc(),
+                     "registers must be in range r0-r7 or lr");
+    }
+    case ARM::t2STMIA:
+    case ARM::t2STMDB: {
+      if (SP || PC)
+        return Error(Operands[OpNo]->getStartLoc(),
+                     "SP, PC not allowed in register list");
+      break;
+    }
+    }
+  }
+
+  if (EntryLDM == std::end(AllowedCombinationsLDM)) {
+    switch (Opcode) {
+    case ARM::t2LDMIA_UPD:
+    case ARM::t2LDMDB_UPD: {
+      if (BaseReg)
+        return Error(Operands.back()->getStartLoc(),
+                     "writeback register not allowed in register list");
+    }
+    case ARM::tLDMIA: {
+      // If we're parsing Thumb2, the .w variant is available and handles
+      // most cases that are normally illegal for a Thumb1 LDM instruction.
+      // We'll make the transformation in processInstruction() if necessary.
+      //
+      // Thumb LDM instructions are writeback iff the base register is not
+      // in the register list.
+      bool HasWritebackToken =
+          (static_cast<ARMOperand &>(*Operands[3]).isToken() &&
+           static_cast<ARMOperand &>(*Operands[3]).getToken() == "!");
+
+      if (LowReg && !isThumbTwo())
+        return Error(Operands[3 + HasWritebackToken]->getStartLoc(),
+                     "registers must be in range r0-r7");
+      // If we should have writeback, then there should be a '!' token.
+      if (!BaseReg && !HasWritebackToken && !isThumbTwo())
+        return Error(Operands[2]->getStartLoc(),
+                     "writeback operator '!' expected");
+
+      // If we should not have writeback, there must not be a '!'. This is
+      // true even for the 32-bit wide encodings.
+      if (BaseReg && HasWritebackToken)
+        return Error(Operands[3]->getStartLoc(),
+                     "writeback operator '!' not allowed when base register "
+                     "in register list");
+    }
+    case ARM::tPOP: {
+      if (LowReg && !PC && !isThumbTwo())
+        return Error(Operands[2]->getStartLoc(),
+                     "registers must be in range r0-r7 or pc");
+    }
+    case ARM::t2LDMIA:
+    case ARM::t2LDMDB: {
+      if (SP)
+        return Error(Operands[OpNo]->getStartLoc(),
+                     "SP not allowed in register list");
+      if (PC && LR)
+        return Error(
+            Operands[OpNo]->getStartLoc(),
+            "LR not allowed in the list, when PC is in the register list");
+      if (PC && inITBlock() && !lastInITBlock())
+        return Error(Operands[OpNo]->getStartLoc(),
+                     "Instruction should be outside an IT block or last in IT "
+                     "block, when PC is in the register list");
+      break;
+    }
+    case ARM::LDMIA_UPD:
+    case ARM::LDMDB_UPD:
+    case ARM::LDMIB_UPD:
+    case ARM::LDMDA_UPD: {
+      if (BaseReg)
+        return Error(Operands.back()->getStartLoc(),
+                     "writeback register not allowed in register list");
+      break;
+    }
+    case ARM::sysLDMIA_UPD:
+    case ARM::sysLDMDA_UPD:
+    case ARM::sysLDMDB_UPD:
+    case ARM::sysLDMIB_UPD: {
+      if (!PC)
+        return Error(Operands[4]->getStartLoc(),
+                     "writeback register only allowed on system LDM "
+                     "if PC in register-list");
+      break;
+    }
+    }
+  }
   return false;
 }
 

 // Return true if instruction has the interesting property of being
 // allowed in IT blocks, but not being predicable.
 static bool instIsBreakpoint(const MCInst &Inst) {
-    return Inst.getOpcode() == ARM::tBKPT ||
-           Inst.getOpcode() == ARM::BKPT ||
-           Inst.getOpcode() == ARM::tHLT ||
-           Inst.getOpcode() == ARM::HLT;
-
+  return Inst.getOpcode() == ARM::tBKPT || Inst.getOpcode() == ARM::BKPT ||
+         Inst.getOpcode() == ARM::tHLT || Inst.getOpcode() == ARM::HLT;
 }
 
 // FIXME: We would really like to be able to tablegen'erate this.

                    "bitfield width must be in range [1,32-lsb]");
     return false;
   }
-  // Notionally handles ARM::tLDMIA_UPD too.
-  case ARM::tLDMIA: {
-    // If we're parsing Thumb2, the .w variant is available and handles
-    // most cases that are normally illegal for a Thumb1 LDM instruction.
-    // We'll make the transformation in processInstruction() if necessary.
-    //
-    // Thumb LDM instructions are writeback iff the base register is not
-    // in the register list.
-    unsigned Rn = Inst.getOperand(0).getReg();
-    bool HasWritebackToken =
-        (static_cast<ARMOperand &>(*Operands[3]).isToken() &&
-         static_cast<ARMOperand &>(*Operands[3]).getToken() == "!");
-    bool ListContainsBase;
-    if (checkLowRegisterList(Inst, 3, Rn, 0, ListContainsBase) && !isThumbTwo())
-      return Error(Operands[3 + HasWritebackToken]->getStartLoc(),
-                   "registers must be in range r0-r7");
-    // If we should have writeback, then there should be a '!' token.
-    if (!ListContainsBase && !HasWritebackToken && !isThumbTwo())
-      return Error(Operands[2]->getStartLoc(),
-                   "writeback operator '!' expected");
-    // If we should not have writeback, there must not be a '!'. This is
-    // true even for the 32-bit wide encodings.
-    if (ListContainsBase && HasWritebackToken)
-      return Error(Operands[3]->getStartLoc(),
-                   "writeback operator '!' not allowed when base register "
-                   "in register list");
-    if (listContainsReg(Inst, 3 + HasWritebackToken, ARM::SP))
-      return Error(Operands[3 + HasWritebackToken]->getStartLoc(),
-                   "SP not allowed in register list");
-    break;
-  }
+
   case ARM::LDMIA_UPD:
   case ARM::LDMDB_UPD:
   case ARM::LDMIB_UPD:

     // UNPREDICTABLE on v7 upwards. Goodness knows what they did before.
     if (!hasV7Ops())
       break;
-    if (listContainsReg(Inst, 3, Inst.getOperand(0).getReg()))
-      return Error(Operands.back()->getStartLoc(),
-                   "writeback register not allowed in register list");
-    break;
+  case ARM::tLDMIA: // Notionally handles ARM::tLDMIA_UPD too.
   case ARM::t2LDMIA:
   case ARM::t2LDMDB:
   case ARM::t2STMIA:
-  case ARM::t2STMDB: {
-    if (listContainsReg(Inst, 3, ARM::SP))
-      return Error(Operands.back()->getStartLoc(),
-                   "SP not allowed in register list");
-    break;
-  }
+  case ARM::t2STMDB:
   case ARM::t2LDMIA_UPD:
   case ARM::t2LDMDB_UPD:
   case ARM::t2STMIA_UPD:
-  case ARM::t2STMDB_UPD: {
-    if (listContainsReg(Inst, 3, Inst.getOperand(0).getReg()))
-      return Error(Operands.back()->getStartLoc(),
-                   "writeback register not allowed in register list");
-
-    if (listContainsReg(Inst, 4, ARM::SP))
-      return Error(Operands.back()->getStartLoc(),
-                   "SP not allowed in register list");
-    break;
-  }
+  case ARM::t2STMDB_UPD:
   case ARM::sysLDMIA_UPD:
   case ARM::sysLDMDA_UPD:
   case ARM::sysLDMDB_UPD:
-  case ARM::sysLDMIB_UPD:
-    if (!listContainsReg(Inst, 3, ARM::PC))
-      return Error(Operands[4]->getStartLoc(),
-                   "writeback register only allowed on system LDM "
-                   "if PC in register-list");
+  case ARM::sysLDMIB_UPD: {
+    validateRegListOperands(Inst, Operands, 3);
     break;
+  }
   case ARM::sysSTMIA_UPD:
   case ARM::sysSTMDA_UPD:
   case ARM::sysSTMDB_UPD:

   // Like for ldm/stm, push and pop have hi-reg handling version in Thumb2,
   // so only issue a diagnostic for thumb1. The instructions will be
   // switched to the t2 encodings in processInstruction() if necessary.
-  case ARM::tPOP: {
-    bool ListContainsBase;
-    if (checkLowRegisterList(Inst, 2, 0, ARM::PC, ListContainsBase) &&
-        !isThumbTwo())
-      return Error(Operands[2]->getStartLoc(),
-                   "registers must be in range r0-r7 or pc");
-    break;
-  }
+  case ARM::tPOP:
   case ARM::tPUSH: {
-    bool ListContainsBase;
-    if (checkLowRegisterList(Inst, 2, 0, ARM::LR, ListContainsBase) &&
-        !isThumbTwo())
-      return Error(Operands[2]->getStartLoc(),
-                   "registers must be in range r0-r7 or lr");
+    validateRegListOperands(Inst, Operands, 2);
     break;
   }
   case ARM::tSTMIA_UPD: {
-    bool ListContainsBase, InvalidLowList;
-    InvalidLowList = checkLowRegisterList(Inst, 4, Inst.getOperand(0).getReg(),
-                                          0, ListContainsBase);
-    if (InvalidLowList && !isThumbTwo())
-      return Error(Operands[4]->getStartLoc(),
-                   "registers must be in range r0-r7");
-
-    // This would be converted to a 32-bit stm, but that's not valid if the
-    // writeback register is in the list.
-    if (InvalidLowList && ListContainsBase)
-      return Error(Operands[4]->getStartLoc(),
-                   "writeback operator '!' not allowed when base register "
-                   "in register list");
-    if (listContainsReg(Inst, 4, ARM::SP) && !inITBlock())
-      return Error(Operands.back()->getStartLoc(),
-                   "SP not allowed in register list");
+    validateRegListOperands(Inst, Operands, 4);
     break;
   }
   case ARM::tADDrSP: {

test/MC/ARM/thumb-diagnostics.s

         ldmfd r2!, {r1, r3-r6, sp}
         ldmdb r1, {r2, r3, sp}
         ldmdb r1!, {r2, r3, sp} 
+        ldm r2, {r5, lr, pc}
 @ CHECK-ERRORS: error: registers must be in range r0-r7
 @ CHECK-ERRORS:         ldm r2!, {r5, r8}
 @ CHECK-ERRORS:                  ^

 @ CHECK-ERRORS-V7M: error: SP not allowed in register list
 @ CHECK-ERRORS-V7M:         ldmdb r1!, {r2, r3, sp}
 @ CHECK-ERRORS-V7M:                    ^
+@ CHECK-ERRORS-V7M: error: LR not allowed in the list, when PC is in the register list
+@ CHECK-ERRORS-V7M:         ldm r2, {r5, lr, pc}
+@ CHECK-ERRORS-V7M:                 ^
 
 @ Invalid writeback and register lists for PUSH/POP
         pop {r1, r2, r10}
+        pop {r1, r2, lr, pc}
         push {r8, r9}
+        push {r8, r9, sp}
+        push {r8, r9, pc}
 @ CHECK-ERRORS: error: registers must be in range r0-r7 or pc
 @ CHECK-ERRORS:         pop {r1, r2, r10}
 @ CHECK-ERRORS:             ^
+@ CHECK-ERRORS-V7M: error: LR not allowed in the list, when PC is in the register list
+@ CHECK-ERRORS-V7M:         pop {r1, r2, lr, pc}
+@ CHECK-ERRORS-V7M:             ^
 @ CHECK-ERRORS: error: registers must be in range r0-r7 or lr
 @ CHECK-ERRORS:         push {r8, r9}
 @ CHECK-ERRORS:              ^
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
+@ CHECK-ERRORS-V7M:          push {r8, r9, sp}
+@ CHECK-ERRORS-V7M:               ^
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
+@ CHECK-ERRORS-V7M:          push {r8, r9, pc}
+@ CHECK-ERRORS-V7M:               ^
 
 
 @ Invalid writeback and register lists for STM

         stmia r4!, {r0-r3, sp}
         stmdb r1, {r2, r3, sp}
         stmdb r1!, {r2, r3, sp}
+        stmia r4, {r2, sp, pc}
+        stmdb r1!, {r2, r3, pc} 
 @ CHECK-ERRORS: error: instruction requires: thumb2
 @ CHECK-ERRORS:         stm r1, {r2, r6}
 @ CHECK-ERRORS:         ^

 @ CHECK-ERRORS-V8: error: writeback register not allowed in register list
 @ CHECK-ERRORS-V8:         stmdb r2!, {r0, r2}
 @ CHECK-ERRORS-V8:                  ^
-@ CHECK-ERRORS-V7M: error: SP not allowed in register list
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
 @ CHECK-ERRORS-V7M:         stm r1!, {r2, sp}
 @ CHECK-ERRORS-V7M:                  ^
-@ CHECK-ERRORS-V7M: error: SP not allowed in register list
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
 @ CHECK-ERRORS-V7M:         stmia r4!, {r0-r3, sp}
 @ CHECK-ERRORS-V7M:                    ^
-@ CHECK-ERRORS-V7M: error: SP not allowed in register list
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
 @ CHECK-ERRORS-V7M:         stmdb r1, {r2, r3, sp}
 @ CHECK-ERRORS-V7M:                   ^
-@ CHECK-ERRORS-V7M: error: SP not allowed in register list
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
 @ CHECK-ERRORS-V7M:         stmdb r1!, {r2, r3, sp}
 @ CHECK-ERRORS-V7M:                    ^
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
+@ CHECK-ERRORS-V7M:         stmia r4, {r2, sp, pc}
+@ CHECK-ERRORS-V7M:                   ^
+@ CHECK-ERRORS-V7M: error: SP, PC not allowed in register list
+@ CHECK-ERRORS-V7M:         stmdb r1!, {r2, r3, pc}
+@ CHECK-ERRORS-V7M:                    ^
 
 @ Out of range immediates for LSL instruction.
         lsls r4, r5, #-1

test/MC/ARM/thumb2-diagnostics.s

         iteeee gt
         ittfe le
         nopeq
+        ite eq
+        ldmiaeq sp!, {r4, pc}
+        movne r0, #0
 
 @ CHECK-ERRORS: error: incorrect condition in IT block; got 'le', but expected 'eq'
 @ CHECK-ERRORS:         addle r0, r1, r2

 @ CHECK-ERRORS: error: predicated instructions must be in IT block
 @ CHECK-ERRORS:         nopeq
 @ CHECK-ERRORS:         ^
+@ CHECK-ERRORS: error: Instruction should be outside an IT block or last in IT block, when PC is in the register list
+@ CHECK-ERRORS:         ldmiaeq sp!, {r4, pc}
+@ CHECK-ERRORS:                      ^
 
         @ Out of range immediates for MRC/MRC2/MRRC/MRRC2
         mrc  p14, #8, r1, c1, c2, #4

test/MC/ARM/v8_IT_manual.s

 @ PUSH, encoding T2 (32-bit)
 @ CHECK: [[@LINE+2]]:1: warning: deprecated instruction in IT block
 it ge
-pushge {r1, r13, r7}
+pushge {r1, r3, r7}
 @ PUSH, encoding T3 (32-bit)
 @ CHECK: [[@LINE+2]]:1: warning: deprecated instruction in IT block
 it ge
-pushge {r13}
+pushge {r3}
 
 @ REV, encoding T1
 @ CHECK: [[@LINE+2]]:1: warning: deprecated instruction in IT block

 @ CHECK: [[@LINE+2]]:1: warning: deprecated instruction in IT block
 it ge
 stmge r1, {r2, r3}
+@ STM, encoding T3 (32-bit)
 @ CHECK: [[@LINE+2]]:1: warning: deprecated instruction in IT block
 it ge
-stmge r1!, {r2, r13}
+stmge r1!, {r2}
 
 @ LDM, encoding T1
 @ CHECK: [[@LINE+2]]:1: warning: deprecated instruction in IT block