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

[Timothy Arceri]

On 19/6/19 11:55 pm, Brian Paul wrote:
> 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;
>              ^~~~~~~~~

Whoops I meant to make a patch to avoid this warning. I believe the 
compiler warning is wrong here.

> 
> -Brian