[Mesa-dev] [PATCH 07/17] i965/fs: Move offset() and half() to the fs_builder

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Wed, Jun 24, 2015 at 6:44 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>>> On Jun 24, 2015 6:29 AM, "Francisco Jerez" <currojerez at riseup.net> wrote:
>>>>
>>>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>>>
>>>> > On Jun 24, 2015 4:29 AM, "Francisco Jerez" <currojerez at riseup.net>
>>> wrote:
>>>> >>
>>>> >> Jason Ekstrand <jason at jlekstrand.net> writes:
>>>> >>
>>>> >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez <
>>> currojerez at riseup.net>
>>>> > wrote:
>>>> >> >> Jason Ekstrand <jason at jlekstrand.net> writes:
>>>> >> >>
>>>> >> >>> We want to move these into the builder so that they know the
>>> current
>>>> >> >>> builder's dispatch width.  This will be needed by a later commit.
>>>> >> >>
>>>> >> >> I very much like the idea of this series, but, why do you need to
>>> move
>>>> >> >> these register manipulators into the builder?  The builder is an
>>> object
>>>> >> >> you can use to:
>>>> >> >>  - Manipulate and query parameters affecting code generation.
>>>> >> >>  - Create instructions into the program (::emit and friends).
>>>> >> >>  - Allocate virtual registers from the program (::vgrf and friends).
>>>> >> >>
>>>> >> >> offset() and half() logically perform an action on a given register
>>>> >> >> object (or rather, compute a function of a given register object),
>>> not
>>>> >> >> on a builder object, the builder is only required as an auxiliary
>>>> >> >> parameter -- Any reason you didn't just pass it as a third
>>> parameter?
>>>> >> >
>>>> >> > What's required as a third parameter is the current execution size.
>>> I
>>>> >> > could have passed that directly, but I figured that, especially for
>>>> >> > half(), it would get messed up.  I could pass the builder in but I
>>>> >> > don't see a whole lot of difference between that and what I'm doing
>>>> >> > right now.
>>>> >>
>>>> >> Assembly-wise there's no difference, but it seems inconsistent with
>>> both
>>>> >> the remaining register manipulators and remaining builder methods, and
>>>> >> IMHO it's kind of an anti-pattern to make something a method that
>>>> >> doesn't need access to any internal details of the object.
>>>> >>
>>>> >> > As is, it's not entirely obvious whether you should call
>>>> >> > half(reg) on the half-width or full-width builder.  I'm not 100% sure
>>>> >> > what to do about that.
>>>> >> >
>>>> >> Actually, does half() really need to know about the builder?  AFAICT it
>>>> >> only needs it because of dispatch_width(), and before doing anything
>>>> >> useful with it it asserts that it's equal to 16, what points at the
>>>> >> parameter being redundant.  By convention a "half" is a group of 8
>>>> >> channels (we may want to revise this convention when we implement
>>> SIMD32
>>>> >> -- E.g. make half a group of 16 channels and quarter a group of 8
>>>> >> channels), so 'half(reg)' could simply be implemented as
>>>> >> "horiz_offset(reg, 8 * i)" without any dependency on the builder.  As
>>>> >> additional paranoia to catch half() being called on a non-16-aligned
>>>> >> register you could assert that either 'stride == 0' or 16 divides
>>>> >> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why
>>>> >> don't we have a reg_offset already in bytes again?) -- That would also
>>>> >> catch cases in which the register and builder "widths" get out of sync,
>>>> >> e.g. if half is called in an already halved register but the builder
>>>> >> used happens to be of the correct exec_size.
>>>> >
>>>> > OK, fine, we can pull half() back out.  Should offset() stay in the
>>>> > builder? If not, where should it get its dispatch width.
>>>> >
>>>> I'm for leaving it as a stand-alone function (like all other register
>>>> manipulators), and add a third argument to pass the 'fs_builder' it can
>>>> take the dispatch width from?
>>>
>>> I'm not a big fan.  However, in the interest of keeping the builder clean,
>>
>> It also keeps the register interface consistent IMHO.  Why do you say
>> you're not a big fan?
>
> Unfortunately, there's really no good place to put it.  Ideally, we'd
> have more/better information in the allocator or somewhere and we
> could do offset() type operations with just the register.
> Unfortunately, that's not an option in the current setup and I don't
> think keeping reg.width around just for offset() is worth it.  That
> means it has to pull it from somewhere.  It can't pull it from the
> visitor because it needs to be the width we're currently using for
> building.  That means it needs to pull it from the builder.
>
I think that part of the problem here is that width was really used for
two subtly different purposes:
 - Determining the distance between individual components of a vector.
 - Determining the execution size of new instructions based on a rather
   complex function of the above.

The builder makes the second function unnecessary, but not the first, so
it's not surprising to see some awkwardness while trying to remove it
regardless (need for an additional parameter, either as an explicit
function argument or as "this").

> Yes, it could take an extra parameter, but it seems kind of awkward to
> have something that takes a builder that isn't "building" anything.

That's precisely the reason why I found it awkward to add a method to
the builder that didn't build anything ;).  Isn't passing something as
argument the usual way to represent that the computation is a function
of the argument among other things?  It doesn't necessarily imply that
the computation has any side effects on the argument, as implicit "this"
arguments often do.

> On the other hand, it seemed to me at the time like offset() went
> together rather well with fs_builder::vgrf().  You get one with vgrf()
> and you get different components with offset() on the same builder.
>
> I'm not that attached to keeping it in the builder.  It just seems
> like it's not the best of the bad options available.
> --Jason
>
>>> I'm willing to go with that.
>>> --Jason
>>>
>>>> >> >> As offset() and half() don't require access to any private details
>>> of
>>>> >> >> the builder, that would actually improve encapsulation, and would
>>> avoid
>>>> >> >> the dubious overloading of fs_builder::half() with two methods with
>>>> >> >> completely different semantics.
>>>> >> >
>>>> >> > Yeah, I don't really like that either.  I just couldn't come up with
>>>> >> > anything better at the time.
>>>> >> >
>>>> >> > Suggestions are very much welcome.  But I would like to settle on
>>>> >> > whatever we do fairly quickly so as to limit the amount of
>>>> >> > refactoring.
>>>> >> > --Jason
>>>> >> >
>>>> >> >> Thanks.
>>>> >> >>
>>>> >> >>>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150624/11a4df80/attachment-0001.sig>