[llvm-commits] PATCH: Tablegen - Add HWEncoding field to Register class and generate getHWEncoding() function

[Jim Grosbach]

On May 10, 2012, at 8:11 AM, Tom Stellard <thomas.stellard at amd.com> wrote:

> On Tue, May 08, 2012 at 09:25:40AM -0700, Jim Grosbach wrote:
>> 
>> On May 8, 2012, at 7:09 AM, Tom Stellard <thomas.stellard at amd.com> wrote:
>> 
>>> Hi Jim,
>>> 
>>> Thanks for the feedback.
>>> 
>>> On Mon, May 07, 2012 at 03:59:51PM -0700, Jim Grosbach wrote:
>>>> Hi Tom,
>>>> 
>>>> Good idea. ARM could use this, too.
>>>> 
>>>> The function shouldn't go into the <Target>GenRegisterInfo class, though. That's derived from TargetRegisterInfo, which is part of the Target layer. The code layers that will want to access this information, at least for the most part, are part of the MC layer and can't access that. In particular, the <Target>MCCodeEmitter bits want this and can't hook into Target classes. It could be made a lookup table and hooked in MCRegisterInfo as a generic function w/ TableGen'erated data payload, perhaps. That would require teaching the MCCodeEmitter how to get access to the MCRegisterInfo instance. There was a recent patch to do that for the InstPrinter that would provide an example of the sorts of things needed for that. TargetRegisterInfo derives from MCRegisterInfo, so anything in the Target layer that actually wants the encoding bits for a register could still get to them that way.
>>>> 
>>> 
>>> I'm not really familiar with the MC layer, so I need to spend some time
>>> learning about it before I can come up with a new patch.  Is this more
>>> or less what you are suggesting that I do:
>>> 
>>> 1. Have tablegen generate a lookup table for registers and their
>>>  encodings.
>>> 2. Add an extra parameter to InitMCRegisterInfo for the new lookup table
>>> 3. Add a function to the MCRegisterInfo class called getHWEncoding() (or
>>>  something similar) that uses the new lookup table.
>>> 4. Teach MCCodeEmitter how to access MCRegisterInfo.
>> 
>> Yep, that's it exactly. I would probably name the function something like getEncodingValue() instead, but that's purely stylistic and not a big deal.
>> 
> 
> Hi,
> 
> I've split these changes into two patches. The first patch,
> tablegen-gen-reg-hw-encoding-v2.patch causes tablegen to generate a
> lookup table that maps registers to their encodings and adds an accessor
> for the lookup table to MCRegisterInfo.
> 

Looks good. A couple of small details.

The number of physical registers for a target is limited to 64k, so we can use a 16-bit value for entries in the table and for the bitfield in the .td file. The exception is the external interface (getEncodingValue()), which should stay w/ a 32-bit input so the assert() can do its job of detecting virtual regs that somehow lived past their expiration date.

+    uint64_t Value = 0;
+    for (unsigned b = 0, be = BI->getNumBits(); b != be; ++b) {
+        if (BitInit *B = dynamic_cast<BitInit*>(BI->getBit(b)))
+        Value |= (uint64_t)B->getValue() << b;
+    }

Only need two spaces of indentation in the body of the 'for' loop here.

> The second patch, teach-MCCodeEmitter-about-MCRegisterInfo.patch
> allows MCCodeEmitters to access a target's MCRegisterInfo.  I wasn't
> sure if I should add an MCRegisterInfo member to MCCodeEmitter or if
> it was sufficent to just pass a MCRegisterInfo parameter to all the
> create*MCCodeEmitter functions.  This patch implements the latter
> solution, but I can change it if you would prefer the former.
> 

I think this is fine. It's consistent with how we deal with MCInstrInfo, anyway. We can change both in a later patch if need be.

Regards,
  Jim


> -Tom
> 
> <tablegen-gen-reg-hw-encoding-v2.patch><teach-MCCodeEmitter-about-MCRegisterInfo.patch>