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

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> 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 thank you for that Matt.

> 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.
>
I had the impression that asking questions is quite an efficient
workflow actually.

This series has an example of a very long commit message (PATCH 01),
even in relation to the amount of changes it makes -- I'm not saying
that the whole series was like that, it definitely also had a few
patches with less-than-average commit messages -- What I'm trying to
point out is that I'm happy to spend time documenting what a change does
*when* I feel that there is something non-trivial to say.  For the same
reason that it was understandably frustrating for you to try to guess
what I was thinking when I wrote this patch, it's frustrating for me to
try to guess what information I consider obvious is going to be less
obvious (or crucial to understand the context of the commit) for other
people.  I can only try to make a guess, as I can't know for certain
what the reviewer does and doesn't know (not even who it's going to be)
until some communication has happened in both directions.

I don't think there's anything wrong about exchanging questions and
answers when there's something not particularly clear about a change:
trying to avoid that situation at all costs by being pessimistic about
what the reviewer doesn't know is a waste of time for both the author of
a commit (because of the amount of time invested in explaining things
that will in the end turn out to be obvious for other people) and the
reviewer (because of the amount of information you'll have to read that
you already know).

Writing down and answering a *specific* question (rather than trying to
answer all conceivable questions in advance) and possibly updating the
commit message with the answer for future reference seems like a
comparatively minor harm.

> The patch with the commit message is
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150608/1cce2056/attachment-0001.sig>