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

[Jakob Stoklund Olesen]

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.

> +  /// \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