Enable MatchRegisterName to match register altnames

[asb]

Summary

Currently Tablegen includes support for alternate register names, as added in r133940. These alternate names are made use of in the AsmWriter, but not in AsmMatcher. A number of in-tree targets have multiple names for register (e.g. ABI and architectural names), but set ShouldEmitMatchRegisterName to false and write their own matching function, duplicating the information already available in Tablegen. This commit will cause the TableGen'ned MatchRegisterName function to match register altnames provided that MatchRegisterNameShouldMatchAltNames is set. This is set to false by default to ensure no unexpected changes to the behaviour of in-tree and out-of-tree targets, but I'd hope to later switch the default to true and remove the option altogether in the future.

This commit also modifies the doc comment for RegAltNameIndices to make it a little clearer, and removes altNameIndex from RegisterClass which is entirely unused.

I wrote this for an out-of-tree target, but so that it can be tested I've optimistically enabled the functionality for the Hexagon backend (given that HexagonRegisterInfo.td specified alternate register names and AsmParsing support was very recently added, it seemed a good target).

Patch by Alex Bradbury.

[asb]

Updated diff to include the newly-added test file.

[asb]

Adding Krzysztof, the new Hexagon code owner as reviewer.

[colinl]

Is there a reason to not emit all AltNames as register matches? This way we aren't putting a requirement on the number of altnames for a register.

Maybe in emitMatchRegisterName:

    Matches.emplace_back(Reg.TheDef->getValueAsString("AsmName"),
                         "return " + utostr(Reg.EnumValue) + ";");
+    for (auto &I: Reg.TheDef->getValueAsListOfStrings("AltNames"))
+      Matches.emplace_back(*I, "return " + utostr(Reg.EnumValue) + ";");

[asb]

Hi Colin, I agree this would be a reasonable way for the AltNames mechanism to work. However, it's not how AltNames are implemented to work in AsmWriterEmitter (which will only print an alternative altname if the appropriate AltNameIndex is associated with the record) and I feel there should be symmetry. See http://reviews.llvm.org/diffusion/L/browse/llvm/trunk/utils/TableGen/AsmWriterEmitter.cpp;254644$543

[colinl]

Hmm interesting. That code section seems to be dead, for instance I did:

-                    dag regList, RegAltNameIndex idx = NoRegAltName>
+                    dag regList>
}

-  RegAltNameIndex altNameIndex = idx
+  RegAltNameIndex altNameIndex = NoRegAltName;

And all the targets seem to still build which seems to indicate no target was using the defaulted template parameter.

I'm not sure why it would have been implemented that way. When printing we need to make a one-to-many decision, one register enum maps to multiple register names and it seems like the natural name to pick would have been the primary one. If they wanted to print a different register they would have swapped an alt name for the primary.

In the reverse direction it doesn't seem necessary to make that distinction, it's a many-to-one decision where multiple register names all map to a single register enum.

[asb]

In D14994#301864, @colinl wrote:

Hmm interesting. That code section seems to be dead, for instance I did:

-                    dag regList, RegAltNameIndex idx = NoRegAltName>
+                    dag regList>
}

-  RegAltNameIndex altNameIndex = idx
+  RegAltNameIndex altNameIndex = NoRegAltName;

And all the targets seem to still build which seems to indicate no target was using the defaulted template parameter.

That is correct, that parameter is dead and this submitted patchset removes it. Do beware when reading the AsmWriterEmitter code that the variable naming surrounding AltName and AltNameIndex can be very confusing.

I'm not sure why it would have been implemented that way. When printing we need to make a one-to-many decision, one register enum maps to multiple register names and it seems like the natural name to pick would have been the primary one. If they wanted to print a different register they would have swapped an alt name for the primary.

Well sometimes you have a distinction between the ABI and the architectural name, and you want to print the ABI name in generated assembly but refer to the architectural name in the backend. See also my bugfix D15044 to AsmWriterEmitter.

You could imagine having multiple different ABI names, and selecting the preferred one (and thus the preferred AltNameIndex) based on the Target, though of course this isn't done currently.

In the reverse direction it doesn't seem necessary to make that distinction, it's a many-to-one decision where multiple register names all map to a single register enum.

The point I think is that that .td description for any register with altnames isn't complete unless there is also a list of RegAltNameIndices which indicates how they can be addressed. This is similar to the SubRegs and SubRegIndices lists.

Perhaps @grosbach, the original author of the altname system could comment on this?

