[MachinePipeliner] Use VirtRegOrUnit instead of Register appropriately#177535
[MachinePipeliner] Use VirtRegOrUnit instead of Register appropriately#177535
Conversation
s-barannikov
left a comment
s-barannikov
left a comment
There was a problem hiding this comment.
I left a couple of comments, but I don't really know the algorithm and thus not confident of the changes.
In general, any functional change needs a regression test.
| for (MCRegUnit Unit : TRI->regunits(Reg.asMCReg())) | ||
| UpdateTargetRegs(VirtRegOrUnit(Unit)); |
There was a problem hiding this comment.
This branch is unreachable: PHI register can never be a physical register.
There was a problem hiding this comment.
Removed the branch. I'm not familiar with these infrastructures so this may be a silly question, but is PHI register always a virtual register?
There was a problem hiding this comment.
but is PHI register always a virtual register
Yes
| // Return true if Reg is reserved one, for example, stack pointer | ||
| bool isReservedRegister(Register Reg) const { | ||
| return Reg.isPhysical() && MRI.isReserved(Reg.asMCReg()); | ||
| bool isReservedRegister(VirtRegOrUnit Reg) const { |
There was a problem hiding this comment.
The comment and the name don't match the implementation
kasuga-fj
left a comment
kasuga-fj
left a comment
There was a problem hiding this comment.
Thanks for the comments.
In general, any functional change needs a regression test.
Yeah, but for various reasons, it's difficult to write meaningful tests for this part (not just here, but throughout the entire MachinePipeliner as well). This is definitely technical debt, but there isn't much I can do at this point.
| for (MCRegUnit Unit : TRI->regunits(Reg.asMCReg())) | ||
| UpdateTargetRegs(VirtRegOrUnit(Unit)); |
There was a problem hiding this comment.
Removed the branch. I'm not familiar with these infrastructures so this may be a silly question, but is PHI register always a virtual register?
| // Return true if Reg is reserved one, for example, stack pointer | ||
| bool isReservedRegister(Register Reg) const { | ||
| return Reg.isPhysical() && MRI.isReserved(Reg.asMCReg()); | ||
| bool isReservedRegister(VirtRegOrUnit Reg) const { |
3 participants
Resolve the FIXMEs in MachinePipeliner added in #167730, primarily by replacing
RegisterwithVirtRegOrUnitwhere necessary to remove invalidstatic_casts. Based on my local testing, there was no noticeable performance impact.