[ARM] Tidy up banked registers encoding

[javed.absar]

Summary

Moves encoding (SYSm) information of banked registers to ARMSystemRegister.td, where it rightly belongs and forms a single point of reference in the code.

[fhahn]

LGTM, looks like a straight-forward improvement. I just have 2 nits. Please hold off with committing for a bit, so other people have time to voice concerns.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

4178 ↗

(On Diff #109335)

Should this comment be moved to lib/Target/ARM/ARMSystemRegister.td too?

lib/Target/ARM/Utils/ARMBaseInfo.cpp

46 ↗

(On Diff #109335)

not sure what the correct indentation for macros is, but this seems slightly inconsistent with lib/Target/ARM/Utils/ARMBaseInfo.h:69, where it was aligned with the code.

[rovka]

lib/Target/ARM/Utils/ARMBaseInfo.cpp

46 ↗

(On Diff #109335)

It's consistent with the indentation above, for GET_MYCLASSSYSTREG_IMPL, so if you change one you should change both.
While we're discussing indentation, I think we don't usually indent a namespace within a namespace, so both ARMSysReg and ARMBankedReg should be unindented here (luckily, the code in ARMSysReg isn't indented, so it would be a tiny fix).

[javed.absar]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

4178 ↗

(On Diff #109335)

Yes. That should be. Thanks for catching it.

lib/Target/ARM/Utils/ARMBaseInfo.cpp

46 ↗

(On Diff #109335)

I too was confused with what the right indentation should be.
I saw that Tim indented namespace within namespace (https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AArch64/Utils/AArch64BaseInfo.cpp) so followed that one.

[olista01]

We also have the reverse mapping (encoding->name) in ARMInstPrinter::printBankedRegOperand, could that use this table too?

[javed.absar]

In this update-

1. Removed the manual (name->Encoding) in ARMISelDAGToDAG. Had missed it out earlier.
2. Moved the comment to ARMSystemRegister.td
3. Fixed indentation of "namespace namespace " . I too think no indentation on namespace is right choice (as Diana also mentioned).

[javed.absar]

In D36219#829225, @olista01 wrote:

We also have the reverse mapping (encoding->name) in ARMInstPrinter::printBankedRegOperand, could that use this table too?

I will attempt to clean this in a separate patch as the existing handling seems bit messy/tricky.

[fhahn]

Thanks, I'm happy with the indentation and lib/Target/ARM/ARMISelDAGToDAG.cpp was a good spot.

[javed.absar]

Thanks @fhahn . I will rebase and commit this patch now.
Will work on the reverse mapping which @olista01 pointed out after that.

llvm/trunk/lib/Target/ARM/ARMISelDAGToDAG.cpp

 }
 
 // Maps a Banked Register string to its mask value. The mask value returned is
 // for use in the MRSbanked / MSRbanked instruction nodes as the Banked Register
 // mask operand, which expresses which register is to be used, e.g. r8, and in
 // which mode it is to be used, e.g. usr. Returns -1 to signify that the string
 // was invalid.
 static inline int getBankedRegisterMask(StringRef RegString) {
-  return StringSwitch<int>(RegString.lower())
-          .Case("r8_usr", 0x00)
-          .Case("r9_usr", 0x01)
-          .Case("r10_usr", 0x02)
-          .Case("r11_usr", 0x03)
-          .Case("r12_usr", 0x04)
-          .Case("sp_usr", 0x05)
-          .Case("lr_usr", 0x06)
-          .Case("r8_fiq", 0x08)
-          .Case("r9_fiq", 0x09)
-          .Case("r10_fiq", 0x0a)
-          .Case("r11_fiq", 0x0b)
-          .Case("r12_fiq", 0x0c)
-          .Case("sp_fiq", 0x0d)
-          .Case("lr_fiq", 0x0e)
-          .Case("lr_irq", 0x10)
-          .Case("sp_irq", 0x11)
-          .Case("lr_svc", 0x12)
-          .Case("sp_svc", 0x13)
-          .Case("lr_abt", 0x14)
-          .Case("sp_abt", 0x15)
-          .Case("lr_und", 0x16)
-          .Case("sp_und", 0x17)
-          .Case("lr_mon", 0x1c)
-          .Case("sp_mon", 0x1d)
-          .Case("elr_hyp", 0x1e)
-          .Case("sp_hyp", 0x1f)
-          .Case("spsr_fiq", 0x2e)
-          .Case("spsr_irq", 0x30)
-          .Case("spsr_svc", 0x32)
-          .Case("spsr_abt", 0x34)
-          .Case("spsr_und", 0x36)
-          .Case("spsr_mon", 0x3c)
-          .Case("spsr_hyp", 0x3e)
-          .Default(-1);
+  auto TheReg = ARMBankedReg::lookupBankedRegByName(RegString.lower());
+  if (!TheReg)
+     return -1;
+  return TheReg->Encoding;
 }
 
 // The flags here are common to those allowed for apsr in the A class cores and
 // those allowed for the special registers in the M class cores. Returns a
 // value representing which flags were present, -1 if invalid.
 static inline int getMClassFlagsMask(StringRef Flags) {
   return StringSwitch<int>(Flags)
           .Case("", 0x2) // no flags means nzcvq for psr registers, and 0x2 is

llvm/trunk/lib/Target/ARM/ARMSystemRegister.td

 def : MClassSysReg<0,    0,    1,    0x891, "basepri_ns">;
 def : MClassSysReg<0,    0,    1,    0x893, "faultmask_ns">;
 }
 
 let Requires = [{ {ARM::Feature8MSecExt} }] in {
 def : MClassSysReg<0,    0,    1,    0x894, "control_ns">;
 def : MClassSysReg<0,    0,    1,    0x898, "sp_ns">;
 }
+
+
+// Banked Registers
+//
+class BankedReg<string name,  bits<8> enc>
+               : SearchableTable {
+  string Name;
+  bits<8> Encoding;
+  let Name = name;
+  let Encoding = enc;
+  let SearchableFields = ["Name", "Encoding"];
+}
+
+// The values here come from B9.2.3 of the ARM ARM, where bits 4-0 are SysM
+// and bit 5 is R.
+def : BankedReg<"r8_usr",   0x00>;
+def : BankedReg<"r9_usr",   0x01>;
+def : BankedReg<"r10_usr",  0x02>;
+def : BankedReg<"r11_usr",  0x03>;
+def : BankedReg<"r12_usr",  0x04>;
+def : BankedReg<"sp_usr",   0x05>;
+def : BankedReg<"lr_usr",   0x06>;
+def : BankedReg<"r8_fiq",   0x08>;
+def : BankedReg<"r9_fiq",   0x09>;
+def : BankedReg<"r10_fiq",  0x0a>;
+def : BankedReg<"r11_fiq",  0x0b>;
+def : BankedReg<"r12_fiq",  0x0c>;
+def : BankedReg<"sp_fiq",   0x0d>;
+def : BankedReg<"lr_fiq",   0x0e>;
+def : BankedReg<"lr_irq",   0x10>;
+def : BankedReg<"sp_irq",   0x11>;
+def : BankedReg<"lr_svc",   0x12>;
+def : BankedReg<"sp_svc",   0x13>;
+def : BankedReg<"lr_abt",   0x14>;
+def : BankedReg<"sp_abt",   0x15>;
+def : BankedReg<"lr_und",   0x16>;
+def : BankedReg<"sp_und",   0x17>;
+def : BankedReg<"lr_mon",   0x1c>;
+def : BankedReg<"sp_mon",   0x1d>;
+def : BankedReg<"elr_hyp",  0x1e>;
+def : BankedReg<"sp_hyp",   0x1f>;
+def : BankedReg<"spsr_fiq", 0x2e>;
+def : BankedReg<"spsr_irq", 0x30>;
+def : BankedReg<"spsr_svc", 0x32>;
+def : BankedReg<"spsr_abt", 0x34>;
+def : BankedReg<"spsr_und", 0x36>;
+def : BankedReg<"spsr_mon", 0x3c>;
+def : BankedReg<"spsr_hyp", 0x3e>;

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

 ARMAsmParser::parseBankedRegOperand(OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   SMLoc S = Parser.getTok().getLoc();
   const AsmToken &Tok = Parser.getTok();
   if (!Tok.is(AsmToken::Identifier))
     return MatchOperand_NoMatch;
   StringRef RegName = Tok.getString();
 
-  // The values here come from B9.2.3 of the ARM ARM, where bits 4-0 are SysM
-  // and bit 5 is R.
-  unsigned Encoding = StringSwitch<unsigned>(RegName.lower())
-                          .Case("r8_usr", 0x00)
-                          .Case("r9_usr", 0x01)
-                          .Case("r10_usr", 0x02)
-                          .Case("r11_usr", 0x03)
-                          .Case("r12_usr", 0x04)
-                          .Case("sp_usr", 0x05)
-                          .Case("lr_usr", 0x06)
-                          .Case("r8_fiq", 0x08)
-                          .Case("r9_fiq", 0x09)
-                          .Case("r10_fiq", 0x0a)
-                          .Case("r11_fiq", 0x0b)
-                          .Case("r12_fiq", 0x0c)
-                          .Case("sp_fiq", 0x0d)
-                          .Case("lr_fiq", 0x0e)
-                          .Case("lr_irq", 0x10)
-                          .Case("sp_irq", 0x11)
-                          .Case("lr_svc", 0x12)
-                          .Case("sp_svc", 0x13)
-                          .Case("lr_abt", 0x14)
-                          .Case("sp_abt", 0x15)
-                          .Case("lr_und", 0x16)
-                          .Case("sp_und", 0x17)
-                          .Case("lr_mon", 0x1c)
-                          .Case("sp_mon", 0x1d)
-                          .Case("elr_hyp", 0x1e)
-                          .Case("sp_hyp", 0x1f)
-                          .Case("spsr_fiq", 0x2e)
-                          .Case("spsr_irq", 0x30)
-                          .Case("spsr_svc", 0x32)
-                          .Case("spsr_abt", 0x34)
-                          .Case("spsr_und", 0x36)
-                          .Case("spsr_mon", 0x3c)
-                          .Case("spsr_hyp", 0x3e)
-                          .Default(~0U);
-
-  if (Encoding == ~0U)
+  auto TheReg = ARMBankedReg::lookupBankedRegByName(RegName.lower());
+  if (!TheReg)
     return MatchOperand_NoMatch;
+  unsigned Encoding = TheReg->Encoding;
 
   Parser.Lex(); // Eat identifier token.
   Operands.push_back(ARMOperand::CreateBankedReg(Encoding, S));
   return MatchOperand_Success;
 }
 
 OperandMatchResultTy
 ARMAsmParser::parsePKHImm(OperandVector &Operands, StringRef Op, int Low,

llvm/trunk/lib/Target/ARM/Utils/ARMBaseInfo.h

 
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "MCTargetDesc/ARMMCTargetDesc.h"
 
 namespace llvm {
 
+// System Registers
 namespace ARMSysReg {
   struct MClassSysReg {
     const char *Name;
     uint16_t M1Encoding12;
     uint16_t M2M3Encoding8;
     uint16_t Encoding;
     FeatureBitset FeaturesRequired;
 
namespace ARMSysReg {
   // Note: ARMv7-M deprecates using MSR APSR without a _<bits> qualifier
   const MClassSysReg *lookupMClassSysRegAPSRNonDeprecated(unsigned SYSm);
 
   // lookup system registers using 8-bit SYSm value
   const MClassSysReg *lookupMClassSysRegBy8bitSYSmValue(unsigned SYSm);
 
 } // end namespace ARMSysReg
 
+// Banked Registers
+namespace ARMBankedReg {
+  struct BankedReg {
+    const char *Name;
+    uint16_t Encoding;
+  };
+  #define GET_BANKEDREG_DECL
+  #include "ARMGenSystemRegister.inc"
+} // end namespace ARMBankedReg
+
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_ARM_UTILS_ARMBASEINFO_H

llvm/trunk/lib/Target/ARM/Utils/ARMBaseInfo.cpp

 #include "ARMBaseInfo.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Regex.h"
 
 using namespace llvm;
 namespace llvm {
-  namespace ARMSysReg {
 
 // lookup system register using 12-bit SYSm value.
 // Note: the search is uniqued using M1 mask
 const MClassSysReg *lookupMClassSysRegBy12bitSYSmValue(unsigned SYSm) {
   return lookupMClassSysRegByM1Encoding12(SYSm);
 }
 
 // returns APSR with _<bits> qualifier.
 // Note: ARMv7-M deprecates using MSR APSR without a _<bits> qualifier
 const MClassSysReg *lookupMClassSysRegAPSRNonDeprecated(unsigned SYSm) {
   return lookupMClassSysRegByM2M3Encoding8((1<<9)|(SYSm & 0xFF));
 }
 
 // lookup system registers using 8-bit SYSm value
 const MClassSysReg *lookupMClassSysRegBy8bitSYSmValue(unsigned SYSm) {
   return ARMSysReg::lookupMClassSysRegByM2M3Encoding8((1<<8)|(SYSm & 0xFF));
 }
 
 #define GET_MCLASSSYSREG_IMPL
 #include "ARMGenSystemRegister.inc"
 
-  }
-}
+} // end namespace ARMSysReg
+
+namespace ARMBankedReg {
+#define GET_BANKEDREG_IMPL
+#include "ARMGenSystemRegister.inc"
+} // end namespce ARMSysReg
+} // end namespace llvm