[hfinkel]

In PowerPC, we have registers with names like rNN, where NN is some number, but when parsing, we accept %rNN and NN by default, and rNN when targeting Darwin systems. Could this system handle that, or would we need some target-dependence in the selection of which altname indices were available?

[asb]

In D14994#307568, @hfinkel wrote:

In PowerPC, we have registers with names like rNN, where NN is some number, but when parsing, we accept %rNN and NN by default, and rNN when targeting Darwin systems. Could this system handle that, or would we need some target-dependence in the selection of which altname indices were available?

As is currently implemented, the AltName indices mechanism doesn't provide that flexibility. The existing code in AsmWriterEmitter just assumes that the altname indicated by the first AltNameIndex is preferred, and this patch will match any listed AltNameIndex. Having some flexibility to prefer or match different register names depending on ABI and target platform would be a logical extension, but remains to be implemented.

Off the top of my head, it seems there are two sensible ways of doing this: 1) by allowing a predicate function to be specified that will filter the AltNameIndex list, or 2) just requiring the user to specify a function that returns the AltNameIndex list in the first place, perhaps have this be a field in RegisterClass.

[hfinkel]

In D14994#308104, @asb wrote:

In D14994#307568, @hfinkel wrote:

In PowerPC, we have registers with names like rNN, where NN is some number, but when parsing, we accept %rNN and NN by default, and rNN when targeting Darwin systems. Could this system handle that, or would we need some target-dependence in the selection of which altname indices were available?

As is currently implemented, the AltName indices mechanism doesn't provide that flexibility. The existing code in AsmWriterEmitter just assumes that the altname indicated by the first AltNameIndex is preferred, and this patch will match any listed AltNameIndex. Having some flexibility to prefer or match different register names depending on ABI and target platform would be a logical extension, but remains to be implemented.

Off the top of my head, it seems there are two sensible ways of doing this: 1) by allowing a predicate function to be specified that will filter the AltNameIndex list, or 2) just requiring the user to specify a function that returns the AltNameIndex list in the first place, perhaps have this be a field in RegisterClass.

I think that, to be useful across a range of targets, we should really generate values for the AltName indices that can form a bitmask. Then, when printing we pick one (as we do now), and when matching, we can provide a bitmask of accepted AltName classes.

[sidneym]

lib/Target/Hexagon/HexagonRegisterInfo.td

94

If RegAltNameIndices is needed then the registers below will need the same treatment.

lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.h

37

Shouldn't the primary name always be favored over the alternate?

utils/TableGen/AsmMatcherEmitter.cpp

2214

So this change permits multiple possible alternates? Is that needed, can it just check if !AltNames.empty()?

[kparzysz]

I think this is fixed already. There is a flag ShouldEmitMatchRegisterAltName in AsmParser now.

[asb]

Since this patch was submitted, a similar patch has been merged upstream (providing MatchRegisterAltName). I'm abandoning this revision in favour of the current upstream support for an equivalent feature.

include/llvm/Target/Target.td

class Register<string n, list<string> altNames = []> {
   list<Register> SubRegs = [];
 
   // SubRegIndices - For each register in SubRegs, specify the SubRegIndex used
   // to address it. Sub-sub-register indices are automatically inherited from
   // SubRegs.
   list<SubRegIndex> SubRegIndices = [];
 
   // RegAltNameIndices - The alternate name indices which are valid for this
-  // register.
+  // register. Their order corresponds to the order of entries in the altNames
+  // list.
   list<RegAltNameIndex> RegAltNameIndices = [];
 
   // DwarfNumbers - Numbers used internally by gcc/gdb to identify the register.
   // These values can be determined by locating the <target>.h file in the
   // directory llvmgcc/gcc/config/<target>/ and looking for REGISTER_NAMES.  The
   // order of these names correspond to the enumeration used by gcc.  A value of
   // -1 indicates that the gcc number is undefined and -2 that register number
   // is invalid for this mode/flavour.

 class DAGOperand { }
 
