This patch adds support for named vreg operands in MIR.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/CodeGen/MachineRegisterInfo.cpp | ||
---|---|---|
180 | I missed a line of critical code here, will add: + VReg2Name.grow(Reg); |
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? |
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? |
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?
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.
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. |
.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 = ... Hmm, I should probably make it never parse the regclass in the case of use. |
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. |
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). |
I have made some more changes to address all of the feedback. Please let me know what else I can do to improve this.
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. |
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? |
lib/CodeGen/MIRParser/MIParser.cpp | ||
---|---|---|
986 | What I mean by that is:
This is the part where I am not completely sure:
|
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. |
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. |
lib/CodeGen/MIRParser/MIParser.cpp | ||
---|---|---|
986 | Yes that makes sense. We should be able to use %eax and $eax that map to different registers. |
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? |
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. |
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? |
lib/CodeGen/MIRParser/MIParser.cpp | ||
---|---|---|
2534 | Yeah. I removed the check anyways. |
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; |
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 ‐‐‐‐‐‐‐
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? |
Ah yeah that was a silly mistake. Thanks
PL
Sent with ProtonMail Secure Email.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Awesome thanks. Will land asap. Thank You Francis!
PL
Sent with ProtonMail Secure Email.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Don't repeat the name in the comment.