Named VReg support for MIR

[plotfi]

Summary

This patch adds support for named vreg operands in MIR.

[plotfi]

lib/CodeGen/MachineRegisterInfo.cpp

180

I missed a line of critical code here, will add:

+ VReg2Name.grow(Reg);

[thegameg]

Thanks for the patch!

Few remarks:

• The named vregs are not mapped back to the same vreg id. Take this example:

---
name:            Proc
registers:
  - { id: 0, class: gpr32, preferred-register: '' }
body:             |
  bb.0:
    %foo:gpr64 = IMPLICIT_DEF

This will give us this:

registers:
  - { id: 0, class: gpr32, preferred-register: '' }
  - { id: 1, class: gpr64, preferred-register: '' }
  - { id: 2, class: gpr64, preferred-register: '' }

• There is no way to specify something like a preferred-register for the named regs, or explicitly force a class on it.

• Some of the remarks in comments.

I think it's good approach to keep an unique ID, since it doesn't request much change from MachineRegisterInfo.

include/llvm/CodeGen/MachineRegisterInfo.h

429

StringRef might be better here?

[thegameg]

include/llvm/CodeGen/TargetRegisterInfo.h

1155

Should we use MRI->getTargetRegisterInfo and only pass MRI to printReg?

lib/CodeGen/MIRParser/MIParser.cpp

968

Should probably first look in PFS.Names2RegBanks or just re-use parseRegisterClassOrRegBank.

982

You're first creating a virtual register, then you're calling PFS.getVRegInfo(Reg) at the end of the function, which creates the VRegInfo by also creating another virtual register.

Basically for this:

---
name:            Proc
tracksRegLiveness: true
body:             |
  bb.0.bb:
    %foo:gpr64 = IMPLICIT_DEF
...

you end up with

registers:
  - { id: 0, class: gpr64, preferred-register: '' }
  - { id: 1, class: gpr64, preferred-register: '' }

lib/CodeGen/TargetRegisterInfo.cpp

96

StringRef here too?

[plotfi]

In D43916#1024336, @thegameg wrote:

Thanks for the patch!

Few remarks:

• The named vregs are not mapped back to the same vreg id. Take this example:

---
name:            Proc
registers:
  - { id: 0, class: gpr32, preferred-register: '' }
body:             |
  bb.0:
    %foo:gpr64 = IMPLICIT_DEF

This will give us this:

registers:
  - { id: 0, class: gpr32, preferred-register: '' }
  - { id: 1, class: gpr64, preferred-register: '' }
  - { id: 2, class: gpr64, preferred-register: '' }

• There is no way to specify something like a preferred-register for the named regs, or explicitly force a class on it.

• Some of the remarks in comments.

I think it's good approach to keep an unique ID, since it doesn't request much change from MachineRegisterInfo.

I think this could be added in a future patch. I dont think it would be hard to reference a table for the proper pre-allocated vreg. Currently it creates the vreg at parsing time for the named vregs. Does this sound like an ok design for the first version of this patch to you?

[plotfi]

include/llvm/CodeGen/MachineRegisterInfo.h

429

Yeah. I'll update the patch.

include/llvm/CodeGen/TargetRegisterInfo.h

1155

Oh! that sounds pretty good. Hmm. The question is, does all the calling code where we call printReg have an MRI. From my initial survey that doesn't appear to be the case.

[thegameg]

In D43916#1024515, @plotfi wrote:

I think this could be added in a future patch.

I think this should be done in this patch, because if you run passes manually like:

$ llc -run-pass pass1 mir0 -o mir1
$ llc -run-pass pass1 mir1 -o mir2
$ llc -run-pass pass1 mir2 -o mir3
$ llc -run-pass pass1 mir3 -o -

You'll end up with 4 vreg entries when only the last one actually matters.

[bogner]

include/llvm/CodeGen/MachineRegisterInfo.h

79

Don't repeat the name in the comment.

86–88

No need to repeat yourself, one sentence will do here.

lib/CodeGen/MIRParser/MIParser.cpp

1202–1207

