[Mesa-dev] [PATCH 1/2] nir/loop_analyze: used nir_alu_src to track loop limit

[Brian Paul]

On 06/19/2019 02:08 AM, Timothy Arceri wrote:
> This helps reduce the amount of abstraction in this pass and allows
> us to retain more information about the src such as any swizzles.
> Retaining the swizzle information is required for a bugfix in the
> following patch.
> 
> Fixes: 6772a17acc8e ("nir: Add a loop analysis pass")
> ---
>   src/compiler/nir/nir_loop_analyze.c | 37 +++++++++++++++--------------
>   1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_loop_analyze.c b/src/compiler/nir/nir_loop_analyze.c
> index e85a404da1b..57d2d94cad2 100644
> --- a/src/compiler/nir/nir_loop_analyze.c
> +++ b/src/compiler/nir/nir_loop_analyze.c
> @@ -543,25 +543,26 @@ guess_loop_limit(loop_info_state *state, nir_const_value *limit_val,
>   }
>   
>   static bool
> -try_find_limit_of_alu(nir_loop_variable *limit, nir_const_value *limit_val,
> -                      nir_loop_terminator *terminator, loop_info_state *state)
> +try_find_limit_of_alu(nir_alu_src *limit, nir_const_value *limit_val,
> +                      nir_loop_terminator *terminator)
>   {
> -   if(!is_var_alu(limit))
> +   if(limit->src.ssa->parent_instr->type != nir_instr_type_alu)
>         return false;
>   
> -   nir_alu_instr *limit_alu = nir_instr_as_alu(limit->def->parent_instr);
> +   nir_alu_instr *limit_alu = nir_instr_as_alu(limit->src.ssa->parent_instr);
>   
>      if (limit_alu->op == nir_op_imin ||
>          limit_alu->op == nir_op_fmin) {
> -      limit = get_loop_var(limit_alu->src[0].src.ssa, state);
> +      limit = &limit_alu->src[0];
>   
> -      if (!is_var_constant(limit))
> -         limit = get_loop_var(limit_alu->src[1].src.ssa, state);
> +      if (limit->src.ssa->parent_instr->type != nir_instr_type_load_const)
> +         limit = &limit_alu->src[1];
>   
> -      if (!is_var_constant(limit))
> +      if (limit->src.ssa->parent_instr->type != nir_instr_type_load_const)
>            return false;
>   
> -      *limit_val = nir_instr_as_load_const(limit->def->parent_instr)->value[0];
> +      *limit_val =
> +         nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];
>   
>         terminator->exact_trip_count_unknown = true;
>   
> @@ -777,19 +778,19 @@ is_supported_terminator_condition(nir_alu_instr *alu)
>   
>   static bool
>   get_induction_and_limit_vars(nir_alu_instr *alu, nir_loop_variable **ind,
> -                             nir_loop_variable **limit,
> +                             nir_alu_src **limit,
>                                loop_info_state *state)
>   {
>      bool limit_rhs = true;
>   
>      /* We assume that the limit is the "right" operand */
>      *ind = get_loop_var(alu->src[0].src.ssa, state);
> -   *limit = get_loop_var(alu->src[1].src.ssa, state);
> +   *limit = &alu->src[1];
>   
>      if ((*ind)->type != basic_induction) {
>         /* We had it the wrong way, flip things around */
>         *ind = get_loop_var(alu->src[1].src.ssa, state);
> -      *limit = get_loop_var(alu->src[0].src.ssa, state);
> +      *limit = &alu->src[0];
>         limit_rhs = false;
>      }
>   
> @@ -799,7 +800,7 @@ get_induction_and_limit_vars(nir_alu_instr *alu, nir_loop_variable **ind,
>   static void
>   try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
>                                    nir_loop_variable **ind,
> -                                 nir_loop_variable **limit,
> +                                 nir_alu_src **limit,
>                                    bool *limit_rhs,
>                                    loop_info_state *state)
>   {
> @@ -848,7 +849,7 @@ try_find_trip_count_vars_in_iand(nir_alu_instr **alu,
>   
>      /* Try the other iand src if needed */
>      if (*ind == NULL || (*ind && (*ind)->type != basic_induction) ||
> -       !is_var_constant(*limit)) {
> +       (*limit)->src.ssa->parent_instr->type != nir_instr_type_load_const) {
>         src = iand->src[1].src.ssa;
>         if (src->parent_instr->type == nir_instr_type_alu) {
>            nir_alu_instr *tmp_alu = nir_instr_as_alu(src->parent_instr);
> @@ -891,7 +892,7 @@ find_trip_count(loop_info_state *state)
>   
>         bool limit_rhs;
>         nir_loop_variable *basic_ind = NULL;
> -      nir_loop_variable *limit;
> +      nir_alu_src *limit;
>         if (alu->op == nir_op_inot || alu->op == nir_op_ieq) {
>            nir_alu_instr *new_alu = alu;
>            try_find_trip_count_vars_in_iand(&new_alu, &basic_ind, &limit,
> @@ -931,13 +932,13 @@ find_trip_count(loop_info_state *state)
>   
>         /* Attempt to find a constant limit for the loop */
>         nir_const_value limit_val;
> -      if (is_var_constant(limit)) {
> +      if (limit->src.ssa->parent_instr->type == nir_instr_type_load_const) {
>            limit_val =
> -            nir_instr_as_load_const(limit->def->parent_instr)->value[0];
> +            nir_instr_as_load_const(limit->src.ssa->parent_instr)->value[0];
>         } else {
>            trip_count_known = false;
>   
> -         if (!try_find_limit_of_alu(limit, &limit_val, terminator, state)) {
> +         if (!try_find_limit_of_alu(limit, &limit_val, terminator)) {
>               /* Guess loop limit based on array access */
>               if (!guess_loop_limit(state, &limit_val, basic_ind)) {
>                  continue;
> 

I'm seeing a suspicious warning with these patches:

[105/1031] Compiling C object 
'src/com...ler/nir/nir at sta/nir_loop_analyze.c.o'.
../src/compiler/nir/nir_loop_analyze.c: In function ‘process_loops’:
../src/compiler/nir/nir_loop_analyze.c:933:7: warning: ‘limit_rhs’ may 
be used uninitialized in this function [-Wmaybe-uninitialized]
        terminator->induction_rhs = !limit_rhs;
        ^~~~~~~~~~
../src/compiler/nir/nir_loop_analyze.c:895:12: note: ‘limit_rhs’ was 
declared here
        bool limit_rhs;
             ^~~~~~~~~

-Brian