Matt Turner <matts...@gmail.com> writes: > On Thu, Feb 11, 2016 at 11:21 PM, Francisco Jerez <curroje...@riseup.net> > wrote: >> Matt Turner <matts...@gmail.com> writes: >> >>> On Thu, Feb 11, 2016 at 7:31 PM, Francisco Jerez <curroje...@riseup.net> >>> wrote: >>>> Matt Turner <matts...@gmail.com> writes: >>>> >>>>> On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez <curroje...@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.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
