[1/2] nir/loop_analyze: used nir_alu_src to track loop limit

Submitted by Timothy Arceri on June 19, 2019, 8:08 a.m.

Details

Message ID 20190619080900.16648-1-tarceri@itsqueeze.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Timothy Arceri June 19, 2019, 8:08 a.m.
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(-)

Patch hide | download patch | download mbox

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;

Comments

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@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

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@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
On 20/6/19 5:57 am, Jason Ekstrand wrote:
> On Wed, Jun 19, 2019 at 3:09 AM Timothy Arceri <tarceri@itsqueeze.com 
> <mailto:tarceri@itsqueeze.com>> 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];
> 
> 
> This is still horribly broken w.r.t swizzles because we're not tracking 
> the component as we make this or the jump above for [if]min.

I think we should probably just do a check to make sure the limit and 
invariant are scalar if we don't already. The rest of the pass cannot 
handle vectors anyway swizzles or not, and I don't think we should 
bother handling it either.


> 
> 
>     -      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;
>     -- 
>     2.21.0
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On 20/6/19 5:57 am, Jason Ekstrand wrote:
> On Wed, Jun 19, 2019 at 3:09 AM Timothy Arceri <tarceri@itsqueeze.com 
> <mailto:tarceri@itsqueeze.com>> 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];
> 
> 
> This is still horribly broken w.r.t swizzles because we're not tracking 
> the component as we make this or the jump above for [if]min.

On further inspection I don't think this is a problem because the GLSL 
rules say the loop condition must be scalar boolean.

> 
> 
>     -      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;
>     -- 
>     2.21.0
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>