Jason Ekstrand <ja...@jlekstrand.net> writes:
> On Fri, Feb 20, 2015 at 4:11 AM, Francisco Jerez <curroje...@riseup.net>
> wrote:
>
>> Jason Ekstrand <ja...@jlekstrand.net> writes:
>>
>> > I'm still a little pensive. But
>> >
>> > Reviewed-by: Jason Ekstrand <jason.ekstr...@intel.com>
>> >
>> Thanks.
>>
>> > Now for a little aside. I have come to the conclusion that I made a
>> grave
>> > mistake when I did the LOAD_PAYLOAD stuff. In retrospect, I should have
>> > just subclassed fs_inst for load_payload. The problem is that we need to
>> > snag a bunch of information for the sources when we create the
>> > load_payload. In particular, we need to know the width of the source so
>> > that we know how much space it consumes in the payload and we need to
>> know
>> > the information required to properly re-create the mov such as
>> > force_sechalf and force_writemask_all. Really, in order to do things
>> > properly, we need to gather this information *before* we do any
>> > optimizations. The nasty pile of code that you're editing together with
>> > the "effective_width" parameter is a lame attempt to capture/reconstruct
>> > this information. Really, we should just subclass, capture the
>> information
>> > up-front, and do it properly.
>> >
>> Yes, absolutely, this lowering pass is a real mess. There are four more
>> unreviewed patches in the mailing list fixing bugs of the metadata
>> guessing of lower_load_payload() [1][2][3][4], you may want to take a
>> look at them. There are more bugs I'm aware of but it didn't seem
>> practical to fix them.
>>
>
> Yeah, Matt pointed me at those. I'll give them a look later today.
>
>
>> That said, I don't think it would be worth subclassing fs_inst.
>>
>> My suggestion would have been to keep it simple and lower LOAD_PAYLOAD
>> into a series of MOVs with force_writemask_all enabled in all cases,
>> simply rely on the optimizer to eliminate those where possible. Then
>> get rid of the metadata and effective_width guessing. Instead agree on
>> immediates and uniforms being exec_size-wide by convention
>> (i.e. LOAD_PAYLOAD's exec_size rather than the original instruction's),
>> then prevent constant propagation from propagating an immediate load
>> into a LOAD_PAYLOAD if it would lead to a change in the semantics of the
>> program, and maybe just run constant propagation after lowering so we
>> can be sure those cases are cleaned up properly where register coalesce
>> isn't enough.
>>
>
> First off, simply setting force_writemask_all isn't an option. Consider
> the following scenario:
>
> a = foo;
> if (bar) {
> b = 7;
> } else {
> use(a);
> }
> use(b);
>
> If "use(a)" is the last use of the variable a, then the live ranges of a
> and b don't actually over-lap and we can assign a and b to the same
> register. However, if force_writemask_all is set on b, it will destroy the
> value in a before its last use. Right now, we don't actually do this
> because our liveness analysis pass flattens everything so it will think
> that b and a over-lap even though they don't. However, if we ever decide
> to make the liveness information more accurate, having a pile of
> force_writemask_all instructions will be a major problem. So, while it is
> *technically* safe for now, it's a really bad idea in the long-term.
>
The thing is we *will* have to deal with that scenario. Building a
message payload inherently involves crossing channel boundaries (because
of headers, unsupported SIMD modes by some shared functions, etc.). I'd
say it's a bug in the liveness analysis pass if it wouldn't consider the
liveness interval of a and b to overlap in your example if the
assignment of b had force_writemask_all set.A reasonable compromise might be to leave it up to the caller whether to set the force_writemask_all and force_sechalf flags or not. I.e. just copy the same flags set on the LOAD_PAYLOAD instruction to the MOV instructions. That would still allow reducing the liveness intervals in cases where we know that the payload respects channel boundaries. Tracking the flag information per-register in cases where the same payload has well- and ill-behaved values seems rather premature and rather useless to me because the optimizer is likely to be able to get rid of redundant copies with force_writemask_all -- register coalesce is doing this already AFAIK, maybe by accident. > Regarding the other suggestion of just requiring width == exec_size for > immediates and uniforms, that seems pretty reasonable to me. I'd like to > know what it will do to optimizations, but it seems ok initially. I'm > still a bigger fan of just subclassing and stashing everything we need to > know up-front. If we do it right, the only things that will actually need > to know about the subclass are the function for creating a LOAD_PAYLOAD and > lower_load_payloads. > > --Jason > > >> >> [1] >> http://lists.freedesktop.org/archives/mesa-dev/2015-January/074614.html >> [2] >> http://lists.freedesktop.org/archives/mesa-dev/2015-February/076094.html >> [3] >> http://lists.freedesktop.org/archives/mesa-dev/2015-February/076097.html >> [4] >> http://lists.freedesktop.org/archives/mesa-dev/2015-February/076098.html >> >> >> > --Jason >> > >> > On Thu, Feb 19, 2015 at 1:53 PM, Jason Ekstrand <ja...@jlekstrand.net> >> > wrote: >> > >> >> >> >> >> >> On Thu, Feb 19, 2015 at 1:25 PM, Francisco Jerez <curroje...@riseup.net >> > >> >> wrote: >> >> >> >>> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >>> >> >>> > On Thu, Feb 19, 2015 at 12:13 PM, Francisco Jerez < >> >>> curroje...@riseup.net> >> >>> > wrote: >> >>> > >> >>> >> Jason Ekstrand <ja...@jlekstrand.net> writes: >> >>> >> >> >>> >> > On Fri, Feb 6, 2015 at 4:01 PM, Francisco Jerez < >> >>> curroje...@riseup.net> >> >>> >> > wrote: >> >>> >> > >> >>> >> >> Hey Matt, >> >>> >> >> >> >>> >> >> Matt Turner <matts...@gmail.com> writes: >> >>> >> >> >> >>> >> >> > On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez < >> >>> >> curroje...@riseup.net> >> >>> >> >> wrote: >> >>> >> >> >> MRFs cannot be read from anyway so they cannot possibly be a >> >>> valid >> >>> >> >> >> source of LOAD_PAYLOAD. >> >>> >> >> >> --- >> >>> >> >> > >> >>> >> >> > The function only seems to test inst->dst.file == MRF. I don't >> >>> see any >> >>> >> >> > code for handling MRF sources. What am I missing? >> >>> >> >> >> >>> >> >> That test is for "handling" MRF sources -- More precisely, it's >> >>> >> >> collecting the writemask and half flags for MRF writes, which can >> >>> only >> >>> >> >> possibly be useful if we're going to use them later on to read >> >>> something >> >>> >> >> out of an MRF into a payload, which we shouldn't be doing in the >> >>> first >> >>> >> >> place. >> >>> >> >> >> >>> >> >> Aside from simplifying the function somewhat, that allows us to >> >>> drop the >> >>> >> >> 16 register gap reserved for MRFs at register offset zero, what >> will >> >>> >> >> allow us to drop the vgrf_to_reg[] offset calculation completely >> >>> (also >> >>> >> >> in split_virtual_grfs()) in a future patch (not sent for review >> >>> yet). >> >>> >> >> >> >>> >> > >> >>> >> > No, we do read from MRF's sort-of... Send messages have an >> implicit >> >>> >> "read" >> >>> >> > from an MRF. >> >>> >> >> >>> >> Heh, and that's pretty much the only way you "read" from it. >> >>> >> >> >>> >> > This was written precicely so that we could use LOAD_PAYLOAD >> >>> >> > to build MRF payloads. We do on pre-GEN6. >> >>> >> > >> >>> >> I'm aware, but you don't need any of this meta-data to LOAD_PAYLOAD >> >>> >> *into* an MRF, and LOAD_PAYLOAD with an MRF as source should be >> illegal >> >>> >> anyway. >> >>> >> >> >>> > >> >>> > And no one is using it that way. All of the metadata checks you are >> >>> > deleting are checks on the *destination*. >> >>> > >> >>> >> >>> Didn't you write this code yourself? The only use for the collected >> >>> metadata is initializing the instruction flags of the MOVs subsequent >> >>> LOAD_PAYLOAD instructions are lowered to, based on the metadata already >> >>> collected for its source registers, which can never be MRFs, so the >> >>> metadata you collect from MRF writes is never actually used. >> >>> >> >> >> >> Right... I misred something initially. Yes, we should never be tracking >> >> MRF's as a source of a LOAD_PAYLOAD. I'll give it a better look a bit >> >> later, but it looks better. >> >> >>
pgpjCYAIAA08P.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
