[llvm-dev] MCRegisterClass::getSize() - spill size vs reg size

[Björn Pettersson A via llvm-dev]

In MCRegisterClass (and TargetRegisterClass) there is a method getSize.
According to the comments it should return both the size of "the register" (I assume that means "any register in the class")
which is assumed to be the same as the size needed on the stack when spilling such a register.

The definition looks like this:

  /// getSize - Return the size of the register in bytes, which is also the size
  /// of a stack slot allocated to hold a spilled copy of this register.
  unsigned getSize() const { return RegSize; }


To me it seems like the primary use of getSize has been to get the SpillSize, but some users
(GlobalISel?) also use this to get the register size. Note that the variable actually is called
SpillSize in tablegen (CodeGenRegisters/RegisterInfoEmitter). But for some reason the
MCRegisterClass renames it to RegSize and claims that it also denotes the register size.


Some problems I see:


a)       Is always register size and spill size the same?
(It is not for my downstream, out-of-tree, target.)


b)      Is always the register size in bits a multiple of the byte size?
No, lots of targets implement 1-bit registers. So what is the size in bytes for such a register?


c)       When defining register classes in <Target>RegisterInfo.td the spill size can be given explicitly
(by using "Size = <size in bits>"). If not given (or rather if Size=0), then the SpillSize is derived

from the first value type that is mapped to the register class.
In tablegen this size is denoted SpillSize and it is given in bits. But tablegen
will convert this to bytes by simply dividing by 8 (remainder is truncated).

So, for example, one bit registers (exists for several targets) will get a spill size of zero bytes,
unless Size is explicitly set.

This is not easy to understand. Neither when looking at the RegisterClass definitions, nor as
a user of MCRegisterClass::getSize (which according to the comment should return both size
of register and spill size). Well, a 1-bit register is not a full byte, so maybe it is obvious that

the size in bytes is zero?


Some examples:

PPC has a register class CRBITRC that has one bit registers, but it sets Size=32.
So for this register class MCRegisterClass::getSize() will return 4 bytes. So that is the spill size,
but if a user of getSize() expects to get the register size it is definitely not the same as the spill size!

Hexagon has the UsrBits register class with one bit registers. It does not set Size, but it sets
isAllocatable=0 so it will not be used for virtual register (and thus never spilled). So a derived spill
size of 0 is probably expected. But still, is it expected that the size of the register is 0 bytes, or
do we want to round up the size to 1 byte?


One idea to get this more general, and less confusing, could be as follows:

1)      Let tablegen save both SpillSize and RegSize for each register class.
We should preferably keep the size in bits here. At least for the RegSize. This makes it possible for a user
of the information to determine by itself it size should be rounded up/down if a conversion to bytes is needed.

2)      Add new methods in MCRegisterClass (and TargetRegisterClass) to getSpillSize() and getRegSizeInBits().

3)      Tablegen could assert that spill size is a multiple of the byte size.

4)      Mark the old getSize() as deprecated (and let it return the spill size until it can be removed).

5)      Start replacing uses of getSize() by the new methods, depending on if the register size or the spill size is of interest.

Another improvement would be to rename Size to SpillSize when defining register classes in <Target>RegisterInfo.td files.
If SpillSize isn't explicitly given it should be derived from the value type. If it is explicitly given it should be allowed to set it
to zero wihout tablegen replacing it by deriving size from value type.

Any comments on this are welcome!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161017/e672aed6/attachment.html>