[Mesa-dev] [PATCH 10/32] i965/fs: Remove logic to keep track of MRF metadata in lower_load_payload().

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> I'm still a little pensive.  But
>
> Reviewed-by: Jason Ekstrand <jason.ekstrand at 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.

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.

[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 <jason at jlekstrand.net>
> wrote:
>
>>
>>
>> On Thu, Feb 19, 2015 at 1:25 PM, Francisco Jerez <currojerez at riseup.net>
>> wrote:
>>
>>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>>
>>> > On Thu, Feb 19, 2015 at 12:13 PM, Francisco Jerez <
>>> currojerez at riseup.net>
>>> > wrote:
>>> >
>>> >> Jason Ekstrand <jason at jlekstrand.net> writes:
>>> >>
>>> >> > On Fri, Feb 6, 2015 at 4:01 PM, Francisco Jerez <
>>> currojerez at riseup.net>
>>> >> > wrote:
>>> >> >
>>> >> >> Hey Matt,
>>> >> >>
>>> >> >> Matt Turner <mattst88 at gmail.com> writes:
>>> >> >>
>>> >> >> > On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <
>>> >> currojerez at 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.
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150220/6c45f7c7/attachment.sig>