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

[Tim Northover]

Hi Jyoti,

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5788
@@ +5787,3 @@
+// starting at the indicated operand number.
+static bool listContainsSplReg(MCInst &Inst, unsigned OpNo, bool &SP, bool &PC,
+                               bool &LR) {
----------------
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(...)".

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6003
@@ -5981,1 +6002,3 @@
                    "SP not allowed in register list");
+    if (PC && LR && !inITBlock())
+      return Error(
----------------
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;

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6028-6029
@@ +6027,4 @@
+    if (listContainsSplReg(Inst, 3, SP, PC, LR)) {
+      if (((Opcode == ARM::t2STMIA) || (Opcode == ARM::t2STMDB)) && (SP || PC))
+        return Error(Operands.back()->getStartLoc(),
+                     "SP, PC not allowed in register list");
----------------
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.

http://reviews.llvm.org/D6090