[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

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

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

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

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

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

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

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 ↗

(On Diff #15868)

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 ↗

(On Diff #16487)

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

5810–5816 ↗

(On Diff #16487)

Isn't this already the default implementation?

5819–5822 ↗

(On Diff #16487)

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

5834–5835 ↗

(On Diff #16487)

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 ↗

(On Diff #16487)

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 ↗

(On Diff #16487)

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 ↗

(On Diff #16487)

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

5834–5835 ↗

(On Diff #16487)

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

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

   return false;
 }
 
-// Check if the specified regisgter is in the register list of the inst,
+// Check if the specified register is in the register list of the inst,
 // starting at the indicated operand number.
 static bool listContainsReg(MCInst &Inst, unsigned OpNo, unsigned Reg) {
   for (unsigned i = OpNo; i < Inst.getNumOperands(); ++i) {

   return false;
 }
 
+// Check if there are any special registers in the register list of the inst,
+// starting at the indicated operand number.
+static bool listContainsSplReg(MCInst &Inst, unsigned OpNo, bool &SP, bool &PC,
+                               bool &LR) {
+  SP = PC = LR = false;
+  for (unsigned i = OpNo; i < Inst.getNumOperands(); ++i) {
+    unsigned OpReg = Inst.getOperand(i).getReg();
+    if (OpReg == ARM::SP)
+      SP = true;
+    else if (OpReg == ARM::PC)
+      PC = true;
+    else if (OpReg == ARM::LR)
+      LR = true;
+  }
+  if (SP || PC || LR)
+    return true;
+
+  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 Error(Operands[3]->getStartLoc(),
                    "writeback operator '!' not allowed when base register "
                    "in register list");
-    if (listContainsReg(Inst, 3 + HasWritebackToken, ARM::SP))
+    bool SP, PC, LR;
+    if (listContainsSplReg(Inst, 3 + HasWritebackToken, SP, PC, LR) && SP)
       return Error(Operands[3 + HasWritebackToken]->getStartLoc(),
                    "SP not allowed in register list");
+    if (PC && LR && !inITBlock())
+      return Error(
+          Operands[3 + HasWritebackToken]->getStartLoc(),
+          "LR not allowed in the list, when PC is in the register list");
+
     break;
   }
   case ARM::LDMIA_UPD:

   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");
+    bool SP, PC, LR;
+    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");
+      else if (SP)
+        return Error(Operands.back()->getStartLoc(),
+                     "SP not allowed in register list");
+      if (((Opcode == ARM::t2LDMIA) || (Opcode == ARM::t2LDMDB)) &&
+          (PC && LR) && !inITBlock())
+        return Error(
+            Operands.back()->getStartLoc(),
+            "LR not allowed in the list, when PC is in the register list");
+    }
     break;
   }
   case ARM::t2LDMIA_UPD:

   case ARM::t2LDMDB_UPD:
   case ARM::t2STMIA_UPD:
   case ARM::t2STMDB_UPD: {
+    bool SP, PC, LR;
     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");
+    if (listContainsSplReg(Inst, 3, SP, PC, LR)) {
+      if (((Opcode == ARM::t2STMIA_UPD) || (Opcode == ARM::t2STMDB_UPD)) &&
+          (SP || PC))
+        return Error(Operands.back()->getStartLoc(),
+                     "SP, PC not allowed in register list");
+      else if (SP)
+        return Error(Operands.back()->getStartLoc(),
+                     "SP not allowed in register list");
+      if (((Opcode == ARM::t2LDMIA_UPD) || (Opcode == ARM::t2LDMDB_UPD)) &&
+          (PC && LR) && !inITBlock())
+        return Error(
+            Operands.back()->getStartLoc(),
+            "LR not allowed in the list, when PC is in the register list");
+    }
     break;
   }
   case ARM::sysLDMIA_UPD:

         !isThumbTwo())
       return Error(Operands[2]->getStartLoc(),
                    "registers must be in range r0-r7 or pc");
+    bool SP, PC, LR;
+    if (listContainsSplReg(Inst, 2, SP, PC, LR) && (PC && LR))
+      return Error(
+          Operands[2]->getStartLoc(),
+          "LR not allowed in the list, when PC is in the register list");
+    if (SP)
+      return Error(Operands[2]->getStartLoc(),
+                   "SP not allowed in register list");
     break;
   }
   case ARM::tPUSH: {

         !isThumbTwo())
       return Error(Operands[2]->getStartLoc(),
                    "registers must be in range r0-r7 or lr");
+    bool SP, PC, LR;
+    if (listContainsSplReg(Inst, 2, SP, PC, LR) && (SP || PC) && !inITBlock())
+      return Error(Operands[2]->getStartLoc(),
+                   "SP, PC not allowed in register list");
     break;
   }
   case ARM::tSTMIA_UPD: {

       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");
+    bool SP, PC, LR;
+    if (listContainsSplReg(Inst, 4, SP, PC, LR) && !inITBlock()) {
+      if (SP || PC)
+        return Error(Operands.back()->getStartLoc(),
+                     "SP, PC not allowed in register list");
+    }
     break;
   }
   case ARM::tADDrSP: {

llvm/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