[PATCH] D43916: Named VReg support for MIR

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

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)



================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:434
 
+  std::string getVRegName(unsigned Reg) const {
+    return VReg2Name.inBounds(Reg) ? VReg2Name[Reg] : "";
----------------
thegameg wrote:
> plotfi wrote:
> > plotfi wrote:
> > > thegameg wrote:
> > > > StringRef might be better here?
> > > Yeah. I'll update the patch.
> > Tried stringref, had some bugs in debug mode. Changed it back. 
> What was the issue?
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.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:86-87
+
+  /// StringSet that is used to unique the vreg names.
+  StringSet<> VRegNames;
+
----------------
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).


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:711
+  unsigned createVirtualRegister(const TargetRegisterClass *RegClass,
+                                 std::string name = "");
 
----------------
thegameg wrote:
> `Name` instead of `name`.
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).


================
Comment at: lib/CodeGen/MIRParser/MILexer.cpp:414
+static Cursor lexNamedVirtualRegister(Cursor C, MIToken &Token) {
+  auto Range = C;
+  C.advance(); // Skip '%'
----------------
Just type `Cursor` instead of `auto`?


================
Comment at: lib/CodeGen/MIRParser/MILexer.h:174-175
     return Kind == NamedRegister || Kind == underscore ||
+           Kind == NamedVirtualRegister ||
            Kind == VirtualRegister;
   }
----------------
Either put each `==` on a single line, or merge so you have 2 on each line.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1202-1207
+  bool isNamedVreg = Token.is(MIToken::NamedVirtualRegister);
   if (parseRegister(Reg, RegInfo))
     return true;
-  lex();
+  if (isNamedVreg && !Token.is(MIToken::NamedVirtualRegister)) {
+    if (parseRegisterClassOrBank(*RegInfo))
+        return true;
----------------
thegameg wrote:
> plotfi wrote:
> > bogner wrote:
> > > 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.
> > 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. 
> 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.
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.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:970
+  const TargetRegisterClass *RC = nullptr;
+  for (unsigned i = 0; i < TRI->getNumRegClasses(); i++) {
+    const TargetRegisterClass *curRC = TRI->getRegClass(i);
----------------
Use
```
for (unsigned I = 0, N = TRI->getNumRegClasss(); I < N; ++I) //...
```


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:985
+  bool WasInserted =
+    Names2Regs.insert(std::make_pair(Name.lower(), Reg)).second;
+
----------------
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.


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:1203
   VRegInfo *RegInfo;
+  const bool isNamedVreg = Token.is(MIToken::NamedVirtualRegister);
   if (parseRegister(Reg, RegInfo))
----------------
local variables should start with uppercase (and most people don't bother putting a `const` on scalars, but why not).


Repository:
  rL LLVM

https://reviews.llvm.org/D43916