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

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> 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 :)
>
How so?  Because they have exactly the same semantics but subreg_offset
has lower granularity, every use-case for reg_offset is a legitimate
use-case of subreg_offset.

>>>
>>> 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.
>
Without byte_offset() you have no sane way to do things like "give me
the n-th scalar element from this register region" (several places
indeed do it by poking the offset directly instead of going through the
helper and they're all rather dubious if not outright broken, see
e.g. fs_reg::set_smear() (which blows up for widths greater than 8 or
types larger than a dword) or lower_integer_multiplication() which
overwrites the subreg_offset of the destination without considering what
the previous offset of the region was).

> I understand the argument that "they have the same semantics", but I
> don't know that we gain anything by combining them,

Again, by combining them we spare ourselves from considering two terms
rather than one for the *same* thing anytime we need to find out or
specify what region of the (virtual) register file a backend_reg refers
to.  If you need some examples of why this is annoying and repetitive
search for the many 'x.reg_offset * 32 + x.subreg_offset' in the
backend:

   brw_fs_copy_propagation.cpp:355
   brw_fs_copy_propagation.cpp:469
   brw_fs_copy_propagation.cpp:471-472
   brw_fs_copy_propagation.cpp:526
   brw_fs.cpp:1650

[The list is by no means comprehensive, I just got bored of reading
through the grep results]

There are also some examples of places where we take into account
reg_offset but don't take into account subreg_offset which illustrate
what I meant by "error-prone".  Was the omission intentional?  How is
the assumption justified that a region is 32B-aligned?  Is it a bug?

   brw_fs_copy_propagation.cpp:354
   brw_fs_copy_propagation.cpp:525
   brw_fs.cpp:363
   brw_fs.cpp:377
   brw_fs.cpp:1496
   ...

> and also one of the two is a hardware concept, and the other is an IR
> concept. Having them separate seems fine to me...

The split between reg_ and subreg_offset might be a hardware concept,
but it's of little use at the IR level and only a source of bugs in a
world of variable SIMD-width and execution types.  And our split between
the two doesn't even match the hardware's.  For the hardware a register
is always 32B, while the reg_offset unit is in fact dependent on the
register file in a way I won't claim to remember exactly (what leads to
a number of dubious assert() calls that wouldn't be there if offsets
were represented consistently across register files, see
e.g. byte_offset()).  Thankfully it's at least no longer dependent on
the dispatch_width.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160212/8ab15fdd/attachment.sig>