[Mesa-dev] [PATCH] i965/vec4: Consider sources of non-GRF-dst instructions for dead channels.

[Eric Anholt]

Matt Turner <mattst88 at gmail.com> writes:

> Previously we'd ignore the sources of instructions with non-GRF
> destinations when calculating calculating the dead channels. This would
> lead to us incorrectly removing the first instruction in this sequence:
>
>    mov vgrf11, ...
>    cmp.ne.f0 null, vgrf11, 1.0
>    mov vgrf11, ...
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76616
> ---
>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index 32a3892..4d8740a 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -324,6 +324,9 @@ src_reg::equals(src_reg *r)
>  static bool
>  try_eliminate_instruction(vec4_instruction *inst, int new_writemask)
>  {
> +   if (inst->has_side_effects())
> +      return false;
> +
>     if (new_writemask == 0) {
>        /* Don't dead code eliminate instructions that write to the
>         * accumulator as a side-effect. Instead just set the destination
> @@ -383,9 +386,6 @@ vec4_visitor::dead_code_eliminate()
>  
>        seen_control_flow = inst->is_control_flow() || seen_control_flow;
>  
> -      if (inst->has_side_effects())
> -         continue;
> -
>        bool inst_writes_flag = false;
>        if (inst->dst.file != GRF) {
>           if (inst->dst.is_null() && inst->writes_flag()) {
> @@ -438,22 +438,19 @@ vec4_visitor::dead_code_eliminate()
>             node = prev, prev = prev->prev) {
>           vec4_instruction *scan_inst = (vec4_instruction  *)node;
>  
> -         if (scan_inst->has_side_effects())
> -            continue;
> -
>           if (inst_writes_flag) {
>              if (scan_inst->dst.is_null() && scan_inst->writes_flag()) {
>                 scan_inst->remove();
>                 progress = true;
> +               continue;

You don't want to be remove()ing in the has_side_effects() case, right?
(think removing an atomic inc).

> -         } else if (scan_inst->dst.file != GRF) {
> -            continue;
>           }
>  
> -         if (inst->dst.reg == scan_inst->dst.reg) {
> +         if (inst->dst.file == GRF &&
> +             scan_inst->dst.file == GRF &&
> +             inst->dst.reg == scan_inst->dst.reg) {
>              int new_writemask = scan_inst->dst.writemask & ~dead_channels;
>  
>              progress = try_eliminate_instruction(scan_inst, new_writemask) ||

Doesn't this change prevent the inst_writes_flag == true case from
optimizing?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140331/c58596ed/attachment.sig>