[PATCH] D43916: Named VReg support for MIR

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 15 inline comments as done.
plotfi added inline comments.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:434
 
+  std::string getVRegName(unsigned Reg) const {
+    return VReg2Name.inBounds(Reg) ? VReg2Name[Reg] : "";
----------------
MatzeB wrote:
> 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.
I see what you mean. Return a StringRef, take a std::string on creation. 


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:86-87
+
+  /// StringSet that is used to unique the vreg names.
+  StringSet<> VRegNames;
+
----------------
MatzeB wrote:
> 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).
That sounds good. Should I do this in a subsequent patch? Should I leave out the name check here? 


Repository:
  rL LLVM

https://reviews.llvm.org/D43916