[PATCH] D43916: Named VReg support for MIR
Francis Visoiu Mistrih via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 19 14:13:25 PDT 2018
thegameg 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) &&
----------------
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)`).
================
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;
----------------
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?
================
Comment at: lib/CodeGen/MIRParser/MIRParser.cpp:519
+ };
+ for (auto VRegInfos : VRegInfosMaps) {
+ for (auto P : *VRegInfos) {
----------------
```
for (auto *VRegInfos : {&PFS.VRegInfos, &PFS.VRegInfosNamed})
```
should do the trick.
================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:176
TheDelegate->MRI_NoteNewVirtualRegister(Reg);
+ insertVRegByName(Name, Reg);
return Reg;
----------------
Not needed anymore, right?
Repository:
rL LLVM
https://reviews.llvm.org/D43916
More information about the llvm-commits
mailing list