 // RegisterClass - Now that all of the registers are defined, and aliases
 // between registers are defined, specify which registers belong to which
 // register classes.  This also defines the default allocation order of
 // registers by register allocators.
 //
 class RegisterClass<string namespace, list<ValueType> regTypes, int alignment,
-                    dag regList, RegAltNameIndex idx = NoRegAltName>
+                    dag regList>
   : DAGOperand {
   string Namespace = namespace;
 
   // RegType - Specify the list ValueType of the registers in this register
   // class.  Note that all registers in a register class must have the same
   // ValueTypes.  This is a list because some targets permit storing different
   // types in same register, for example vector values with 128-bit total size,
   // but different count/size of items, like SSE on x86.
class RegisterClass<string namespace, list<ValueType> regTypes, int alignment,
   int CopyCost = 1;
 
   // MemberList - Specify which registers are in this class.  If the
   // allocation_order_* method are not specified, this also defines the order of
   // allocation used by the register allocator.
   //
   dag MemberList = regList;
 
-  // AltNameIndex - The alternate register name to use when printing operands
-  // of this register class. Every register in the register class must have
-  // a valid alternate name for the given index.
-  RegAltNameIndex altNameIndex = idx;
-
   // isAllocatable - Specify that the register class can be used for virtual
   // registers and register allocation.  Some register classes are only used to
   // model instruction operand constraints, and should have isAllocatable = 0.
   bit isAllocatable = 1;
 
   // AltOrders - List of alternative allocation orders. The default order is
   // MemberList itself, and that is good enough for most targets since the
   // register allocators automatically remove reserved registers and move
class AsmParser {
   // function of the AsmParser class to call on every matched instruction.
   // This can be used to perform target specific instruction post-processing.
   string AsmParserInstCleanup  = "";
 
   // ShouldEmitMatchRegisterName - Set to false if the target needs a hand
   // written register name matcher
   bit ShouldEmitMatchRegisterName = 1;
 
+  // MatchRegisterNameShouldMatchAltNames - set to true if the
+  // MatchRegisterName function should also match register AltNames. It is
+  // hoped this will become the default in the future and the option removed.
+  bit MatchRegisterNameShouldMatchAltNames = 0;
+
   /// Does the instruction mnemonic allow '.'
   bit MnemonicContainsDot = 0;
 }
 def DefaultAsmParser : AsmParser;
 
 //===----------------------------------------------------------------------===//
 // AsmParserVariant - Subtargets can have multiple different assembly parsers
 // (e.g. AT&T vs Intel syntax on X86 for example). This class can be

lib/Target/Hexagon/Hexagon.td

def : Proc<"hexagonv55", HexagonModelV55,
            [ArchV4, ArchV5, ArchV55]>;
 def : Proc<"hexagonv60", HexagonModelV60,
            [ArchV4, ArchV5, ArchV55, ArchV60, ExtensionHVX]>;
 
 //===----------------------------------------------------------------------===//
 // Declare the target which we are implementing
 //===----------------------------------------------------------------------===//
 
+def HexagonAsmParser : AsmParser {
+  let MatchRegisterNameShouldMatchAltNames = 1;
+}
+
 def HexagonAsmParserVariant : AsmParserVariant {
   int Variant = 0;
   string TokenizingCharacters = "#()=:.<>!+*";
 }
 
 def Hexagon : Target {
   // Pull in Instruction Info:
   let InstructionSet = HexagonInstrInfo;
+  let AssemblyParsers = [HexagonAsmParser];
   let AssemblyParserVariants = [HexagonAsmParserVariant];
 }

lib/Target/Hexagon/HexagonRegisterInfo.td

 //  Declarations that describe the Hexagon register file.
 //===----------------------------------------------------------------------===//
 
 let Namespace = "Hexagon" in {
 
   class HexagonReg<bits<5> num, string n, list<string> alt = [], 
                    list<Register> alias = []> : Register<n> {
     field bits<5> Num;
+    let AltNames = alt;
     let Aliases = alias;
     let HWEncoding{4-0} = num;
   }
 
   class HexagonDoubleReg<bits<5> num, string n, list<Register> subregs,
                          list<string> alt = []> :
         RegisterWithSubRegs<n, subregs> {
     field bits<5> Num;
let Namespace = "Hexagon" in {
   class Mx<bits<1> num, string n> : HexagonReg<{0b0000, num}, n> {
     let Num = !cast<bits<5>>(num);
   }
 
   def subreg_loreg  : SubRegIndex<32>;
   def subreg_hireg  : SubRegIndex<32, 32>;
   def subreg_overflow : SubRegIndex<1, 0>;
 
+  def regaltname : RegAltNameIndex;
+
   // Integer registers.
   foreach i = 0-28 in {
     def R#i  : Ri<i, "r"#i>,  DwarfRegNum<[i]>;
   }
 
+  let RegAltNameIndices = [regaltname] in {
   def R29 : Ri<29, "r29", ["sp"]>, DwarfRegNum<[29]>;
   def R30 : Ri<30, "r30", ["fp"]>, DwarfRegNum<[30]>;
   def R31 : Ri<31, "r31", ["lr"]>, DwarfRegNum<[31]>;
+  }
 
   // Aliases of the R* registers used to hold 64-bit int values (doubles).
   let SubRegIndices = [subreg_loreg, subreg_hireg], CoveredBySubRegs = 1 in {
   def D0  : Rd< 0,  "r1:0",  [R0,  R1]>, DwarfRegNum<[32]>;
   def D1  : Rd< 2,  "r3:2",  [R2,  R3]>, DwarfRegNum<[34]>;
   def D2  : Rd< 4,  "r5:4",  [R4,  R5]>, DwarfRegNum<[36]>;
   def D3  : Rd< 6,  "r7:6",  [R6,  R7]>, DwarfRegNum<[38]>;
   def D4  : Rd< 8,  "r9:8",  [R8,  R9]>, DwarfRegNum<[40]>;

lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.h

public:
   explicit HexagonInstPrinter(MCAsmInfo const &MAI, MCInstrInfo const &MII,
                               MCRegisterInfo const &MRI);
   void printInst(MCInst const *MI, raw_ostream &O, StringRef Annot,
                  const MCSubtargetInfo &STI) override;
   virtual StringRef getOpcodeName(unsigned Opcode) const;
   void printInstruction(MCInst const *MI, raw_ostream &O);
 
   StringRef getRegName(unsigned RegNo) const;
-  static char const *getRegisterName(unsigned RegNo);
+  static const char *getRegisterName(unsigned RegNo,
+                                     unsigned AltIdx = Hexagon::NoRegAltName);
   void printRegName(raw_ostream &O, unsigned RegNo) const override;
 
   void printOperand(MCInst const *MI, unsigned OpNo, raw_ostream &O) const;
   void printExtOperand(MCInst const *MI, unsigned OpNo, raw_ostream &O) const;
   void printUnsignedImmOperand(MCInst const *MI, unsigned OpNo,
                                raw_ostream &O) const;
   void printNegImmOperand(MCInst const *MI, unsigned OpNo,
                           raw_ostream &O) const;

test/MC/Hexagon/reg_altnames.s

+# RUN: llvm-mc -triple hexagon -filetype=obj %s | llvm-objdump -d - | FileCheck %s
+
+# CHECK: 11 df 75 f1
+r17 = xor(r21, lr)
+
+# CHECK: 1d df 35 f3
+sp = sub(lr, r21)
+
+# CHECK: 15 c0 3e 71
+fp.l = #21

utils/TableGen/AsmMatcherEmitter.cpp

static void emitMatchRegisterName(CodeGenTarget &Target, Record *AsmParser,
   std::vector<StringMatcher::StringPair> Matches;
   const auto &Regs = Target.getRegBank().getRegisters();
   for (const CodeGenRegister &Reg : Regs) {
     if (Reg.TheDef->getValueAsString("AsmName").empty())
       continue;
 
     Matches.emplace_back(Reg.TheDef->getValueAsString("AsmName"),
                          "return " + utostr(Reg.EnumValue) + ";");
+
+    if (AsmParser->getValueAsBit("MatchRegisterNameShouldMatchAltNames")) {
+      const auto &AltNames = Reg.TheDef->getValueAsListOfStrings("AltNames");
+      const auto &RegAltNameIndices =
+          Reg.TheDef->getValueAsListOfDefs("RegAltNameIndices");
+
+      unsigned AltNameIndex = 0;
+      for (const Record *RegAltNameIndex : RegAltNameIndices) {
+        if (AltNames.size() <= AltNameIndex)
+          PrintFatalError(Reg.TheDef->getLoc(),
+                          "Register definition missing alt name for '" +
+                              RegAltNameIndex->getName() + "'.");
+        const StringRef &AltName = AltNames[AltNameIndex];
+        if (AltName.empty())
+          continue;
+        Matches.emplace_back(AltName, "return " + utostr(Reg.EnumValue) + ";");
+        AltNameIndex++;
+      }
+    }
   }
 
   OS << "static unsigned MatchRegisterName(StringRef Name) {\n";
 
   StringMatcher("Name", Matches, OS).Emit();
 
   OS << "  return 0;\n";
   OS << "}\n\n";