[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 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.

> 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.
-------------- 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/20150606/60aaebc6/attachment.sig>