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

[Tim Northover]

Hi Jyoti,

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

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5802
@@ +5801,3 @@
+struct RegListOperandRules {
+  unsigned OPcode;
+  bool AllowSP;
----------------
It's usually capitalised as "Opcode" elsewhere in LLVM.

================
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);
+  }
+};
----------------
Isn't this already the default implementation?

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5819-5822
@@ +5818,6 @@
+
+struct RegListOperandRulesForSTM {
+  RegListOperandRules RegListOperand;
+  RegListOperandRulesForSTM(const RegListOperandRules &X) : RegListOperand(X) {}
+  bool operator()(const RegListOperandRules &regListOperand) const {
+    return (RegListOperand.OPcode == regListOperand.OPcode &&
----------------
This is only used once, so would probably be best as a lambda.

================
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},
----------------
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.

http://reviews.llvm.org/D6090