[Mesa-dev] [PATCH 1/3] glsl: don't drop intructions from unreachable terminators continue branch

[Nicolai Hähnle]

On 14.09.2017 06:47, Timothy Arceri wrote:
> These instruction will be executed on every iteration of the loop
> we cannot drop them.
> ---
>   src/compiler/glsl/loop_analysis.h   |  7 +++++++
>   src/compiler/glsl/loop_controls.cpp | 15 +++++++++++++++
>   src/compiler/glsl/loop_unroll.cpp   |  7 -------
>   3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/src/compiler/glsl/loop_analysis.h b/src/compiler/glsl/loop_analysis.h
> index 2894c6359b..0e1bfd8142 100644
> --- a/src/compiler/glsl/loop_analysis.h
> +++ b/src/compiler/glsl/loop_analysis.h
> @@ -27,20 +27,27 @@
>   
>   #include "ir.h"
>   #include "util/hash_table.h"
>   
>   /**
>    * Analyze and classify all variables used in all loops in the instruction list
>    */
>   extern class loop_state *
>   analyze_loop_variables(exec_list *instructions);
>   
> +static inline bool
> +is_break(ir_instruction *ir)
> +{
> +   return ir != NULL && ir->ir_type == ir_type_loop_jump &&
> +      ((ir_loop_jump *) ir)->is_break();
> +}
> +
>   
>   /**
>    * Fill in loop control fields
>    *
>    * Based on analysis of loop variables, this function tries to remove
>    * redundant sequences in the loop of the form
>    *
>    *  (if (expression bool ...) (break))
>    *
>    * For example, if it is provable that one loop exit condition will
> diff --git a/src/compiler/glsl/loop_controls.cpp b/src/compiler/glsl/loop_controls.cpp
> index 895954fc2d..2dff26aec0 100644
> --- a/src/compiler/glsl/loop_controls.cpp
> +++ b/src/compiler/glsl/loop_controls.cpp
> @@ -215,21 +215,36 @@ loop_control_visitor::visit_leave(ir_loop *ir)
>       * that are associated with a fixed iteration count, except for the one
>       * associated with the limiting terminator--that one needs to stay, since
>       * it terminates the loop.  Exception: if the loop still has a normative
>       * bound, then that terminates the loop, so we don't even need the limiting
>       * terminator.
>       */
>      foreach_in_list(loop_terminator, t, &ls->terminators) {
>         if (t->iterations < 0)
>            continue;
>   
> +      exec_list *branch_instructions;
>         if (t != ls->limiting_terminator) {
> +         ir_instruction *ir_if_last = (ir_instruction *)
> +           t->ir->then_instructions.get_tail();
> +         if (is_break(ir_if_last)) {
> +            branch_instructions = &t->ir->else_instructions;
> +         } else {
> +            branch_instructions = &t->ir->then_instructions;
> +            assert(is_break(ir_if_last));

This should probably be

    assert(is_break((ir_instruction *)t->ir->else_instructions.get_tail()));

The fact that this wasn't hit suggests that maybe we need a piglit test 
for this?

(I generally suggest running CTS / piglit with an optimized build that 
has assertions enabled. My configure line for that looks like this:

../mesa-src/configure --prefix=$PREFIX_SAN --enable-sanitize=address 
--with-dri-drivers=no --with-gallium-drivers=swrast,r300,r600,radeonsi 
--with-egl-platforms=x11,drm --enable-texture-float 
--enable-llvm-shared-libs --enable-glx-tls --enable-debug 
--with-vulkan-drivers=radeon CC=/usr/lib/ccache/cc 
CXX=/usr/lib/ccache/c++ "CFLAGS=-fno-omit-frame-pointer -g -O2" 
"CXXFLAGS=-fno-omit-frame-pointer -g -O2"

Now that I look at it, parts are probably outdated, but the basic idea 
of --enable-debug and -g -O2 is sound :) )

Cheers,
Nicolai


> +         }
> +
> +         exec_list copy_list;
> +         copy_list.make_empty();
> +         clone_ir_list(ir, &copy_list, branch_instructions);
> +
> +         t->ir->insert_before(&copy_list);
>            t->ir->remove();
>   
>            assert(ls->num_loop_jumps > 0);
>            ls->num_loop_jumps--;
>   
>            this->progress = true;
>         }
>      }
>   
>      return visit_continue;
> diff --git a/src/compiler/glsl/loop_unroll.cpp b/src/compiler/glsl/loop_unroll.cpp
> index dbb3fa2fa5..7f601295a1 100644
> --- a/src/compiler/glsl/loop_unroll.cpp
> +++ b/src/compiler/glsl/loop_unroll.cpp
> @@ -46,27 +46,20 @@ public:
>      void splice_post_if_instructions(ir_if *ir_if, exec_list *splice_dest);
>   
>      loop_state *state;
>   
>      bool progress;
>      const struct gl_shader_compiler_options *options;
>   };
>   
>   } /* anonymous namespace */
>   
> -static bool
> -is_break(ir_instruction *ir)
> -{
> -   return ir != NULL && ir->ir_type == ir_type_loop_jump
> -		     && ((ir_loop_jump *) ir)->is_break();
> -}
> -
>   class loop_unroll_count : public ir_hierarchical_visitor {
>   public:
>      int nodes;
>      bool unsupported_variable_indexing;
>      bool array_indexed_by_induction_var_with_exact_iterations;
>      /* If there are nested loops, the node count will be inaccurate. */
>      bool nested_loop;
>   
>      loop_unroll_count(exec_list *list, loop_variable_state *ls,
>                        const struct gl_shader_compiler_options *options)
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.