TargetRegisterInfo: Add getRegAsmName()

[• tstellarAMD]

Summary

The motivation for this new function is to move an invalid assumption
about the relationship between the names of register definitions in
tablegen files and their assembly names into TargetRegisterInfo, so that
we can begin working on fixing this assumption.

The current problem is that if you have a register definition in
TableGen like:

def MYReg0 : Register<"r0", 0>;

The function TargetLowering::getRegForInlineAsmConstraint() derives the
assembly name from the tablegen name: "MyReg0" rather than the given
assembly name "r0". This is working, because on most targets the
tablegen name and the assembly names are case insensitive matches for
each other (e.g. def EAX : X86Reg<"eax", ...>

getRegAsmName() will allow targets to override this default assumption and
return the correct assembly name.

[echristo]

Seems like extra complexity versus just doing what the PPC port did?

[• tstellarAMD]

In D15614#322025, @echristo wrote:

Seems like extra complexity versus just doing what the PPC port did?

How did the PPC port solve this? I haven't found anything in PPCISelLowering::getRegForInlineAsmConstraint() that seems to work-around the issue.

[echristo]

In D15614#322249, @tstellarAMD wrote:

In D15614#322025, @echristo wrote:

Seems like extra complexity versus just doing what the PPC port did?

How did the PPC port solve this? I haven't found anything in PPCISelLowering::getRegForInlineAsmConstraint() that seems to work-around the issue.

They did it via this:

class PPCReg<string n> : Register<n> {
  let Namespace = "PPC";
}

// We identify all our registers with a 5-bit ID, for consistency's sake.                                                                                                           

// GPR - One of the 32 32-bit general-purpose registers                                                                                                                             
class GPR<bits<5> num, string n> : PPCReg<n> {
  let HWEncoding{4-0} = num;
}

Where it just sets the register rather than using the default. Take a look at lib/Target/PPCRegisterInfo.td.

[• tstellarAMD]

In D15614#324163, @echristo wrote:

They did it via this:

class PPCReg<string n> : Register<n> {
  let Namespace = "PPC";
}

// We identify all our registers with a 5-bit ID, for consistency's sake.                                                                                                           

// GPR - One of the 32 32-bit general-purpose registers                                                                                                                             
class GPR<bits<5> num, string n> : PPCReg<n> {
  let HWEncoding{4-0} = num;
}

Where it just sets the register rather than using the default. Take a look at lib/Target/PPCRegisterInfo.td.

I'm not sure we are talking about this same thing, because this code appears to affect the encoding value and not the assembly string.

[hfinkel]

In D15614#338347, @tstellarAMD wrote:

In D15614#324163, @echristo wrote:

They did it via this:

class PPCReg<string n> : Register<n> {
  let Namespace = "PPC";
}

// We identify all our registers with a 5-bit ID, for consistency's sake.                                                                                                           

// GPR - One of the 32 32-bit general-purpose registers                                                                                                                             
class GPR<bits<5> num, string n> : PPCReg<n> {
  let HWEncoding{4-0} = num;
}

Where it just sets the register rather than using the default. Take a look at lib/Target/PPCRegisterInfo.td.

I'm not sure we are talking about this same thing, because this code appears to affect the encoding value and not the assembly string.

That's right, this is a different issue. PPC does have a workaround for this:

// r[0-9]+ are used, on PPC64, to refer to the corresponding 64-bit registers
// (which we call X[0-9]+). If a 64-bit value has been requested, and a
// 32-bit GPR has been selected, then 'upgrade' it to the 64-bit parent
// register.
// FIXME: If TargetLowering::getRegForInlineAsmConstraint could somehow use
// the AsmName field from *RegisterInfo.td, then this would not be necessary.
if (R.first && VT == MVT::i64 && Subtarget.isPPC64() &&
    PPC::GPRCRegClass.contains(R.first))
  return std::make_pair(TRI->getMatchingSuperReg(R.first,
                          PPC::sub_32, &PPC::G8RCRegClass),
                        &PPC::G8RCRegClass);

and I'd like to get rid of this.

This seems like a good first step; LGTM.

[echristo]

Sure. I guess I was hoping for something else, but I don't have a strong
enough opinion when someone else really wants something

include/llvm/Target/TargetRegisterInfo.h

public:
   /// this method. This method may modify or replace the specified instruction,
   /// as long as it keeps the iterator pointing at the finished product.
   /// SPAdj is the SP adjustment due to call frame setup instruction.
   /// FIOperandNum is the FI operand number.
   virtual void eliminateFrameIndex(MachineBasicBlock::iterator MI,
                                    int SPAdj, unsigned FIOperandNum,
                                    RegScavenger *RS = nullptr) const = 0;
 
+  /// Return the assembly name for \p Reg.
+  virtual std::string getRegAsmName(unsigned Reg) const {
+    // FIXME: We are assuming that the assembly name is equal to the TableGen
+    // name converted to lower case
+    //
+    // The TableGen name is the name of the definition for this register in the
+    // target's tablegen files.  For example, the TableGen name of
+    // def EAX : Register <...>; is "EAX"
+    return StringRef(getName(Reg)).lower();
+  }
+
   //===--------------------------------------------------------------------===//
   /// Subtarget Hooks
 
   /// \brief SrcRC and DstRC will be morphed into NewRC if this returns true.
   virtual bool shouldCoalesce(MachineInstr *MI,
                               const TargetRegisterClass *SrcRC,
                               unsigned SubReg,
                               const TargetRegisterClass *DstRC,

lib/CodeGen/SelectionDAG/TargetLowering.cpp

for (TargetRegisterInfo::regclass_iterator RCI = RI->regclass_begin(),
 
     // If none of the value types for this register class are valid, we
     // can't use it.  For example, 64-bit reg classes on 32-bit targets.
     if (!isLegalRC(RC))
       continue;
 
     for (TargetRegisterClass::iterator I = RC->begin(), E = RC->end();
          I != E; ++I) {
-      if (RegName.equals_lower(RI->getName(*I))) {
+      if (RegName.equals(RI->getRegAsmName(*I))) {
         std::pair<unsigned, const TargetRegisterClass*> S =
           std::make_pair(*I, RC);
 
         // If this register class has the requested value type, return it,
         // otherwise keep searching and return the first class found
         // if no other is found which explicitly has the requested type.
         if (RC->hasType(VT))
           return S;