[Mesa-dev] [PATCH 07/17] i965/fs: Move offset() and half() to the fs_builder
Jason Ekstrand
jason at jlekstrand.net
Wed Jun 24 08:31:11 PDT 2015
On Wed, Jun 24, 2015 at 7:56 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> 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").
That's exactly the problem, yes.
>> 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 ;).
Fair point.
> 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.
Like I said, I'm not married to it. I'll go ahead and make it an
extra argument.
--Jason
>> 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.
>>>>> >> >>
>>>>> >> >>>[...]
More information about the mesa-dev
mailing list