[PATCH] Add a way to define the bit range covered by a SubRegIndex.

[Ahmed Bougacha]

On Tue, May 28, 2013 at 1:48 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>
> On May 26, 2013, at 3:45 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
>
>> This patch adds offset/size information to SubRegIndex, makes it
>> available through MCRegisterInfo, and provides it for ARM and X86.
>>
>> I'm not sure how solid the offset calculation is; at first I tried to
>> make it smart to handle all synthesized indices, but for some (ARM
>> register tuples), no offsets (and sometimes sizes) can be easily
>> computed, and most of these subregs aren't even contiguous anyway, so
>> it's not really a problem. In that case, the MCRegisterInfo method
>> returns false.
>
> This generally looks good to me. Some comments below.
>
>> -class SubRegIndex<list<SubRegIndex> comps = []> {
>> +class SubRegIndex<list<SubRegIndex> comps = [], int offset = -1, int size = -1> {
>
>
> I think we should flip these arguments around to be (size, offset, comps). That is more convenient for most targets. You can also make the offset default = 0, that is usually the right thing.
>
> In a second patch, I think it would make sense to make the size argument mandatory.

OK then - quick update on the patch - what about this: Instead of
having the comps list, let ComposedOf only be settable by
ComposedSubRegIndex, a subclass of SubRegIndex. That way, SubRegIndex
only has size/offset, whereas ComposedSubRegIndex only has the A and B
elements of comps. (By the way, this lets us do some cleanup in
TableGen).

-- Ahmed Bougacha

>> +  /// \brief Get the bit range covered by a given sub-register index.
>> +  /// In some cases, for instance non-contiguous synthesized indices,
>> +  /// there is no meaningful bit range to get, so return whether \p Offset
>> +  /// and \p Size were set.
>> +  bool getSubRegIdxCoveredBits(unsigned Idx, uint16_t &Offset,
>> +                               uint16_t &Size) const;
>
> Please document when the function returns true and false. Some early LLVM APIs return false for success (!)
>
> The Size and Offset references should be unsigned, the API clients don’t care about the internal representation of the tables.
>
> Go ahead and commit with these changes.
>
> Thanks,
> /jakob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: subreg-covered-range-2.diff
Type: application/octet-stream
Size: 14046 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130530/6aa8e6ae/attachment.obj>