[PATCH] D43916: Named VReg support for MIR

Puyan Lotfi via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 19 14:52:07 PDT 2018


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


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:91
 VRegInfo &PerFunctionMIParsingState::getVRegInfo(unsigned Num) {
-  auto I = VRegInfos.insert(std::make_pair(Num, nullptr));
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+  const bool isNamed = TargetRegisterInfo::isVirtualRegister(Num) &&
----------------
thegameg wrote:
> 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)`).
I could do a separate function. 


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:2534
+    if (!Names2VRegs.insert(std::make_pair(RegName, Reg)).second)
+      return error(Twine("Invalid register name '") + RegName + "'");
+    return Reg;
----------------
thegameg wrote:
> 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?
So just do Names2VRegs.insert(std::make_pair(RegName, Reg)) ?


================
Comment at: lib/CodeGen/MIRParser/MIRParser.cpp:519
+  };
+  for (auto VRegInfos : VRegInfosMaps) {
+    for (auto P : *VRegInfos) {
----------------
thegameg wrote:
> ```
> for (auto *VRegInfos : {&PFS.VRegInfos, &PFS.VRegInfosNamed})
> ```
> should do the trick.
Ah, yeah thanks. 


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:176
     TheDelegate->MRI_NoteNewVirtualRegister(Reg);
+  insertVRegByName(Name, Reg);
   return Reg;
----------------
thegameg wrote:
> Not needed anymore, right?
True, but I figured why not keep it consistent? I can remove it. 


Repository:
  rL LLVM

https://reviews.llvm.org/D43916





More information about the llvm-commits mailing list