[Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.

[Matt Turner]

On Thu, Feb 11, 2016 at 11:21 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>
>> On Thu, Feb 11, 2016 at 7:31 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Matt Turner <mattst88 at gmail.com> writes:
>>>
>>>> On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>> Would be really nice if we could also get rid of reg_offset as we're at
>>>>> it.  reg and subreg_offset basically represent the same thing but with
>>>>> different units, couldn't we just have a single offset field in bytes?
>>>>> Should it be part of brw_reg or backend_reg?  I think I would lean
>>>>> towards backend_reg.  In that case does it make sense to move this into
>>>>> brw_reg now only to move it back to backend_reg later on?
>>>>
>>>> That would be nice.
>>>>
>>>> I'm just not sure how to do it. brw_reg has to have the subnr field,
>>>> and it's nice if that's the field the higher levels use too.
>>>>
>>> I guess at this point brw_reg is just an implementation detail of
>>> backend_reg, if some of it doesn't make sense at the IR level
>>> (e.g. because the IR wants more than 5 bits of sub-(V)GRF offset)
>>> there's no need to keep the IR tied up to the lower-level brw_reg
>>> representation.
>>
>> Do you have an example of where we might want a subreg_offset >= 32?
>
> reg_offset is basically a subreg_offset in 32B units, any use of
> reg_offset is a good example I guess. ;)

No, that's circular reasoning :)

>>
>> I think using brw_reg is nice... it pretty simply contains the bits
>> that are common to the IR and the hardware. I'm not finding this limiting.
>
> I don't think it's limiting, but it's silly and error-prone to have two
> different fields with the exact same semantics but different units.  It
> means anytime you need to find out what a register reads or writes you
> need to add two terms, and anytime you need to specify a register region
> you need to split up the offset in two terms (or use some of the helpers
> we have for that purpose, e.g. byte_offset(), *or* assume that your
> offset is a multiple of 32b as some places do which will blow up when we
> start doing sub-dword types more extensively).

FWIW, I haven't found the behavior of byte_offset -- that it
increments reg_offset if we go past the end of the register -- to be
useful. Do we actually rely on that somewhere? That's more in line
with my previous question.

I understand the argument that "they have the same semantics", but I
don't know that we gain anything by combining them, and also one of
the two is a hardware concept, and the other is an IR concept. Having
them separate seems fine to me...