This doesn't seem right. Parsing a named virtual register should either always or never parse the register class. As is, it seems that it sometimes does, so you have to do this complicated dance here.

lib/CodeGen/MachineRegisterInfo.cpp

165

Here too, name can probably be a StringRef (also it should be called Name) - we'll happily create a std::string if/when we actually store it in VRegNames.

180

Don't leave commented out code laying around, please.

[plotfi]

.gitignore

26 ↗

(On Diff #136831)

I will not commit this. Added by accident.

include/llvm/CodeGen/MachineRegisterInfo.h

86–88

Ah, sorry. I was just doing the same as the VRegInfo comment.

429

Tried stringref, had some bugs in debug mode. Changed it back.

lib/CodeGen/MIRParser/MIParser.cpp

1202–1207

In the case of the vreg assignment it always parses it, in the case of a use it doesn't need to. For example:

%foo:gpr32 = ...
$5 = ADD %foo, %foo:gpr32

Hmm, I should probably make it never parse the regclass in the case of use.

[jcranmer]

As a drive-by feature request, could you add a MachineRegisterInfo::setName method?

[plotfi]

In D43916#1026022, @jcranmer wrote:

As a drive-by feature request, could you add a MachineRegisterInfo::setName method?

Sure!

[thegameg]

include/llvm/CodeGen/MachineRegisterInfo.h

429

What was the issue?

708

Name instead of name.

include/llvm/CodeGen/TargetRegisterInfo.h

1155

You should be able to grab an MRI at any point but I guess it's not worth changing it everywhere. Maybe in a future patch.

lib/CodeGen/MIRParser/MIParser.cpp

1202–1207

I don't think you should special-case this here. We can also have regclasses repeated on uses. What about using something like:

VRegInfo &PerFunctionMIParsingState::getVRegInfo(StringRef Name)

and follow the way non-named vregs are created.

[MatzeB]

Thanks for working on this; I'm looking forward to have this!

• Please upload patches with full context next time (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

include/llvm/CodeGen/MachineRegisterInfo.h

86–87

I wonder if we shouldn't better move the duplicate name check to the MachineVerifier to save creating this datastructure by default?

I also wonder if there is a way to change the comment, as the first time I was reading this I expected this to be used to unique the storage used when the same string is used multiple times (which of course makes little sense for vregs that should have unique names).

429

This really should use StringRef! Or we will get an extra malloc+memcpy for the string each time you call this.

StringRef could fail here if someone keeps a reference around and changes the VReg2Name mapping later. But the correct fix for that would be that the caller makes a copy into an std::string and not forcing every user of the API to do so.

708

I'd rather see const char *Name or StringRef Name to avoid extra string copies/allocations. (I'm not sure about it, but std::string name = "" could also cause an allocation every time this is called).

lib/CodeGen/MIRParser/MILexer.cpp

414

Just type Cursor instead of auto?

lib/CodeGen/MIRParser/MILexer.h

174–175

Either put each == on a single line, or merge so you have 2 on each line.

lib/CodeGen/MIRParser/MIParser.cpp

970

Use

for (unsigned I = 0, N = TRI->getNumRegClasss(); I < N; ++I) //...

985

LLVM IR is case sensitive with the names, so is the logic that you added to MachineRegisterInfo. I think we should make this case sensitive too.

1202–1207

I think you can construct a case where you never define a vreg and just use it in undef operands. So at least being able to specify register classes on uses make sense.

1203

local variables should start with uppercase (and most people don't bother putting a const on scalars, but why not).

[thegameg]

Also, please update MIRLangRef for this patch as well, thanks!

[plotfi]

include/llvm/CodeGen/MachineRegisterInfo.h

86–87

That sounds good. Should I do this in a subsequent patch? Should I leave out the name check here?

429

I see what you mean. Return a StringRef, take a std::string on creation.

[plotfi]

I have made some more changes to address all of the feedback. Please let me know what else I can do to improve this.

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

986

I think the code would be much simpler if we do the same thing as parseVirtualRegister here. By that I mean call createIncompleteVirtualRegister, create a VRegInfo object without any reg class, then let the rest of the code parse the reg class and follow the usual path. I also think that you don't need to explicitly parse the reg class in this function since the user will get an error like Cannot determine class/bank of virtual register if no reg class is specified at def time anyway. This would allow you to get rid of the other change you have where you conditionally lex things.

Please let me know if what I'm saying doesn't make sense in this case as I haven't really tried to implement this.

[plotfi]

lib/CodeGen/MIRParser/MIParser.cpp

986

I don't understand this completely, but I think one thing I could do is add a name parameter to createIncompleteVirtualRegister and to getVRegInfo and you're saying parseRegisterClassOrBank will just finish building the vreg for me?

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

986

What I mean by that is:

• the only way to parse a named virtual register is through parseRegisterOperand
• parseRegisterOperand handles all kinds of registers, sub registers, register classes / banks.
• (1) the only thing parseVirtualRegister does is create an empty VRegInfo and create an "incomplete" virtual register.
• (2) the call to parseRegisterClassOrBank in parseRegisterOperand will continue the parsing.

This is the part where I am not completely sure:

• can you do the same as (1), with additional steps as inserting the name into Names2VRegs and MRI::VReg2Name
• will re-using (2) work with no additional changes here?

[bogner]

include/llvm/CodeGen/MachineRegisterInfo.h

430

Very minor, but calling c_str() here causes an extra call to strlen - better to construct the StringRef from the std::string directly.

[plotfi]

lib/CodeGen/MIRParser/MIParser.cpp

986

Actually the only reason I created parseVirtualRegister was to use Names2VRegs. Since we use a different sigil for the physregs I think we should use a different table to map the names.

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

986

Yes that makes sense. We should be able to use %eax and $eax that map to different registers.

[thegameg]

include/llvm/CodeGen/MachineRegisterInfo.h

430

I think what @bogner meant here is to make use of StringRef’s constructor that takes an std::string (and uses the .size() from that object) instead of the constructor taking a const char * (which has to call strlen to get the actual size).

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

91

I still think this is a bit confusing because sometimes Num is a register, sometimes it's an ID. Why not make a separate function where you do what you do in getVirtualRegisterByName + what's needed from here (something like VRegInfo &PerFunctionMIParsingState::getVRegInfo(StringRef Num)).

2534

It's a little unclear how to differentiate from an error (result = 1) or a register with the value 1. How about inlining this in the caller since it's only used once?

lib/CodeGen/MIRParser/MIRParser.cpp

519 ↗

(On Diff #139000)

for (auto *VRegInfos : {&PFS.VRegInfos, &PFS.VRegInfosNamed})

should do the trick.

lib/CodeGen/MachineRegisterInfo.cpp

176

Not needed anymore, right?

[plotfi]

lib/CodeGen/MIRParser/MIParser.cpp

91

I could do a separate function.

2534

So just do Names2VRegs.insert(std::make_pair(RegName, Reg)) ?

lib/CodeGen/MIRParser/MIRParser.cpp

519 ↗

(On Diff #139000)

Ah, yeah thanks.

lib/CodeGen/MachineRegisterInfo.cpp

176

True, but I figured why not keep it consistent? I can remove it.

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

2534

Hmm now that I'm thinking about it, if this insertion doesn't succeed it means that the key is already in the map. And since you checked at the beginning of this function I assume this can't happen?

lib/CodeGen/MachineRegisterInfo.cpp

176

Isn't createVirtualRegister always calling createIncompleteVirtualRegister?

[plotfi]

lib/CodeGen/MIRParser/MIParser.cpp

2534

Yeah. I removed the check anyways.

[thegameg]

lib/CodeGen/MIRParser/MIParser.cpp

145

I still think this should be merged with VRegInfosNamed in this patch, and end up with something like:

DenseMap<std::string, VRegInfo*> VRegInfosNamed;

[llvm-commits]

Could this bit be in a subsequent patch?
Right now im not exactly sure what the right way to go about this is because you have a vreginfos but they are named in much the way physregs are named. The named physregs have the map in MIParser. Still thinking of the right way to do this.

PL

​Sent with ProtonMail Secure Email.​

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

[thegameg]

lib/CodeGen/MIRParser/MIRParser.cpp

545 ↗

(On Diff #140380)

The error will print: “Cannot determine class/bank of virtual register in function ‘func’”

Before this change P.first was the vreg ID which was a useful error message when you forget the reg class on a def.

Can you print the register ID if it’s a vreg and its name if it’s a named vreg?

[plotfi]

Ah yeah that was a silly mistake. Thanks

PL

​Sent with ProtonMail Secure Email.​

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

[thegameg]

LGTM, thanks!

[plotfi]

Awesome thanks. Will land asap. Thank You Francis!

PL

​Sent with ProtonMail Secure Email.​

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

include/llvm/CodeGen/MachineRegisterInfo.h

 #include "llvm/ADT/IndexedMap.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/CodeGen/GlobalISel/RegisterBank.h"
 #include "llvm/CodeGen/LowLevelType.h"

              VirtReg2IndexFunctor>
       VRegInfo;
 
+  /// Map for recovering vreg name from vreg number.
+  /// This map is used by the MIR Printer.
+  IndexedMap<std::string, VirtReg2IndexFunctor> VReg2Name;
+
+  /// StringSet that is used to unique vreg names.
+  StringSet<> VRegNames;
+
   /// The flag is true upon \p UpdatedCSRs initialization
   /// and false otherwise.
   bool IsUpdatedCSRsInitialized;

   /// specified register (it may be live-in).
   bool def_empty(unsigned RegNo) const { return def_begin(RegNo) == def_end(); }
 
+  StringRef getVRegName(unsigned Reg) const {
+    return VReg2Name.inBounds(Reg) ? VReg2Name[Reg].c_str() : "";
+  }
+
   /// Return true if there is exactly one operand defining the specified
   /// register.
   bool hasOneDef(unsigned RegNo) const {

   bool recomputeRegClass(unsigned Reg);
 
   /// createVirtualRegister - Create and return a new virtual register in the
-  /// function with the specified register class.
-  unsigned createVirtualRegister(const TargetRegisterClass *RegClass);
+  /// function with the specified register class. Optionally provide a name.
+  unsigned createVirtualRegister(const TargetRegisterClass *RegClass,
+                                 StringRef Name = "");
 
   /// Accessor for VRegToType. This accessor should only be used
   /// by global-isel related work.

include/llvm/CodeGen/TargetRegisterInfo.h

 ///
 /// Usage: OS << printReg(Reg, TRI, SubRegIdx) << '\n';
 Printable printReg(unsigned Reg, const TargetRegisterInfo *TRI = nullptr,
-                   unsigned SubRegIdx = 0);
+                   unsigned SubRegIdx = 0,
+                   const MachineRegisterInfo *MRI = nullptr);
 
 /// Create Printable object to print register units on a \ref raw_ostream.
 ///

lib/CodeGen/MIRParser/MILexer.h

     Identifier,
     IntegerType,
     NamedRegister,
+    NamedVirtualRegister,
     MachineBasicBlockLabel,
     MachineBasicBlock,
     PointerType,

 
   bool isRegister() const {
     return Kind == NamedRegister || Kind == underscore ||
-           Kind == VirtualRegister;
+           Kind == NamedVirtualRegister || Kind == VirtualRegister;
   }
 
   bool isRegisterFlag() const {

lib/CodeGen/MIRParser/MILexer.cpp

   return isIdentifierChar(C) && C != '.';
 }
 
+static Cursor lexNamedVirtualRegister(Cursor C, MIToken &Token) {
+  Cursor Range = C;
+  C.advance(); // Skip '%'
+  while (isRegisterChar(C.peek()))
+    C.advance();
+  Token.reset(MIToken::NamedVirtualRegister, Range.upto(C))
+      .setStringValue(Range.upto(C).drop_front(1)); // Drop the '%'
+  return C;
+}
+
 static Cursor maybeLexRegister(Cursor C, MIToken &Token,
                                ErrorCallbackType ErrorCallback) {
   if (C.peek() != '%' && C.peek() != '$')

     if (isdigit(C.peek(1)))
       return lexVirtualRegister(C, Token);
 
-    // ErrorCallback(Token.location(), "Named vregs are not yet supported.");
+    if (isRegisterChar(C.peek(1)))
+      return lexNamedVirtualRegister(C, Token);
+
     return None;
   }
 

lib/CodeGen/MIRParser/MIParser.h

                             const Name2RegBankMap &Names2RegBanks);
 
   VRegInfo &getVRegInfo(unsigned VReg);
+  VRegInfo &populateVRegInfo(unsigned VReg);
 };
 
 /// Parse the machine basic block definitions, and skip the machine

lib/CodeGen/MIRParser/MIParser.cpp

   return *I.first->second;
 }
 
+VRegInfo &PerFunctionMIParsingState::populateVRegInfo(unsigned Num) {
+  auto I = VRegInfos.insert(std::make_pair(Num, nullptr));
+  if (I.second) {
+    VRegInfo *Info = new (Allocator) VRegInfo;
+    Info->VReg = Num;
+    I.first->second = Info;
+  }
+  return *I.first->second;
+}
+
+
+
 namespace {
 
 /// A wrapper struct around the 'MachineOperand' struct that includes a source

 
   bool parseNamedRegister(unsigned &Reg);
   bool parseVirtualRegister(VRegInfo *&Info);
+  bool parseNamedVirtualRegister(unsigned &Reg, VRegInfo *&Info);
   bool parseRegister(unsigned &Reg, VRegInfo *&VRegInfo);
   bool parseRegisterFlag(unsigned &Flags);
   bool parseRegisterClassOrBank(VRegInfo &RegInfo);

   return false;
 }
 
+bool MIParser::parseNamedVirtualRegister(unsigned &Reg, VRegInfo *&Info) {
+  assert(Token.is(MIToken::NamedVirtualRegister) && "Needs NamedRegister token");
+  StringRef Name = Token.stringValue();
+  if (!getRegisterByName(Name, Reg)) {
+    lex();
+    Info = &PFS.getVRegInfo(Reg);
+    return false;
+  }
+
+  lex();
+  if (!Token.is(MIToken::colon))
+    return error(Twine("unknown register name '") + Name + "'");
+
+  lex();
+  StringRef Type = Token.stringValue();
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+  const TargetRegisterInfo *TRI = MRI.getTargetRegisterInfo();
+
+  const TargetRegisterClass *RC = nullptr;
+  for (unsigned I = 0, N = TRI->getNumRegClasses(); I < N; ++I) {
+    const TargetRegisterClass *CurRC = TRI->getRegClass(I);
+    StringRef NameRC(TRI->getRegClassName(CurRC));
+
+    if (NameRC.lower() == Type.lower()) {
+      RC = CurRC;
+      break;
+    }
+  }
+
+  if (RC == nullptr)
+    return error(Twine("Invalid Register Class '") + Name + "'");
+
+  Reg = MRI.createVirtualRegister(RC, Name.str());
+  bool WasInserted =
+    Names2Regs.insert(std::make_pair(Name.lower(), Reg)).second;
+
+  assert(WasInserted && "Expected regs to be uniq case-insensitively");
+  if (!WasInserted)
+    return error(Twine("Invalid register name '") + Name + "'");
+
+  Info = &PFS.populateVRegInfo(Reg);
+  return parseRegisterClassOrBank(*Info);
+}
+
 bool MIParser::parseVirtualRegister(VRegInfo *&Info) {
   assert(Token.is(MIToken::VirtualRegister) && "Needs VirtualRegister token");
   unsigned ID;

     return false;
   case MIToken::NamedRegister:
     return parseNamedRegister(Reg);
+  case MIToken::NamedVirtualRegister:
+    return parseNamedVirtualRegister(Reg, Info);
   case MIToken::VirtualRegister:
     if (parseVirtualRegister(Info))
       return true;

     return error("expected a register after register flags");
   unsigned Reg;
   VRegInfo *RegInfo;
+  const bool IsNamedVreg = Token.is(MIToken::NamedVirtualRegister);
   if (parseRegister(Reg, RegInfo))
     return true;
-  lex();
+  if (!IsNamedVreg)
+    lex();
   unsigned SubReg = 0;
   if (Token.is(MIToken::dot)) {
     if (parseSubRegisterIndex(SubReg))

   case MIToken::underscore:
   case MIToken::NamedRegister:
   case MIToken::VirtualRegister:
+  case MIToken::NamedVirtualRegister:
     return parseRegisterOperand(Dest, TiedDefIdx);
   case MIToken::IntegerLiteral:
     return parseImmediateOperand(Dest);

lib/CodeGen/MIRPrinter.cpp

     unsigned Reg = TargetRegisterInfo::index2VirtReg(I);
     yaml::VirtualRegisterDefinition VReg;
     VReg.ID = I;
+    if (RegInfo.getVRegName(Reg) != "")
+      continue;
     ::printRegClassOrBank(Reg, VReg.Class, RegInfo, TRI);
     unsigned PreferredReg = RegInfo.getSimpleHint(Reg);
     if (PreferredReg)

lib/CodeGen/MachineOperand.cpp

       OS << "debug-use ";
     if (TargetRegisterInfo::isPhysicalRegister(getReg()) && isRenamable())
       OS << "renamable ";
-    OS << printReg(Reg, TRI);
+
+    const MachineRegisterInfo *MRI = nullptr;
+    if (TargetRegisterInfo::isVirtualRegister(Reg)) {
+      if (const MachineFunction *MF = getMFIfAvailable(*this)) {
+        MRI = &MF->getRegInfo();
+      }
+    }
+
+    OS << printReg(Reg, TRI, 0, MRI);
     // Print the sub register.
     if (unsigned SubReg = getSubReg()) {
       if (TRI)

lib/CodeGen/MachineRegisterInfo.cpp

 /// function with the specified register class.
 ///
 unsigned
-MachineRegisterInfo::createVirtualRegister(const TargetRegisterClass *RegClass){
+MachineRegisterInfo::createVirtualRegister(const TargetRegisterClass *RegClass,
+                                           StringRef Name) {
   assert(RegClass && "Cannot create register without RegClass!");
   assert(RegClass->isAllocatable() &&
          "Virtual register RegClass must be allocatable.");
+  assert((Name.empty() || VRegNames.find(Name) == VRegNames.end()) &&
+         "Named VRegs Must be Unique.");
 
   // New virtual register number.
   unsigned Reg = createIncompleteVirtualRegister();
   VRegInfo[Reg].first = RegClass;
   if (TheDelegate)
     TheDelegate->MRI_NoteNewVirtualRegister(Reg);
+
+  if (!Name.empty()) {
+    VRegNames.insert(Name);
+    VReg2Name.grow(Reg);
+    VReg2Name[Reg] = Name.str();
+  }
   return Reg;
 }
 

lib/CodeGen/TargetRegisterInfo.cpp

 namespace llvm {
 
 Printable printReg(unsigned Reg, const TargetRegisterInfo *TRI,
-                   unsigned SubIdx) {
-  return Printable([Reg, TRI, SubIdx](raw_ostream &OS) {
+                   unsigned SubIdx, const MachineRegisterInfo *MRI) {
+  return Printable([Reg, TRI, SubIdx, MRI](raw_ostream &OS) {
     if (!Reg)
       OS << "$noreg";
     else if (TargetRegisterInfo::isStackSlot(Reg))
       OS << "SS#" << TargetRegisterInfo::stackSlot2Index(Reg);
-    else if (TargetRegisterInfo::isVirtualRegister(Reg))
-      OS << '%' << TargetRegisterInfo::virtReg2Index(Reg);
+    else if (TargetRegisterInfo::isVirtualRegister(Reg)) {
+      StringRef name = MRI ? MRI->getVRegName(Reg) : "";
+      if (name != "") {
+        OS << '%' << name;
+      } else {
+        OS << '%' << TargetRegisterInfo::virtReg2Index(Reg);
+      }
+    }
     else if (!TRI)
       OS << '$' << "physreg" << Reg;
     else if (Reg < TRI->getNumRegs()) {