This is an archive of the discontinued LLVM Phabricator instance.

Move size and alignment information of regclass to TargetRegisterInfo
ClosedPublic

Authored by kparzysz on Apr 6 2017, 12:33 PM.

Details

Summary
  1. Split RegisterClass::getSize() into two functions:
    • TargetRegisterInfo::getRegSizeInBits(const TargetRegisterClass &RC) const;
    • TargetRegisterInfo::getSpillSize(const TargetRegisterClass &RC) const;
  2. Replace RegisterClass::getAlignment() with:
    • TargetRegisterInfo::getSpillAlignment(const TargetRegisterClass &RC) const;

This will allow making those values depend on subtarget features in the future.

The plan for this is that the TargetRegisterInfo object would contain an extra member, indicating the ID of the hardware configuration associated with the subtarget. The ID would have a default value indicating a "default configuration". The target-specific <Xyz>GenRegisterInfo object, would take an optional argument indicating the ID of the hardware configuration and pass it to TargetRegisterInfo.

The TargetRegisterInfo object would then use the the ID to access the corresponding data describing register and spill sizes, spill alignments, and machine value types (coming in another patch after this). Since the ID would not be available to <Abc>RegisterClass objects, the API to access this information is being moved to TRI.

Edit: change the types from * to &.
Edit: explain the plan

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz created this revision.Apr 6 2017, 12:33 PM
kparzysz edited the summary of this revision. (Show Details)Apr 6 2017, 12:45 PM
kparzysz edited the summary of this revision. (Show Details)Apr 6 2017, 1:23 PM
bjope added a subscriber: bjope.Apr 6 2017, 11:34 PM
asb accepted this revision.Apr 7 2017, 1:21 AM

Reference to previous related discussion: D24631 and http://lists.llvm.org/pipermail/llvm-dev/2016-September/105027.html

As you expressed in the RFC mailing list thread, splitting the concept of register size and spill slot size makes sense as they are not always the same. Given the plan to move towards having this information depend on the target machine rather than being an inherent property of a register class, TargetRegisterInfo is the natural home. I've commented on a few tiny nits, but this all looks good to me.

include/llvm/Target/TargetRegisterInfo.h
328 ↗(On Diff #94416)

"the a" -> "the"

lib/Target/AArch64/AArch64InstrInfo.cpp
2728 ↗(On Diff #94416)

I'd personally be tempted to just put the TRI.getRegSize directly in the assert and avoid the need to worry about unused locals.

lib/Target/Mips/MipsSEFrameLowering.cpp
892 ↗(On Diff #94416)

Nit: second and third arguments seem to be indented an extra space, same in the call about 15 lines below.

This revision is now accepted and ready to land.Apr 7 2017, 1:21 AM
bjope added a comment.Apr 7 2017, 9:07 AM

While doing these changes in the API for fetching the sizes (getRegSize and getSpillSize) it would be interesting to at the same time considering to at least change getRegSize to return size in bits, but maybe also the getSpillSize.
The spillSize must be a multiple of the byte size, but there are targets where the register size in bits isn't a multiple of the byte size (e.g. 1-bit registers). So don't we need to be able to express the register size in bits?

The interface could ofcourse also provide methods for fetching the sizes in either bits or bytes:

getRegSizeInBits()
getRegSize()   // In bytes, rounded up? Or down? Maybe call this getRegSizeInBytes()?
getSpillSizeInBits()
getSpillSize()  // In bytes

This would also get rid of some "getRegSize() *8" etc in the code, perhaps making the code easier to read (you do not need to know

As a side note our out-of-tree target has 16-bit bytes (which I think could be common for several other DSP targets etc).
And we have 24-bit and 40 bit registers (that aren't a multiple of the byte size).
And we have patches all over the place to replace (x * 8) and (x / 8) by something where we can toggle byte size depending on target.
I do not expect changes making the code more complicated just to support out-of-tree-targets, but if it isn't a big cost to make the code a little bit more general (and for example hiding those magic "8" constants in a common accessor methods), then I think it could be of great help for other targets also.

kparzysz marked 3 inline comments as done.Apr 11 2017, 7:54 AM
kparzysz updated this revision to Diff 94817.Apr 11 2017, 7:57 AM
kparzysz edited the summary of this revision. (Show Details)

Changed the API to return register size in bits instead of bytes.

MatzeB accepted this revision.Apr 24 2017, 10:59 AM

It feels slightly odd that the size of a register is a property of the register class (amusingly even the comment talks about a register when the function actually takes a class). On the other hand generating a list of register sizes would be extremely wasteful on targets with thousands of registers, so this is the best solution I can of today.

LGTM!

include/llvm/Target/TargetRegisterInfo.h
323 ↗(On Diff #94817)

This takes a register class not a register.

328–329 ↗(On Diff #94817)

This is about a register class not a register.

kparzysz marked 2 inline comments as done.Apr 24 2017, 11:26 AM
This revision was automatically updated to reflect the committed changes.