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

[Matt Turner]

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 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"?

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.