This is an archive of the discontinued LLVM Phabricator instance.

Named VReg support for MIR
ClosedPublic

Authored by plotfi on Feb 28 2018, 4:08 PM.

Details

Summary

This patch adds support for named vreg operands in MIR.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.Feb 28 2018, 4:08 PM
plotfi added inline comments.Mar 1 2018, 8:59 AM
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
432

StringRef might be better here?

thegameg added inline comments.Mar 1 2018, 12:57 PM
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?

plotfi updated this revision to Diff 136612.Mar 1 2018, 2:52 PM
plotfi added a comment.Mar 1 2018, 2:57 PM

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.

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?

plotfi added inline comments.Mar 1 2018, 3:00 PM
include/llvm/CodeGen/MachineRegisterInfo.h
432

Yeah. I'll update the patch.

include/llvm/CodeGen/TargetRegisterInfo.h
1155

Oh! that sounds pretty good. Hmm. The question is, does all the calling code where we call printReg have an MRI. From my initial survey that doesn't appear to be the case.

plotfi updated this revision to Diff 136618.Mar 1 2018, 3:08 PM

I think this could be added in a future patch.

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.

bogner added inline comments.Mar 2 2018, 12:02 PM
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
1203–1208

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.

plotfi updated this revision to Diff 136831.Mar 2 2018, 12:45 PM
plotfi added inline comments.Mar 2 2018, 1:00 PM
.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.

432

Tried stringref, had some bugs in debug mode. Changed it back.

lib/CodeGen/MIRParser/MIParser.cpp
1203–1208

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.

plotfi updated this revision to Diff 136840.Mar 2 2018, 1:36 PM

As a drive-by feature request, could you add a MachineRegisterInfo::setName method?

plotfi added a comment.Mar 2 2018, 3:23 PM

As a drive-by feature request, could you add a MachineRegisterInfo::setName method?

Sure!

thegameg added inline comments.Mar 5 2018, 3:42 AM
include/llvm/CodeGen/MachineRegisterInfo.h
432

What was the issue?

711

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
1203–1208

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.

MatzeB added a comment.Mar 5 2018, 5:03 PM

Thanks for working on this; I'm looking forward to have this!

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).

432

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.

711

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.

1203

local variables should start with uppercase (and most people don't bother putting a const on scalars, but why not).

1203–1208

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.

Also, please update MIRLangRef for this patch as well, thanks!

plotfi updated this revision to Diff 138075.Mar 12 2018, 12:44 PM
plotfi marked 15 inline comments as done.Mar 12 2018, 3:48 PM
plotfi added inline comments.
include/llvm/CodeGen/MachineRegisterInfo.h
86–87

That sounds good. Should I do this in a subsequent patch? Should I leave out the name check here?

432

I see what you mean. Return a StringRef, take a std::string on creation.

plotfi updated this revision to Diff 138100.Mar 12 2018, 3:50 PM
plotfi marked 2 inline comments as done.
plotfi updated this revision to Diff 138263.Mar 13 2018, 2:30 PM
plotfi marked 7 inline comments as done.
plotfi marked 4 inline comments as done.Mar 13 2018, 3:28 PM
plotfi updated this revision to Diff 138296.Mar 13 2018, 9:02 PM
plotfi marked 4 inline comments as done.

I have made some more changes to address all of the feedback. Please let me know what else I can do to improve this.

plotfi updated this revision to Diff 138297.Mar 13 2018, 9:13 PM
plotfi marked an inline comment as done.Mar 14 2018, 10:52 AM
thegameg added inline comments.Mar 14 2018, 1:10 PM
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.

plotfi added inline comments.Mar 14 2018, 1:26 PM
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?

thegameg added inline comments.Mar 14 2018, 1:51 PM
lib/CodeGen/MIRParser/MIParser.cpp
986

What I mean by that is:

  • the only way to parse a named virtual register is through parseRegisterOperand
  • parseRegisterOperand handles all kinds of registers, sub registers, register classes / banks.
  • (1) the only thing parseVirtualRegister does is create an empty VRegInfo and create an "incomplete" virtual register.
  • (2) the call to parseRegisterClassOrBank in parseRegisterOperand will continue the parsing.

This is the part where I am not completely sure:

  • can you do the same as (1), with additional steps as inserting the name into Names2VRegs and MRI::VReg2Name
  • will re-using (2) work with no additional changes here?
bogner added inline comments.Mar 14 2018, 2:26 PM
include/llvm/CodeGen/MachineRegisterInfo.h
433

Very minor, but calling c_str() here causes an extra call to strlen - better to construct the StringRef from the std::string directly.

plotfi marked 2 inline comments as done.Mar 15 2018, 7:45 AM
plotfi added inline comments.
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.

thegameg added inline comments.Mar 15 2018, 7:59 AM
lib/CodeGen/MIRParser/MIParser.cpp
986

Yes that makes sense. We should be able to use %eax and $eax that map to different registers.

plotfi updated this revision to Diff 138623.Mar 15 2018, 2:12 PM
plotfi marked an inline comment as done.
thegameg added inline comments.Mar 15 2018, 4:42 PM
include/llvm/CodeGen/MachineRegisterInfo.h
433

I think what @bogner meant here is to make use of StringRef’s constructor that takes an std::string (and uses the .size() from that object) instead of the constructor taking a const char * (which has to call strlen to get the actual size).

plotfi updated this revision to Diff 138808.Mar 16 2018, 8:57 PM
plotfi marked 4 inline comments as done.
plotfi updated this revision to Diff 139000.Mar 19 2018, 1:53 PM
thegameg added inline comments.Mar 19 2018, 2:13 PM
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?

plotfi marked 4 inline comments as done.Mar 19 2018, 2:50 PM
plotfi added inline comments.
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.

thegameg added inline comments.Mar 19 2018, 3:04 PM
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?

plotfi updated this revision to Diff 139012.Mar 19 2018, 3:12 PM
plotfi marked 4 inline comments as done.
plotfi marked 5 inline comments as done.Mar 28 2018, 12:22 AM
plotfi added inline comments.
lib/CodeGen/MIRParser/MIParser.cpp
2534

Yeah. I removed the check anyways.

plotfi updated this revision to Diff 140048.Mar 28 2018, 12:23 AM
plotfi marked an inline comment as done.
thegameg added inline comments.Mar 29 2018, 4:27 AM
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 ‐‐‐‐‐‐‐

plotfi updated this revision to Diff 140380.Mar 29 2018, 7:00 PM
plotfi marked an inline comment as done.
plotfi marked an inline comment as done.
thegameg added inline comments.Mar 29 2018, 11:48 PM
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 ‐‐‐‐‐‐‐

plotfi updated this revision to Diff 140441.Mar 30 2018, 9:02 AM
plotfi marked an inline comment as done.
thegameg accepted this revision.Mar 30 2018, 9:07 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 30 2018, 9:07 AM

Awesome thanks. Will land asap. Thank You Francis!

PL

​Sent with ProtonMail Secure Email.​

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

bogner accepted this revision.Mar 30 2018, 11:48 AM
plotfi closed this revision.Apr 9 2018, 9:48 AM