Page MenuHomePhabricator

[WIP] CodeGen: Prototype class for registers
ClosedPublic

Authored by arsenm on Jun 18 2019, 8:52 AM.

Details

Summary

Avoids using a plain unsigned for registers throughoug codegen.
Doesn't attempt to change every register use, just something a little
more than the set needed to build after changing the return type of
MachineOperand::getReg().

Diff Detail

Event Timeline

arsenm created this revision.Jun 18 2019, 8:52 AM

I did a quick glance and it seems reasonable.

Thanks for doing this Matt, this is a long due clean-up.
I haven't looked carefully at all the changes, but the general direction LGTM.

One question: right now, we already have MCPhysReg for physical registers (unfortunately it is not used consistently, but that's a different story), should we replace those as well with Register or be explicitly about what is expected in the related passes?

I am torn because MCPhysReg gives more information on what is expected, but on the other hand it will be pretty easy to slip Register here and there over time and we would end up with an inconsistent code base again.
What do you think?

include/llvm/CodeGen/Register.h
28

I am not sure I like those, since it still allows using Reg as plain unsigned.

57

Ditto.

arsenm marked 2 inline comments as done.Jun 18 2019, 12:03 PM

Thanks for doing this Matt, this is a long due clean-up.
I haven't looked carefully at all the changes, but the general direction LGTM.

One question: right now, we already have MCPhysReg for physical registers (unfortunately it is not used consistently, but that's a different story), should we replace those as well with Register or be explicitly about what is expected in the related passes?

I am torn because MCPhysReg gives more information on what is expected, but on the other hand it will be pretty easy to slip Register here and there over time and we would end up with an inconsistent code base again.
What do you think?

I'm not sure. I didn't encounter that many MCPhysRegs. Maybe adding an MCRegister that's nearly identical might be a future improvement? That way it could enforce MCRegister->Register is OK, but Register->MCRegister is not

include/llvm/CodeGen/Register.h
28

I missed the parameter types here. Do you think trying to preserve the isStackSlot is important?

57

I think this is necessary to make this a manageable sized patch. I was hoping to minimize the amount of code touched at once, so this can possibly be removed later

arsenm marked an inline comment as done.Jun 18 2019, 12:16 PM
arsenm added inline comments.
include/llvm/CodeGen/Register.h
28

I guess this can be made private

arsenm updated this revision to Diff 205431.Jun 18 2019, 1:55 PM

Make register test functions private, remove commented out stuff

qcolombet accepted this revision.Jun 19 2019, 10:49 AM

Maybe adding an MCRegister that's nearly identical might be a future improvement? That way it could enforce MCRegister->Register is OK, but Register->MCRegister is not

Sounds like a plan.

This revision is now accepted and ready to land.Jun 19 2019, 10:49 AM
bogner added inline comments.Jun 19 2019, 4:35 PM
include/llvm/CodeGen/Register.h
23–25

Any particular reason to have static methods that take an unsigned instead of just implementing these directly using this's Reg in isVirtual and friends?

bogner accepted this revision.Jun 19 2019, 4:36 PM
arsenm marked an inline comment as done.Jun 19 2019, 4:41 PM
arsenm added inline comments.
include/llvm/CodeGen/Register.h
23–25

Not really, but this was easier to copy-paste from the existing functions

Agreed. I like the direction this is going

include/llvm/CodeGen/Register.h
57

I agree that operator unsigned() will need to be removed to complete the migration but I also see the point about limiting the size of the patch. I'm ok with it being removed in a follow-up step. We can get an idea of how big a job removing it is by marking it deprecated and seeing how many warnings that causes.

arsenm closed this revision.Jun 24 2019, 8:51 AM

r364191