[Mesa-dev] [PATCH 08/38] i965/fs: Don't drop force_writemask_all and _sechalf when copying a CSE temporary.

[Matt Turner]

On Fri, Jun 5, 2015 at 4:19 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>
>> On Thu, Jun 4, 2015 at 9:04 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>>> index 3ddd17c..822a6a3 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>>> @@ -206,9 +206,10 @@ create_copy_instr(fs_visitor *v, fs_inst *inst, fs_reg src, bool negate)
>>>        copy = v->LOAD_PAYLOAD(inst->dst, payload, sources, header_size);
>>>     } else {
>>>        copy = v->MOV(inst->dst, src);
>>> -      copy->force_writemask_all = inst->force_writemask_all;
>>>        copy->src[0].negate = negate;
>>>     }
>>> +   copy->force_writemask_all = inst->force_writemask_all;
>>> +   copy->force_sechalf = inst->force_sechalf;
>>
>> I thought we resolved not to allow force_writemask_all/force_sechalf
>> on load_payload instructions?
>>
> I don't see why not, it's easy to come up with a well-behaved
> interpretation of those flags for LOAD_PAYLOAD (apply them to all
> payload MOVs it's lowered to), it's going to be useful, and, well,
> except for this bug it's already working.

Okay, I reread the PATCH 09/13 thread about how load_payload should
behave and had misremembered me saying that we shouldn't allow
*saturate* on load_payload. We did indeed agree to allow
force_writemask_all/force_sechalf -- Jason summed it up:

> Sure.  I can drop saturate and just assert that it's not set.  We do
> want to keep force_sechalf and force_writemask_all though.

>> I see in lower_load_payload that we often do
>>
>>    mov->force_writemask_all = inst->force_writemask_all;
>>
>> where inst is the load_payload, as if its force_writemask_all might be
>> set, but I don't see any places where we're setting it. I also don't
>> see any uses of force_sechalf with load_payload. Will
>> lower_load_payload do the "right thing" with these set? What is the
>> "right thing"?
>>
> Yes, it does the right thing already.
>
>> We've clearly got a bug in that we're dropping force_sechalf on the
>> MOV, but what problem are you fixing with the LOAD_PAYLOAD?
>
>> Presumably something later will emit a texture or an atomic with
>> force_sechalf and if we drop force_sechalf during CSE we'll break
>> something? That's the kind of thing the commit message needs to say.
>
> It turns out that, yes, image load/store will need LOAD_PAYLOAD
> instructions with force_sechalf set, but I didn't expect it to need any
> further explanation, it seemed obviously incorrect to respect the
> execution controls of the generating instruction *only* if the
> generating instruction wasn't of certain type.

That's fine and makes sense, but having reviewed 78 committed patches
from you this year (and a bunch more that weren't), I think I'm
uniquely qualified to say that better commit messages would help
reviewers a lot.

The point is that to in order to review your code they have to
understand what you're thinking

In order to review your code I have to understand what you're
thinking. I can do that by reading a commit summary that explains what
you're thinking, I can attempt to guess, or I can spend time writing
you an email with a question and wait for a response -- a response
that in most cases contains information that I really just expected to
see in the commit summary, and had it been there would have saved us
both time.

The patch with the commit message is

Reviewed-by: Matt Turner <mattst88 at gmail.com>