[Mesa-dev] [PATCH 07/10] i965: Don't copy prop source mods into instructions that can't take them.

[Kenneth Graunke]

On 09/23/2013 04:13 PM, Matt Turner wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp     |  3 +++
>  src/mesa/drivers/dri/i965/brw_shader.cpp | 19 +++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_shader.h   |  1 +
>  src/mesa/drivers/dri/i965/brw_vec4.cpp   |  3 +++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 6dc37ea..944063c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -366,6 +366,9 @@ fs_visitor::can_do_source_mods(fs_inst *inst)
>     if (inst->is_send_from_grf())
>        return false;
>  
> +   if (!inst->can_do_source_mods())
> +      return false;
> +
>     return true;
>  }
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> index a558d36..4970607 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -567,6 +567,25 @@ backend_instruction::is_control_flow()
>     }
>  }
>  
> +bool
> +backend_instruction::can_do_source_mods()
> +{
> +   switch (opcode) {
> +   case BRW_OPCODE_ADDC:
> +   case BRW_OPCODE_BFE:
> +   case BRW_OPCODE_BFI1:
> +   case BRW_OPCODE_BFI2:
> +   case BRW_OPCODE_BFREV:
> +   case BRW_OPCODE_CBIT:
> +   case BRW_OPCODE_FBH:
> +   case BRW_OPCODE_FBL:
> +   case BRW_OPCODE_SUBB:
> +      return false;
> +   default:
> +      return true;
> +   }
> +}
> +

Putting this in backend_instruction makes a ton of sense, but I don't
like having three methods:

bool backend_instruction::can_do_source_mods()
bool fs_visitor::can_do_source_mods(fs_inst *)
bool vec4_visitor::can_do_source_mods(vec4_instruction *)

Since backend_instruction is the obvious place, I'm concerned that
people will start using that directly...when it doesn't tell the full
story.  (The other two impose generation-specific restrictions.)

Frankly though, I think we should take a different approach: just allow
source modifiers on all instructions in the backend IRs.  Then, have a
lowering pass that detects instructions that can't handle source
modifiers and emits workaround MOVs.  It would run after optimization
(obviously), but before scheduling or register allocation.

This makes the IR reflect the basic concepts of the assembly without
needing to capture all of the generation specific details.  Simpler.

The current system of "make every optimization pass try to avoid bad
things" is also kind of scary.  There are so many opportunities to miss
something, especially as we add more optimization passes.  And if we
miss a case, we get misrendering or GPU hangs...

We could also handle fix_math_operand in that pass.

>  void
>  backend_visitor::dump_instructions()
>  {
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> index 55769ff..f60d550 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -45,6 +45,7 @@ public:
>     bool is_tex();
>     bool is_math();
>     bool is_control_flow();
> +   bool can_do_source_mods();
>  
>     enum opcode opcode; /* BRW_OPCODE_* or FS_OPCODE_* */
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index 6a3285a..66988d4 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -222,6 +222,9 @@ vec4_visitor::can_do_source_mods(vec4_instruction *inst)
>     if (inst->is_send_from_grf())
>        return false;
>  
> +   if (!inst->can_do_source_mods())
> +      return false;
> +
>     return true;
>  }
>  
>