[Mesa-dev] [PATCH 7/9] i965/fs: Restrict optimization that would fail for gen7's SENDs from GRFs

[Kenneth Graunke]

On 11/12/2012 10:53 AM, Eric Anholt wrote:
> ---
>   src/mesa/drivers/dri/i965/brw_fs.cpp               |   28 ++++++++++++++++----
>   src/mesa/drivers/dri/i965/brw_fs.h                 |    3 +++
>   .../drivers/dri/i965/brw_fs_copy_propagation.cpp   |    5 ++--
>   3 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 752ed10..c0bad60 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -288,6 +288,24 @@ fs_inst::is_math()
>              opcode == SHADER_OPCODE_POW);
>   }
>
> +bool
> +fs_inst::is_send_from_grf()
> +{
> +   return opcode == FS_OPCODE_VARYING_PULL_CONSTANT_LOAD_GEN7;
> +}

Hmm.  This has me thinking: in the VS backend, we do send from GRF for 
uniform pull constant loads on Gen7 as well.  We could run into trouble 
there, too...

> +bool
> +fs_visitor::can_do_source_mods(fs_inst *inst)
> +{
> +   if (intel->gen == 6 && inst->is_math())
> +      return false;
> +
> +   if (inst->is_send_from_grf())
> +      return false;
> +
> +   return true;
> +}
> +
>   void
>   fs_reg::init()
>   {
> @@ -1588,7 +1606,9 @@ fs_visitor::register_coalesce()
>   	  inst->dst.type != inst->src[0].type)
>   	 continue;
>
> -      bool has_source_modifiers = inst->src[0].abs || inst->src[0].negate;
> +      bool has_source_modifiers = (inst->src[0].abs ||
> +                                   inst->src[0].negate ||
> +                                   inst->src[0].file == UNIFORM);
>
>         /* Found a move of a GRF to a GRF.  Let's see if we can coalesce
>          * them: check for no writes to either one until the exit of the
> @@ -1611,10 +1631,8 @@ fs_visitor::register_coalesce()
>   	  * unusual register regions, so avoid coalescing those for
>   	  * now.  We should do something more specific.
>   	  */
> -	 if (intel->gen >= 6 &&
> -	     scan_inst->is_math() &&
> -	     (has_source_modifiers || inst->src[0].file == UNIFORM)) {
> -	    interfered = true;
> +	 if (has_source_modifiers && !can_do_source_mods(inst)) {
> +            interfered = true;
>   	    break;
>   	 }

This looks like it'll prevent MATH on uniforms for Gen6.  As far as I 
know, that's allowed, so I'd rather not lose that...

Also, there's another subtle change here: >= 6 becomes == 6.  In other 
words, we begin allowing source modifiers on Gen7+.  This is correct, 
but I'd really love to see it in a separate 1-line patch for the sake of 
bisectability.  (Call me paranoid, but that's the sort of thing that can 
lead to bizarre failures in obscure cases...)

> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 9eefe16..3ebbf93 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -154,6 +154,7 @@ public:
>      bool overwrites_reg(const fs_reg &reg);
>      bool is_tex();
>      bool is_math();
> +   bool is_send_from_grf();
>
>      fs_reg dst;
>      fs_reg src[3];
> @@ -212,6 +213,8 @@ public:
>
>      void swizzle_result(ir_texture *ir, fs_reg orig_val, int sampler);
>
> +   bool can_do_source_mods(fs_inst *inst);
> +
>      fs_inst *emit(fs_inst inst);
>      fs_inst *emit(fs_inst *inst);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> index d296e48..c9c9028 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -216,9 +216,8 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>
>      bool has_source_modifiers = entry->src.abs || entry->src.negate;
>
> -   if (intel->gen == 6 && inst->is_math() &&
> -       (has_source_modifiers || entry->src.file == UNIFORM ||
> -        entry->src.smear != -1))
> +   if ((has_source_modifiers || entry->src.file == UNIFORM ||
> +        entry->src.smear != -1) && !can_do_source_mods(inst))
>         return false;
>
>      inst->src[arg].file = entry->src.file;
>