On Wed, Jun 24, 2015 at 7:56 AM, Francisco Jerez <curroje...@riseup.net> wrote: > Jason Ekstrand <ja...@jlekstrand.net> writes: > >> On Wed, Jun 24, 2015 at 6:44 AM, Francisco Jerez <curroje...@riseup.net> >> wrote: >>> Jason Ekstrand <ja...@jlekstrand.net> writes: >>> >>>> On Jun 24, 2015 6:29 AM, "Francisco Jerez" <curroje...@riseup.net> wrote: >>>>> >>>>> Jason Ekstrand <ja...@jlekstrand.net> writes: >>>>> >>>>> > On Jun 24, 2015 4:29 AM, "Francisco Jerez" <curroje...@riseup.net> >>>> wrote: >>>>> >> >>>>> >> Jason Ekstrand <ja...@jlekstrand.net> writes: >>>>> >> >>>>> >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez < >>>> curroje...@riseup.net> >>>>> > wrote: >>>>> >> >> Jason Ekstrand <ja...@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. >>>>> >> >> >>>>> >> >>>[...] _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
