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

[Tim Northover]

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5810-5816
@@ +5809,9 @@
+
+  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);
+  }
+};
----------------
jyoti.allur wrote:
> t.p.northover wrote:
> > Isn't this already the default implementation?
> 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.
> 
Ah, sorry. For some reason I thought classes got a default implementation of "operator ==".

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5834-5835
@@ +5833,4 @@
+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},
----------------
jyoti.allur wrote:
> t.p.northover wrote:
> > 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.
> 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.
> 
I think that would probably be best. In its current form I think it's practically impenetrable.

http://reviews.llvm.org/D6090