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

[Jyoti Allur]

Hi Tim,

================
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(
----------------
t.p.northover wrote:
> 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;
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; 

================
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");
----------------
t.p.northover wrote:
> 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.
Yes, i was also not happy with this duplicated & complex code. I will try to table generate these checks as you suggested.

http://reviews.llvm.org/D6090