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

[Timothy Arceri]

On 18/09/17 20:09, Nicolai Hähnle wrote:
> 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()));

Yeah that looks right.

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

I'll double check but I recall the break always being put in the then 
branch regardless of what the GLSL looks like. This is here for 
completeness more than anything.


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

Yes, I always test this way :) although occasionally I do forget to rebuild.

> 
> ../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)
>>
> 
>