This is an archive of the discontinued LLVM Phabricator instance.

[MC] Remove PhysRegSize from MCRegisterClass
ClosedPublic

Authored by bjope on May 22 2018, 8:45 AM.

Details

Summary

The interface to get size and spill size of a register
was moved from MCRegisterInfo to TargetRegisterInfo over
a year ago. Afaik the old interface has bee around
to give out-of-tree targets a chance to adapt to the
new interface.

One problem with the old MCRegisterClass::PhysRegSize was that
it represented the size of a register as "size in bits" / 8.
So a register had to be a multiple of eight bits wide for the
size to be correct (and the byte size for the target needed to
be eight bits).

Diff Detail

Event Timeline

bjope created this revision.May 22 2018, 8:45 AM
bjope added a comment.May 22 2018, 8:55 AM

Is this cleanup OK now?
I know that @kparzysz tried this originally in https://reviews.llvm.org/D31299, but that refactoring ended up leaving these methods and the PhysRegSize member around.

It looks like @qcolombet had some opinions about this last time. The original solution for D31299 was reverted in r298727 with the message being "Revert r298652 on Quentin's request.
I'm not sure what the problem was. Is this code actually used from somewhere, or was it just to give out-of-tree clones some time to adapt to the new interface?

I'm ok with it, but please wait for Quentin's input.

kparzysz accepted this revision.Aug 8 2018, 9:11 AM

Accepting as per comments in D50285.

This revision is now accepted and ready to land.Aug 8 2018, 9:11 AM
This revision was automatically updated to reflect the committed changes.
bjope added a comment.Aug 9 2018, 8:28 AM

Accepting as per comments in D50285.

Thanks! I've landed this now.
If it is reverted we can try with D50285 instead. But I really hope this should be OK. We have had it like this in our OOT repo for a long time. We needed to detect if someone starts using getSize()/getPhysRegSize(), as those methods simply doesn't make sense for some of our non-byte sized registers.