[1/2] nir: add an option for skipping split_alu_of_phi

Submitted by Samuel Pitoiset on April 23, 2019, 7:38 a.m.

Details

Message ID 20190423073857.24623-1-samuel.pitoiset@gmail.com
State Rejected
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset April 23, 2019, 7:38 a.m.
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/amd/vulkan/radv_shader.c                 |  2 +-
 src/compiler/nir/nir.h                       |  3 ++-
 src/compiler/nir/nir_opt_if.c                | 17 ++++++++++-------
 src/freedreno/ir3/ir3_nir.c                  |  2 +-
 src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
 src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
 src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
 src/intel/compiler/brw_nir.c                 |  2 +-
 src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
 9 files changed, 19 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
index 13f1f9aa9dc..54a4e732230 100644
--- a/src/amd/vulkan/radv_shader.c
+++ b/src/amd/vulkan/radv_shader.c
@@ -158,7 +158,7 @@  radv_optimize_nir(struct nir_shader *shader, bool optimize_conservatively,
 			NIR_PASS(progress, shader, nir_opt_remove_phis);
                         NIR_PASS(progress, shader, nir_opt_dce);
                 }
-                NIR_PASS(progress, shader, nir_opt_if, true);
+                NIR_PASS(progress, shader, nir_opt_if, true, false);
                 NIR_PASS(progress, shader, nir_opt_dead_cf);
                 NIR_PASS(progress, shader, nir_opt_cse);
                 NIR_PASS(progress, shader, nir_opt_peephole_select, 8, true, true);
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 7d2062d3691..d7506d6ddd1 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -3474,7 +3474,8 @@  bool nir_opt_gcm(nir_shader *shader, bool value_number);
 
 bool nir_opt_idiv_const(nir_shader *shader, unsigned min_bit_size);
 
-bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue);
+bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
+                bool skip_alu_of_phi);
 
 bool nir_opt_intrinsics(nir_shader *shader);
 
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index f674185f1e2..149b3bd1659 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -1385,7 +1385,8 @@  opt_if_cf_list(nir_builder *b, struct exec_list *cf_list,
  * not do anything to cause the metadata to become invalid.
  */
 static bool
-opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
+opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list,
+                    bool skip_alu_of_phi)
 {
    bool progress = false;
    foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
@@ -1395,16 +1396,17 @@  opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
 
       case nir_cf_node_if: {
          nir_if *nif = nir_cf_node_as_if(cf_node);
-         progress |= opt_if_safe_cf_list(b, &nif->then_list);
-         progress |= opt_if_safe_cf_list(b, &nif->else_list);
+         progress |= opt_if_safe_cf_list(b, &nif->then_list, skip_alu_of_phi);
+         progress |= opt_if_safe_cf_list(b, &nif->else_list, skip_alu_of_phi);
          progress |= opt_if_evaluate_condition_use(b, nif);
          break;
       }
 
       case nir_cf_node_loop: {
          nir_loop *loop = nir_cf_node_as_loop(cf_node);
-         progress |= opt_if_safe_cf_list(b, &loop->body);
-         progress |= opt_split_alu_of_phi(b, loop);
+         progress |= opt_if_safe_cf_list(b, &loop->body, skip_alu_of_phi);
+         if (!skip_alu_of_phi)
+            progress |= opt_split_alu_of_phi(b, loop);
          break;
       }
 
@@ -1417,7 +1419,8 @@  opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
 }
 
 bool
-nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
+nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
+           bool skip_alu_of_phi)
 {
    bool progress = false;
 
@@ -1430,7 +1433,7 @@  nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
 
       nir_metadata_require(function->impl, nir_metadata_block_index |
                            nir_metadata_dominance);
-      progress = opt_if_safe_cf_list(&b, &function->impl->body);
+      progress = opt_if_safe_cf_list(&b, &function->impl->body, skip_alu_of_phi);
       nir_metadata_preserve(function->impl, nir_metadata_block_index |
                             nir_metadata_dominance);
 
diff --git a/src/freedreno/ir3/ir3_nir.c b/src/freedreno/ir3/ir3_nir.c
index 76230e3be50..1bec3c030a9 100644
--- a/src/freedreno/ir3/ir3_nir.c
+++ b/src/freedreno/ir3/ir3_nir.c
@@ -147,7 +147,7 @@  ir3_optimize_loop(nir_shader *s)
 			OPT(s, nir_copy_prop);
 			OPT(s, nir_opt_dce);
 		}
-		progress |= OPT(s, nir_opt_if, false);
+		progress |= OPT(s, nir_opt_if, false, false);
 		progress |= OPT(s, nir_opt_remove_phis);
 		progress |= OPT(s, nir_opt_undef);
 
diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c b/src/gallium/auxiliary/nir/tgsi_to_nir.c
index c55e8b84a41..6b40bff1f73 100644
--- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
+++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
@@ -2066,7 +2066,7 @@  ttn_optimize_nir(nir_shader *nir, bool scalar)
          NIR_PASS(progress, nir, nir_opt_dce);
       }
 
-      NIR_PASS(progress, nir, nir_opt_if, false);
+      NIR_PASS(progress, nir, nir_opt_if, false, false);
       NIR_PASS(progress, nir, nir_opt_dead_cf);
       NIR_PASS(progress, nir, nir_opt_cse);
       NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);
diff --git a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
index ee348ca6a93..3522971e435 100644
--- a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
+++ b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
@@ -94,7 +94,7 @@  ir2_optimize_loop(nir_shader *s)
 			OPT(s, nir_opt_dce);
 		}
 		progress |= OPT(s, nir_opt_loop_unroll, nir_var_all);
-		progress |= OPT(s, nir_opt_if, false);
+		progress |= OPT(s, nir_opt_if, false, false);
 		progress |= OPT(s, nir_opt_remove_phis);
 		progress |= OPT(s, nir_opt_undef);
 
diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c b/src/gallium/drivers/radeonsi/si_shader_nir.c
index 5a925f19e09..7f1fe4ba2e9 100644
--- a/src/gallium/drivers/radeonsi/si_shader_nir.c
+++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
@@ -879,7 +879,7 @@  si_lower_nir(struct si_shader_selector* sel)
 			NIR_PASS(progress, sel->nir, nir_copy_prop);
 			NIR_PASS(progress, sel->nir, nir_opt_dce);
 		}
-		NIR_PASS(progress, sel->nir, nir_opt_if, true);
+		NIR_PASS(progress, sel->nir, nir_opt_if, true, false);
 		NIR_PASS(progress, sel->nir, nir_opt_dead_cf);
 		NIR_PASS(progress, sel->nir, nir_opt_cse);
 		NIR_PASS(progress, sel->nir, nir_opt_peephole_select, 8, true, true);
diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index e0a393fc298..ba911049ce3 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -607,7 +607,7 @@  brw_nir_optimize(nir_shader *nir, const struct brw_compiler *compiler,
          OPT(nir_copy_prop);
          OPT(nir_opt_dce);
       }
-      OPT(nir_opt_if, false);
+      OPT(nir_opt_if, false, false);
       if (nir->options->max_unroll_iterations != 0) {
          OPT(nir_opt_loop_unroll, indirect_mask);
       }
diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
index 97b2831b880..3f1f78e875b 100644
--- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
@@ -324,7 +324,7 @@  st_nir_opts(nir_shader *nir, bool scalar)
          NIR_PASS(progress, nir, nir_copy_prop);
          NIR_PASS(progress, nir, nir_opt_dce);
       }
-      NIR_PASS(progress, nir, nir_opt_if, false);
+      NIR_PASS(progress, nir, nir_opt_if, false, false);
       NIR_PASS(progress, nir, nir_opt_dead_cf);
       NIR_PASS(progress, nir, nir_opt_cse);
       NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);

Comments

On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  src/amd/vulkan/radv_shader.c                 |  2 +-
>  src/compiler/nir/nir.h                       |  3 ++-
>  src/compiler/nir/nir_opt_if.c                | 17 ++++++++++-------
>  src/freedreno/ir3/ir3_nir.c                  |  2 +-
>  src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
>  src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
>  src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
>  src/intel/compiler/brw_nir.c                 |  2 +-
>  src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
>  9 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
> index 13f1f9aa9dc..54a4e732230 100644
> --- a/src/amd/vulkan/radv_shader.c
> +++ b/src/amd/vulkan/radv_shader.c
> @@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader *shader, bool optimize_conservatively,
>                         NIR_PASS(progress, shader, nir_opt_remove_phis);
>                          NIR_PASS(progress, shader, nir_opt_dce);
>                  }
> -                NIR_PASS(progress, shader, nir_opt_if, true);
> +                NIR_PASS(progress, shader, nir_opt_if, true, false);
>                  NIR_PASS(progress, shader, nir_opt_dead_cf);
>                  NIR_PASS(progress, shader, nir_opt_cse);
>                  NIR_PASS(progress, shader, nir_opt_peephole_select, 8, true, true);
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 7d2062d3691..d7506d6ddd1 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -3474,7 +3474,8 @@ bool nir_opt_gcm(nir_shader *shader, bool value_number);
>
>  bool nir_opt_idiv_const(nir_shader *shader, unsigned min_bit_size);
>
> -bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue);
> +bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
> +                bool skip_alu_of_phi);

Can we have a flag for this instead (e.g. something like
nir_opt_if_skip_alu_of_phi)? I think have a function with a bunch of
bools is less than ideal as you can't see at the calling site what is
for what arg.
>
>  bool nir_opt_intrinsics(nir_shader *shader);
>
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index f674185f1e2..149b3bd1659 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -1385,7 +1385,8 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list,
>   * not do anything to cause the metadata to become invalid.
>   */
>  static bool
> -opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
> +opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list,
> +                    bool skip_alu_of_phi)
>  {
>     bool progress = false;
>     foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
> @@ -1395,16 +1396,17 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>
>        case nir_cf_node_if: {
>           nir_if *nif = nir_cf_node_as_if(cf_node);
> -         progress |= opt_if_safe_cf_list(b, &nif->then_list);
> -         progress |= opt_if_safe_cf_list(b, &nif->else_list);
> +         progress |= opt_if_safe_cf_list(b, &nif->then_list, skip_alu_of_phi);
> +         progress |= opt_if_safe_cf_list(b, &nif->else_list, skip_alu_of_phi);
>           progress |= opt_if_evaluate_condition_use(b, nif);
>           break;
>        }
>
>        case nir_cf_node_loop: {
>           nir_loop *loop = nir_cf_node_as_loop(cf_node);
> -         progress |= opt_if_safe_cf_list(b, &loop->body);
> -         progress |= opt_split_alu_of_phi(b, loop);
> +         progress |= opt_if_safe_cf_list(b, &loop->body, skip_alu_of_phi);
> +         if (!skip_alu_of_phi)
> +            progress |= opt_split_alu_of_phi(b, loop);
>           break;
>        }
>
> @@ -1417,7 +1419,8 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>  }
>
>  bool
> -nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
> +nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
> +           bool skip_alu_of_phi)
>  {
>     bool progress = false;
>
> @@ -1430,7 +1433,7 @@ nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
>
>        nir_metadata_require(function->impl, nir_metadata_block_index |
>                             nir_metadata_dominance);
> -      progress = opt_if_safe_cf_list(&b, &function->impl->body);
> +      progress = opt_if_safe_cf_list(&b, &function->impl->body, skip_alu_of_phi);
>        nir_metadata_preserve(function->impl, nir_metadata_block_index |
>                              nir_metadata_dominance);
>
> diff --git a/src/freedreno/ir3/ir3_nir.c b/src/freedreno/ir3/ir3_nir.c
> index 76230e3be50..1bec3c030a9 100644
> --- a/src/freedreno/ir3/ir3_nir.c
> +++ b/src/freedreno/ir3/ir3_nir.c
> @@ -147,7 +147,7 @@ ir3_optimize_loop(nir_shader *s)
>                         OPT(s, nir_copy_prop);
>                         OPT(s, nir_opt_dce);
>                 }
> -               progress |= OPT(s, nir_opt_if, false);
> +               progress |= OPT(s, nir_opt_if, false, false);
>                 progress |= OPT(s, nir_opt_remove_phis);
>                 progress |= OPT(s, nir_opt_undef);
>
> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c b/src/gallium/auxiliary/nir/tgsi_to_nir.c
> index c55e8b84a41..6b40bff1f73 100644
> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
> @@ -2066,7 +2066,7 @@ ttn_optimize_nir(nir_shader *nir, bool scalar)
>           NIR_PASS(progress, nir, nir_opt_dce);
>        }
>
> -      NIR_PASS(progress, nir, nir_opt_if, false);
> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>        NIR_PASS(progress, nir, nir_opt_dead_cf);
>        NIR_PASS(progress, nir, nir_opt_cse);
>        NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);
> diff --git a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
> index ee348ca6a93..3522971e435 100644
> --- a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
> +++ b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
> @@ -94,7 +94,7 @@ ir2_optimize_loop(nir_shader *s)
>                         OPT(s, nir_opt_dce);
>                 }
>                 progress |= OPT(s, nir_opt_loop_unroll, nir_var_all);
> -               progress |= OPT(s, nir_opt_if, false);
> +               progress |= OPT(s, nir_opt_if, false, false);
>                 progress |= OPT(s, nir_opt_remove_phis);
>                 progress |= OPT(s, nir_opt_undef);
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c b/src/gallium/drivers/radeonsi/si_shader_nir.c
> index 5a925f19e09..7f1fe4ba2e9 100644
> --- a/src/gallium/drivers/radeonsi/si_shader_nir.c
> +++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
> @@ -879,7 +879,7 @@ si_lower_nir(struct si_shader_selector* sel)
>                         NIR_PASS(progress, sel->nir, nir_copy_prop);
>                         NIR_PASS(progress, sel->nir, nir_opt_dce);
>                 }
> -               NIR_PASS(progress, sel->nir, nir_opt_if, true);
> +               NIR_PASS(progress, sel->nir, nir_opt_if, true, false);
>                 NIR_PASS(progress, sel->nir, nir_opt_dead_cf);
>                 NIR_PASS(progress, sel->nir, nir_opt_cse);
>                 NIR_PASS(progress, sel->nir, nir_opt_peephole_select, 8, true, true);
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index e0a393fc298..ba911049ce3 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -607,7 +607,7 @@ brw_nir_optimize(nir_shader *nir, const struct brw_compiler *compiler,
>           OPT(nir_copy_prop);
>           OPT(nir_opt_dce);
>        }
> -      OPT(nir_opt_if, false);
> +      OPT(nir_opt_if, false, false);
>        if (nir->options->max_unroll_iterations != 0) {
>           OPT(nir_opt_loop_unroll, indirect_mask);
>        }
> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> index 97b2831b880..3f1f78e875b 100644
> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> @@ -324,7 +324,7 @@ st_nir_opts(nir_shader *nir, bool scalar)
>           NIR_PASS(progress, nir, nir_copy_prop);
>           NIR_PASS(progress, nir, nir_opt_dce);
>        }
> -      NIR_PASS(progress, nir, nir_opt_if, false);
> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>        NIR_PASS(progress, nir, nir_opt_dead_cf);
>        NIR_PASS(progress, nir, nir_opt_cse);
>        NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);
> --
> 2.21.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 4/23/19 10:45 AM, Bas Nieuwenhuizen wrote:
> On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>   src/amd/vulkan/radv_shader.c                 |  2 +-
>>   src/compiler/nir/nir.h                       |  3 ++-
>>   src/compiler/nir/nir_opt_if.c                | 17 ++++++++++-------
>>   src/freedreno/ir3/ir3_nir.c                  |  2 +-
>>   src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
>>   src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
>>   src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
>>   src/intel/compiler/brw_nir.c                 |  2 +-
>>   src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
>>   9 files changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/amd/vulkan/radv_shader.c b/src/amd/vulkan/radv_shader.c
>> index 13f1f9aa9dc..54a4e732230 100644
>> --- a/src/amd/vulkan/radv_shader.c
>> +++ b/src/amd/vulkan/radv_shader.c
>> @@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader *shader, bool optimize_conservatively,
>>                          NIR_PASS(progress, shader, nir_opt_remove_phis);
>>                           NIR_PASS(progress, shader, nir_opt_dce);
>>                   }
>> -                NIR_PASS(progress, shader, nir_opt_if, true);
>> +                NIR_PASS(progress, shader, nir_opt_if, true, false);
>>                   NIR_PASS(progress, shader, nir_opt_dead_cf);
>>                   NIR_PASS(progress, shader, nir_opt_cse);
>>                   NIR_PASS(progress, shader, nir_opt_peephole_select, 8, true, true);
>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>> index 7d2062d3691..d7506d6ddd1 100644
>> --- a/src/compiler/nir/nir.h
>> +++ b/src/compiler/nir/nir.h
>> @@ -3474,7 +3474,8 @@ bool nir_opt_gcm(nir_shader *shader, bool value_number);
>>
>>   bool nir_opt_idiv_const(nir_shader *shader, unsigned min_bit_size);
>>
>> -bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue);
>> +bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
>> +                bool skip_alu_of_phi);
> Can we have a flag for this instead (e.g. something like
> nir_opt_if_skip_alu_of_phi)? I think have a function with a bunch of
> bools is less than ideal as you can't see at the calling site what is
> for what arg.
Yes, that seems better to me.
>>   bool nir_opt_intrinsics(nir_shader *shader);
>>
>> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
>> index f674185f1e2..149b3bd1659 100644
>> --- a/src/compiler/nir/nir_opt_if.c
>> +++ b/src/compiler/nir/nir_opt_if.c
>> @@ -1385,7 +1385,8 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list,
>>    * not do anything to cause the metadata to become invalid.
>>    */
>>   static bool
>> -opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>> +opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list,
>> +                    bool skip_alu_of_phi)
>>   {
>>      bool progress = false;
>>      foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
>> @@ -1395,16 +1396,17 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>>
>>         case nir_cf_node_if: {
>>            nir_if *nif = nir_cf_node_as_if(cf_node);
>> -         progress |= opt_if_safe_cf_list(b, &nif->then_list);
>> -         progress |= opt_if_safe_cf_list(b, &nif->else_list);
>> +         progress |= opt_if_safe_cf_list(b, &nif->then_list, skip_alu_of_phi);
>> +         progress |= opt_if_safe_cf_list(b, &nif->else_list, skip_alu_of_phi);
>>            progress |= opt_if_evaluate_condition_use(b, nif);
>>            break;
>>         }
>>
>>         case nir_cf_node_loop: {
>>            nir_loop *loop = nir_cf_node_as_loop(cf_node);
>> -         progress |= opt_if_safe_cf_list(b, &loop->body);
>> -         progress |= opt_split_alu_of_phi(b, loop);
>> +         progress |= opt_if_safe_cf_list(b, &loop->body, skip_alu_of_phi);
>> +         if (!skip_alu_of_phi)
>> +            progress |= opt_split_alu_of_phi(b, loop);
>>            break;
>>         }
>>
>> @@ -1417,7 +1419,8 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>>   }
>>
>>   bool
>> -nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
>> +nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
>> +           bool skip_alu_of_phi)
>>   {
>>      bool progress = false;
>>
>> @@ -1430,7 +1433,7 @@ nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
>>
>>         nir_metadata_require(function->impl, nir_metadata_block_index |
>>                              nir_metadata_dominance);
>> -      progress = opt_if_safe_cf_list(&b, &function->impl->body);
>> +      progress = opt_if_safe_cf_list(&b, &function->impl->body, skip_alu_of_phi);
>>         nir_metadata_preserve(function->impl, nir_metadata_block_index |
>>                               nir_metadata_dominance);
>>
>> diff --git a/src/freedreno/ir3/ir3_nir.c b/src/freedreno/ir3/ir3_nir.c
>> index 76230e3be50..1bec3c030a9 100644
>> --- a/src/freedreno/ir3/ir3_nir.c
>> +++ b/src/freedreno/ir3/ir3_nir.c
>> @@ -147,7 +147,7 @@ ir3_optimize_loop(nir_shader *s)
>>                          OPT(s, nir_copy_prop);
>>                          OPT(s, nir_opt_dce);
>>                  }
>> -               progress |= OPT(s, nir_opt_if, false);
>> +               progress |= OPT(s, nir_opt_if, false, false);
>>                  progress |= OPT(s, nir_opt_remove_phis);
>>                  progress |= OPT(s, nir_opt_undef);
>>
>> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> index c55e8b84a41..6b40bff1f73 100644
>> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>> @@ -2066,7 +2066,7 @@ ttn_optimize_nir(nir_shader *nir, bool scalar)
>>            NIR_PASS(progress, nir, nir_opt_dce);
>>         }
>>
>> -      NIR_PASS(progress, nir, nir_opt_if, false);
>> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>>         NIR_PASS(progress, nir, nir_opt_dead_cf);
>>         NIR_PASS(progress, nir, nir_opt_cse);
>>         NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);
>> diff --git a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>> index ee348ca6a93..3522971e435 100644
>> --- a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>> +++ b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>> @@ -94,7 +94,7 @@ ir2_optimize_loop(nir_shader *s)
>>                          OPT(s, nir_opt_dce);
>>                  }
>>                  progress |= OPT(s, nir_opt_loop_unroll, nir_var_all);
>> -               progress |= OPT(s, nir_opt_if, false);
>> +               progress |= OPT(s, nir_opt_if, false, false);
>>                  progress |= OPT(s, nir_opt_remove_phis);
>>                  progress |= OPT(s, nir_opt_undef);
>>
>> diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c b/src/gallium/drivers/radeonsi/si_shader_nir.c
>> index 5a925f19e09..7f1fe4ba2e9 100644
>> --- a/src/gallium/drivers/radeonsi/si_shader_nir.c
>> +++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
>> @@ -879,7 +879,7 @@ si_lower_nir(struct si_shader_selector* sel)
>>                          NIR_PASS(progress, sel->nir, nir_copy_prop);
>>                          NIR_PASS(progress, sel->nir, nir_opt_dce);
>>                  }
>> -               NIR_PASS(progress, sel->nir, nir_opt_if, true);
>> +               NIR_PASS(progress, sel->nir, nir_opt_if, true, false);
>>                  NIR_PASS(progress, sel->nir, nir_opt_dead_cf);
>>                  NIR_PASS(progress, sel->nir, nir_opt_cse);
>>                  NIR_PASS(progress, sel->nir, nir_opt_peephole_select, 8, true, true);
>> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
>> index e0a393fc298..ba911049ce3 100644
>> --- a/src/intel/compiler/brw_nir.c
>> +++ b/src/intel/compiler/brw_nir.c
>> @@ -607,7 +607,7 @@ brw_nir_optimize(nir_shader *nir, const struct brw_compiler *compiler,
>>            OPT(nir_copy_prop);
>>            OPT(nir_opt_dce);
>>         }
>> -      OPT(nir_opt_if, false);
>> +      OPT(nir_opt_if, false, false);
>>         if (nir->options->max_unroll_iterations != 0) {
>>            OPT(nir_opt_loop_unroll, indirect_mask);
>>         }
>> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> index 97b2831b880..3f1f78e875b 100644
>> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> @@ -324,7 +324,7 @@ st_nir_opts(nir_shader *nir, bool scalar)
>>            NIR_PASS(progress, nir, nir_copy_prop);
>>            NIR_PASS(progress, nir, nir_opt_dce);
>>         }
>> -      NIR_PASS(progress, nir, nir_opt_if, false);
>> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>>         NIR_PASS(progress, nir, nir_opt_dead_cf);
>>         NIR_PASS(progress, nir, nir_opt_cse);
>>         NIR_PASS(progress, nir, nir_opt_peephole_select, 8, true, true);
>> --
>> 2.21.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


On 24/4/19 1:45 am, Samuel Pitoiset wrote:
> 
> On 4/23/19 5:16 PM, Jason Ekstrand wrote:
>> On Tue, Apr 23, 2019 at 7:46 AM Samuel Pitoiset 
>> <samuel.pitoiset@gmail.com <mailto:samuel.pitoiset@gmail.com>> wrote:
>>
>>
>>     On 4/23/19 10:45 AM, Bas Nieuwenhuizen wrote:
>>     > On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset
>>     > <samuel.pitoiset@gmail.com <mailto:samuel.pitoiset@gmail.com>>
>>     wrote:
>>     >> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com
>>     <mailto:samuel.pitoiset@gmail.com>>
>>     >> ---
>>     >>   src/amd/vulkan/radv_shader.c                 |  2 +-
>>     >>   src/compiler/nir/nir.h                       |  3 ++-
>>     >>   src/compiler/nir/nir_opt_if.c                | 17
>>     ++++++++++-------
>>     >>   src/freedreno/ir3/ir3_nir.c                  |  2 +-
>>     >>   src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
>>     >>   src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
>>     >>   src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
>>     >>   src/intel/compiler/brw_nir.c                 |  2 +-
>>     >>   src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
>>     >>   9 files changed, 19 insertions(+), 15 deletions(-)
>>     >>
>>     >> diff --git a/src/amd/vulkan/radv_shader.c
>>     b/src/amd/vulkan/radv_shader.c
>>     >> index 13f1f9aa9dc..54a4e732230 100644
>>     >> --- a/src/amd/vulkan/radv_shader.c
>>     >> +++ b/src/amd/vulkan/radv_shader.c
>>     >> @@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader
>>     *shader, bool optimize_conservatively,
>>     >>                          NIR_PASS(progress, shader,
>>     nir_opt_remove_phis);
>>     >>                           NIR_PASS(progress, shader, nir_opt_dce);
>>     >>                   }
>>     >> -                NIR_PASS(progress, shader, nir_opt_if, true);
>>     >> +                NIR_PASS(progress, shader, nir_opt_if, true,
>>     false);
>>     >>                   NIR_PASS(progress, shader, nir_opt_dead_cf);
>>     >>                   NIR_PASS(progress, shader, nir_opt_cse);
>>     >>                   NIR_PASS(progress, shader,
>>     nir_opt_peephole_select, 8, true, true);
>>     >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>>     >> index 7d2062d3691..d7506d6ddd1 100644
>>     >> --- a/src/compiler/nir/nir.h
>>     >> +++ b/src/compiler/nir/nir.h
>>     >> @@ -3474,7 +3474,8 @@ bool nir_opt_gcm(nir_shader *shader, bool
>>     value_number);
>>     >>
>>     >>   bool nir_opt_idiv_const(nir_shader *shader, unsigned
>>     min_bit_size);
>>     >>
>>     >> -bool nir_opt_if(nir_shader *shader, bool
>>     aggressive_last_continue);
>>     >> +bool nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
>>     >> +                bool skip_alu_of_phi);
>>     > Can we have a flag for this instead (e.g. something like
>>     > nir_opt_if_skip_alu_of_phi)? I think have a function with a bunch of
>>     > bools is less than ideal as you can't see at the calling site
>>     what is
>>     > for what arg.
>>     Yes, that seems better to me.
>>
>>
>> This is the worst kind of hack all around.  We're making NIR more 
>> complicated and adding a flag to disable a useful and correct piece of 
>> an optimization, not because it causes a perf regression but because 
>> the back-end compiler is broken and this is easier than fixing it 
>> properly. Seriously?  Can't we just fix the LLVM back-end?  Or, if 
>> this optimization is actually doing something wrong, fix it?  Or maybe 
>> actually figure out what pattern is causing LLVM to fall over and have 
>> a hack in your NIR -> LLVM pass?  On the list of "good ways to fix 
>> this problem", this seems to be pretty far down if it hasn't fallen 
>> off the bottom.
> 
> Best hack of the month? :-)
> 
> As discussed over IRC, this is definitely not the best solution, I don't 
> like it either as I said.
> 
> I will work on a different solution maybe in our NIR->LLVM pass.
> 
> The aggressive_last_continue option should also be removed (make it 
> default?).

The aggressive_last_continue option is not a hack to avoid a bug in a 
driver backend it is to avoid perf regressions. That said we may be able 
to remove it now that Jason has landed 
cd4ffb376f2aeefdd6a1b80d69a1580c4e569778

> 
>>
>> --Jason
>>
>>     >>   bool nir_opt_intrinsics(nir_shader *shader);
>>     >>
>>     >> diff --git a/src/compiler/nir/nir_opt_if.c
>>     b/src/compiler/nir/nir_opt_if.c
>>     >> index f674185f1e2..149b3bd1659 100644
>>     >> --- a/src/compiler/nir/nir_opt_if.c
>>     >> +++ b/src/compiler/nir/nir_opt_if.c
>>     >> @@ -1385,7 +1385,8 @@ opt_if_cf_list(nir_builder *b, struct
>>     exec_list *cf_list,
>>     >>    * not do anything to cause the metadata to become invalid.
>>     >>    */
>>     >>   static bool
>>     >> -opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list)
>>     >> +opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list,
>>     >> +                    bool skip_alu_of_phi)
>>     >>   {
>>     >>      bool progress = false;
>>     >>      foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
>>     >> @@ -1395,16 +1396,17 @@ opt_if_safe_cf_list(nir_builder *b,
>>     struct exec_list *cf_list)
>>     >>
>>     >>         case nir_cf_node_if: {
>>     >>            nir_if *nif = nir_cf_node_as_if(cf_node);
>>     >> -         progress |= opt_if_safe_cf_list(b, &nif->then_list);
>>     >> -         progress |= opt_if_safe_cf_list(b, &nif->else_list);
>>     >> +         progress |= opt_if_safe_cf_list(b, &nif->then_list,
>>     skip_alu_of_phi);
>>     >> +         progress |= opt_if_safe_cf_list(b, &nif->else_list,
>>     skip_alu_of_phi);
>>     >>            progress |= opt_if_evaluate_condition_use(b, nif);
>>     >>            break;
>>     >>         }
>>     >>
>>     >>         case nir_cf_node_loop: {
>>     >>            nir_loop *loop = nir_cf_node_as_loop(cf_node);
>>     >> -         progress |= opt_if_safe_cf_list(b, &loop->body);
>>     >> -         progress |= opt_split_alu_of_phi(b, loop);
>>     >> +         progress |= opt_if_safe_cf_list(b, &loop->body,
>>     skip_alu_of_phi);
>>     >> +         if (!skip_alu_of_phi)
>>     >> +            progress |= opt_split_alu_of_phi(b, loop);
>>     >>            break;
>>     >>         }
>>     >>
>>     >> @@ -1417,7 +1419,8 @@ opt_if_safe_cf_list(nir_builder *b,
>>     struct exec_list *cf_list)
>>     >>   }
>>     >>
>>     >>   bool
>>     >> -nir_opt_if(nir_shader *shader, bool aggressive_last_continue)
>>     >> +nir_opt_if(nir_shader *shader, bool aggressive_last_continue,
>>     >> +           bool skip_alu_of_phi)
>>     >>   {
>>     >>      bool progress = false;
>>     >>
>>     >> @@ -1430,7 +1433,7 @@ nir_opt_if(nir_shader *shader, bool
>>     aggressive_last_continue)
>>     >>
>>     >>         nir_metadata_require(function->impl,
>>     nir_metadata_block_index |
>>     >> nir_metadata_dominance);
>>     >> -      progress = opt_if_safe_cf_list(&b, &function->impl->body);
>>     >> +      progress = opt_if_safe_cf_list(&b,
>>     &function->impl->body, skip_alu_of_phi);
>>     >>         nir_metadata_preserve(function->impl,
>>     nir_metadata_block_index |
>>     >>  nir_metadata_dominance);
>>     >>
>>     >> diff --git a/src/freedreno/ir3/ir3_nir.c
>>     b/src/freedreno/ir3/ir3_nir.c
>>     >> index 76230e3be50..1bec3c030a9 100644
>>     >> --- a/src/freedreno/ir3/ir3_nir.c
>>     >> +++ b/src/freedreno/ir3/ir3_nir.c
>>     >> @@ -147,7 +147,7 @@ ir3_optimize_loop(nir_shader *s)
>>     >>                          OPT(s, nir_copy_prop);
>>     >>                          OPT(s, nir_opt_dce);
>>     >>                  }
>>     >> -               progress |= OPT(s, nir_opt_if, false);
>>     >> +               progress |= OPT(s, nir_opt_if, false, false);
>>     >>                  progress |= OPT(s, nir_opt_remove_phis);
>>     >>                  progress |= OPT(s, nir_opt_undef);
>>     >>
>>     >> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>     b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>     >> index c55e8b84a41..6b40bff1f73 100644
>>     >> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>     >> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>>     >> @@ -2066,7 +2066,7 @@ ttn_optimize_nir(nir_shader *nir, bool
>>     scalar)
>>     >>            NIR_PASS(progress, nir, nir_opt_dce);
>>     >>         }
>>     >>
>>     >> -      NIR_PASS(progress, nir, nir_opt_if, false);
>>     >> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>>     >>         NIR_PASS(progress, nir, nir_opt_dead_cf);
>>     >>         NIR_PASS(progress, nir, nir_opt_cse);
>>     >>         NIR_PASS(progress, nir, nir_opt_peephole_select, 8,
>>     true, true);
>>     >> diff --git a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>>     b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>>     >> index ee348ca6a93..3522971e435 100644
>>     >> --- a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>>     >> +++ b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>>     >> @@ -94,7 +94,7 @@ ir2_optimize_loop(nir_shader *s)
>>     >>                          OPT(s, nir_opt_dce);
>>     >>                  }
>>     >>                  progress |= OPT(s, nir_opt_loop_unroll,
>>     nir_var_all);
>>     >> -               progress |= OPT(s, nir_opt_if, false);
>>     >> +               progress |= OPT(s, nir_opt_if, false, false);
>>     >>                  progress |= OPT(s, nir_opt_remove_phis);
>>     >>                  progress |= OPT(s, nir_opt_undef);
>>     >>
>>     >> diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c
>>     b/src/gallium/drivers/radeonsi/si_shader_nir.c
>>     >> index 5a925f19e09..7f1fe4ba2e9 100644
>>     >> --- a/src/gallium/drivers/radeonsi/si_shader_nir.c
>>     >> +++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
>>     >> @@ -879,7 +879,7 @@ si_lower_nir(struct si_shader_selector* sel)
>>     >>                          NIR_PASS(progress, sel->nir,
>>     nir_copy_prop);
>>     >>                          NIR_PASS(progress, sel->nir, nir_opt_dce);
>>     >>                  }
>>     >> -               NIR_PASS(progress, sel->nir, nir_opt_if, true);
>>     >> +               NIR_PASS(progress, sel->nir, nir_opt_if, true,
>>     false);
>>     >>                  NIR_PASS(progress, sel->nir, nir_opt_dead_cf);
>>     >>                  NIR_PASS(progress, sel->nir, nir_opt_cse);
>>     >>                  NIR_PASS(progress, sel->nir,
>>     nir_opt_peephole_select, 8, true, true);
>>     >> diff --git a/src/intel/compiler/brw_nir.c
>>     b/src/intel/compiler/brw_nir.c
>>     >> index e0a393fc298..ba911049ce3 100644
>>     >> --- a/src/intel/compiler/brw_nir.c
>>     >> +++ b/src/intel/compiler/brw_nir.c
>>     >> @@ -607,7 +607,7 @@ brw_nir_optimize(nir_shader *nir, const
>>     struct brw_compiler *compiler,
>>     >>            OPT(nir_copy_prop);
>>     >>            OPT(nir_opt_dce);
>>     >>         }
>>     >> -      OPT(nir_opt_if, false);
>>     >> +      OPT(nir_opt_if, false, false);
>>     >>         if (nir->options->max_unroll_iterations != 0) {
>>     >>            OPT(nir_opt_loop_unroll, indirect_mask);
>>     >>         }
>>     >> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp
>>     b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>>     >> index 97b2831b880..3f1f78e875b 100644
>>     >> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
>>     >> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>>     >> @@ -324,7 +324,7 @@ st_nir_opts(nir_shader *nir, bool scalar)
>>     >>            NIR_PASS(progress, nir, nir_copy_prop);
>>     >>            NIR_PASS(progress, nir, nir_opt_dce);
>>     >>         }
>>     >> -      NIR_PASS(progress, nir, nir_opt_if, false);
>>     >> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>>     >>         NIR_PASS(progress, nir, nir_opt_dead_cf);
>>     >>         NIR_PASS(progress, nir, nir_opt_cse);
>>     >>         NIR_PASS(progress, nir, nir_opt_peephole_select, 8,
>>     true, true);
>>     >> --
>>     >> 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
>>     _______________________________________________
>>     mesa-dev mailing list
>>     mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
>>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

On 24/4/19 10:59 am, Jason Ekstrand wrote:
> On Tue, Apr 23, 2019 at 7:39 PM Timothy Arceri <tarceri@itsqueeze.com 
> <mailto:tarceri@itsqueeze.com>> wrote:
> 
>     On 24/4/19 1:45 am, Samuel Pitoiset wrote:
>      >
>      > On 4/23/19 5:16 PM, Jason Ekstrand wrote:
>      >> On Tue, Apr 23, 2019 at 7:46 AM Samuel Pitoiset
>      >> <samuel.pitoiset@gmail.com <mailto:samuel.pitoiset@gmail.com>
>     <mailto:samuel.pitoiset@gmail.com
>     <mailto:samuel.pitoiset@gmail.com>>> wrote:
>      >>
>      >>
>      >>     On 4/23/19 10:45 AM, Bas Nieuwenhuizen wrote:
>      >>     > On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset
>      >>     > <samuel.pitoiset@gmail.com
>     <mailto:samuel.pitoiset@gmail.com> <mailto:samuel.pitoiset@gmail.com
>     <mailto:samuel.pitoiset@gmail.com>>>
>      >>     wrote:
>      >>     >> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com
>     <mailto:samuel.pitoiset@gmail.com>
>      >>     <mailto:samuel.pitoiset@gmail.com
>     <mailto:samuel.pitoiset@gmail.com>>>
>      >>     >> ---
>      >>     >>   src/amd/vulkan/radv_shader.c                 |  2 +-
>      >>     >>   src/compiler/nir/nir.h                       |  3 ++-
>      >>     >>   src/compiler/nir/nir_opt_if.c                | 17
>      >>     ++++++++++-------
>      >>     >>   src/freedreno/ir3/ir3_nir.c                  |  2 +-
>      >>     >>   src/gallium/auxiliary/nir/tgsi_to_nir.c      |  2 +-
>      >>     >>   src/gallium/drivers/freedreno/a2xx/ir2_nir.c |  2 +-
>      >>     >>   src/gallium/drivers/radeonsi/si_shader_nir.c |  2 +-
>      >>     >>   src/intel/compiler/brw_nir.c                 |  2 +-
>      >>     >>   src/mesa/state_tracker/st_glsl_to_nir.cpp    |  2 +-
>      >>     >>   9 files changed, 19 insertions(+), 15 deletions(-)
>      >>     >>
>      >>     >> diff --git a/src/amd/vulkan/radv_shader.c
>      >>     b/src/amd/vulkan/radv_shader.c
>      >>     >> index 13f1f9aa9dc..54a4e732230 100644
>      >>     >> --- a/src/amd/vulkan/radv_shader.c
>      >>     >> +++ b/src/amd/vulkan/radv_shader.c
>      >>     >> @@ -158,7 +158,7 @@ radv_optimize_nir(struct nir_shader
>      >>     *shader, bool optimize_conservatively,
>      >>     >>                          NIR_PASS(progress, shader,
>      >>     nir_opt_remove_phis);
>      >>     >>                           NIR_PASS(progress, shader,
>     nir_opt_dce);
>      >>     >>                   }
>      >>     >> -                NIR_PASS(progress, shader, nir_opt_if,
>     true);
>      >>     >> +                NIR_PASS(progress, shader, nir_opt_if, true,
>      >>     false);
>      >>     >>                   NIR_PASS(progress, shader,
>     nir_opt_dead_cf);
>      >>     >>                   NIR_PASS(progress, shader, nir_opt_cse);
>      >>     >>                   NIR_PASS(progress, shader,
>      >>     nir_opt_peephole_select, 8, true, true);
>      >>     >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>      >>     >> index 7d2062d3691..d7506d6ddd1 100644
>      >>     >> --- a/src/compiler/nir/nir.h
>      >>     >> +++ b/src/compiler/nir/nir.h
>      >>     >> @@ -3474,7 +3474,8 @@ bool nir_opt_gcm(nir_shader
>     *shader, bool
>      >>     value_number);
>      >>     >>
>      >>     >>   bool nir_opt_idiv_const(nir_shader *shader, unsigned
>      >>     min_bit_size);
>      >>     >>
>      >>     >> -bool nir_opt_if(nir_shader *shader, bool
>      >>     aggressive_last_continue);
>      >>     >> +bool nir_opt_if(nir_shader *shader, bool
>     aggressive_last_continue,
>      >>     >> +                bool skip_alu_of_phi);
>      >>     > Can we have a flag for this instead (e.g. something like
>      >>     > nir_opt_if_skip_alu_of_phi)? I think have a function with
>     a bunch of
>      >>     > bools is less than ideal as you can't see at the calling site
>      >>     what is
>      >>     > for what arg.
>      >>     Yes, that seems better to me.
>      >>
>      >>
>      >> This is the worst kind of hack all around.  We're making NIR more
>      >> complicated and adding a flag to disable a useful and correct
>     piece of
>      >> an optimization, not because it causes a perf regression but
>     because
>      >> the back-end compiler is broken and this is easier than fixing it
>      >> properly. Seriously?  Can't we just fix the LLVM back-end?  Or, if
>      >> this optimization is actually doing something wrong, fix it?  Or
>     maybe
>      >> actually figure out what pattern is causing LLVM to fall over
>     and have
>      >> a hack in your NIR -> LLVM pass?  On the list of "good ways to fix
>      >> this problem", this seems to be pretty far down if it hasn't fallen
>      >> off the bottom.
>      >
>      > Best hack of the month? :-)
>      >
>      > As discussed over IRC, this is definitely not the best solution,
>     I don't
>      > like it either as I said.
>      >
>      > I will work on a different solution maybe in our NIR->LLVM pass.
>      >
>      > The aggressive_last_continue option should also be removed (make it
>      > default?).
> 
>     The aggressive_last_continue option is not a hack to avoid a bug in a
>     driver backend it is to avoid perf regressions. That said we may be
>     able
>     to remove it now that Jason has landed
>     cd4ffb376f2aeefdd6a1b80d69a1580c4e569778
> 
> 
> I don't think anyone was claiming it was.  The only claim made was that 
> it's probably an ultimately unnecessary option and we should try to 
> apply it universally.

My point was that its ok to have options (hacks?) for less aggressive 
optimisations to avoid perf regressions, that is different from hacks 
that mask bugs and shouldn't really be discussed in the same context as 
this series.

It definitely should be applied universally in the long run which is why 
I tried to get GCM in a working state before applying the continue 
optimisation. However is was clear that it wasn't going to land in time 
for 19.1 hence the aggressive_last_continue option.

The suggestion was to just remove the option which is what I was 
replying to. The code comment for the function already has a TODO for 
removing the option we just need to get to a point where we can avoid 
regressions.

> 
> --Jason
> 
>      >
>      >>
>      >> --Jason
>      >>
>      >>     >>   bool nir_opt_intrinsics(nir_shader *shader);
>      >>     >>
>      >>     >> diff --git a/src/compiler/nir/nir_opt_if.c
>      >>     b/src/compiler/nir/nir_opt_if.c
>      >>     >> index f674185f1e2..149b3bd1659 100644
>      >>     >> --- a/src/compiler/nir/nir_opt_if.c
>      >>     >> +++ b/src/compiler/nir/nir_opt_if.c
>      >>     >> @@ -1385,7 +1385,8 @@ opt_if_cf_list(nir_builder *b, struct
>      >>     exec_list *cf_list,
>      >>     >>    * not do anything to cause the metadata to become invalid.
>      >>     >>    */
>      >>     >>   static bool
>      >>     >> -opt_if_safe_cf_list(nir_builder *b, struct exec_list
>     *cf_list)
>      >>     >> +opt_if_safe_cf_list(nir_builder *b, struct exec_list
>     *cf_list,
>      >>     >> +                    bool skip_alu_of_phi)
>      >>     >>   {
>      >>     >>      bool progress = false;
>      >>     >>      foreach_list_typed(nir_cf_node, cf_node, node,
>     cf_list) {
>      >>     >> @@ -1395,16 +1396,17 @@ opt_if_safe_cf_list(nir_builder *b,
>      >>     struct exec_list *cf_list)
>      >>     >>
>      >>     >>         case nir_cf_node_if: {
>      >>     >>            nir_if *nif = nir_cf_node_as_if(cf_node);
>      >>     >> -         progress |= opt_if_safe_cf_list(b,
>     &nif->then_list);
>      >>     >> -         progress |= opt_if_safe_cf_list(b,
>     &nif->else_list);
>      >>     >> +         progress |= opt_if_safe_cf_list(b, &nif->then_list,
>      >>     skip_alu_of_phi);
>      >>     >> +         progress |= opt_if_safe_cf_list(b, &nif->else_list,
>      >>     skip_alu_of_phi);
>      >>     >>            progress |= opt_if_evaluate_condition_use(b, nif);
>      >>     >>            break;
>      >>     >>         }
>      >>     >>
>      >>     >>         case nir_cf_node_loop: {
>      >>     >>            nir_loop *loop = nir_cf_node_as_loop(cf_node);
>      >>     >> -         progress |= opt_if_safe_cf_list(b, &loop->body);
>      >>     >> -         progress |= opt_split_alu_of_phi(b, loop);
>      >>     >> +         progress |= opt_if_safe_cf_list(b, &loop->body,
>      >>     skip_alu_of_phi);
>      >>     >> +         if (!skip_alu_of_phi)
>      >>     >> +            progress |= opt_split_alu_of_phi(b, loop);
>      >>     >>            break;
>      >>     >>         }
>      >>     >>
>      >>     >> @@ -1417,7 +1419,8 @@ opt_if_safe_cf_list(nir_builder *b,
>      >>     struct exec_list *cf_list)
>      >>     >>   }
>      >>     >>
>      >>     >>   bool
>      >>     >> -nir_opt_if(nir_shader *shader, bool
>     aggressive_last_continue)
>      >>     >> +nir_opt_if(nir_shader *shader, bool
>     aggressive_last_continue,
>      >>     >> +           bool skip_alu_of_phi)
>      >>     >>   {
>      >>     >>      bool progress = false;
>      >>     >>
>      >>     >> @@ -1430,7 +1433,7 @@ nir_opt_if(nir_shader *shader, bool
>      >>     aggressive_last_continue)
>      >>     >>
>      >>     >>         nir_metadata_require(function->impl,
>      >>     nir_metadata_block_index |
>      >>     >> nir_metadata_dominance);
>      >>     >> -      progress = opt_if_safe_cf_list(&b,
>     &function->impl->body);
>      >>     >> +      progress = opt_if_safe_cf_list(&b,
>      >>     &function->impl->body, skip_alu_of_phi);
>      >>     >>         nir_metadata_preserve(function->impl,
>      >>     nir_metadata_block_index |
>      >>     >>  nir_metadata_dominance);
>      >>     >>
>      >>     >> diff --git a/src/freedreno/ir3/ir3_nir.c
>      >>     b/src/freedreno/ir3/ir3_nir.c
>      >>     >> index 76230e3be50..1bec3c030a9 100644
>      >>     >> --- a/src/freedreno/ir3/ir3_nir.c
>      >>     >> +++ b/src/freedreno/ir3/ir3_nir.c
>      >>     >> @@ -147,7 +147,7 @@ ir3_optimize_loop(nir_shader *s)
>      >>     >>                          OPT(s, nir_copy_prop);
>      >>     >>                          OPT(s, nir_opt_dce);
>      >>     >>                  }
>      >>     >> -               progress |= OPT(s, nir_opt_if, false);
>      >>     >> +               progress |= OPT(s, nir_opt_if, false, false);
>      >>     >>                  progress |= OPT(s, nir_opt_remove_phis);
>      >>     >>                  progress |= OPT(s, nir_opt_undef);
>      >>     >>
>      >>     >> diff --git a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>      >>     b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>      >>     >> index c55e8b84a41..6b40bff1f73 100644
>      >>     >> --- a/src/gallium/auxiliary/nir/tgsi_to_nir.c
>      >>     >> +++ b/src/gallium/auxiliary/nir/tgsi_to_nir.c
>      >>     >> @@ -2066,7 +2066,7 @@ ttn_optimize_nir(nir_shader *nir, bool
>      >>     scalar)
>      >>     >>            NIR_PASS(progress, nir, nir_opt_dce);
>      >>     >>         }
>      >>     >>
>      >>     >> -      NIR_PASS(progress, nir, nir_opt_if, false);
>      >>     >> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>      >>     >>         NIR_PASS(progress, nir, nir_opt_dead_cf);
>      >>     >>         NIR_PASS(progress, nir, nir_opt_cse);
>      >>     >>         NIR_PASS(progress, nir, nir_opt_peephole_select, 8,
>      >>     true, true);
>      >>     >> diff --git a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>      >>     b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>      >>     >> index ee348ca6a93..3522971e435 100644
>      >>     >> --- a/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>      >>     >> +++ b/src/gallium/drivers/freedreno/a2xx/ir2_nir.c
>      >>     >> @@ -94,7 +94,7 @@ ir2_optimize_loop(nir_shader *s)
>      >>     >>                          OPT(s, nir_opt_dce);
>      >>     >>                  }
>      >>     >>                  progress |= OPT(s, nir_opt_loop_unroll,
>      >>     nir_var_all);
>      >>     >> -               progress |= OPT(s, nir_opt_if, false);
>      >>     >> +               progress |= OPT(s, nir_opt_if, false, false);
>      >>     >>                  progress |= OPT(s, nir_opt_remove_phis);
>      >>     >>                  progress |= OPT(s, nir_opt_undef);
>      >>     >>
>      >>     >> diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c
>      >>     b/src/gallium/drivers/radeonsi/si_shader_nir.c
>      >>     >> index 5a925f19e09..7f1fe4ba2e9 100644
>      >>     >> --- a/src/gallium/drivers/radeonsi/si_shader_nir.c
>      >>     >> +++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
>      >>     >> @@ -879,7 +879,7 @@ si_lower_nir(struct
>     si_shader_selector* sel)
>      >>     >>                          NIR_PASS(progress, sel->nir,
>      >>     nir_copy_prop);
>      >>     >>                          NIR_PASS(progress, sel->nir,
>     nir_opt_dce);
>      >>     >>                  }
>      >>     >> -               NIR_PASS(progress, sel->nir, nir_opt_if,
>     true);
>      >>     >> +               NIR_PASS(progress, sel->nir, nir_opt_if,
>     true,
>      >>     false);
>      >>     >>                  NIR_PASS(progress, sel->nir,
>     nir_opt_dead_cf);
>      >>     >>                  NIR_PASS(progress, sel->nir, nir_opt_cse);
>      >>     >>                  NIR_PASS(progress, sel->nir,
>      >>     nir_opt_peephole_select, 8, true, true);
>      >>     >> diff --git a/src/intel/compiler/brw_nir.c
>      >>     b/src/intel/compiler/brw_nir.c
>      >>     >> index e0a393fc298..ba911049ce3 100644
>      >>     >> --- a/src/intel/compiler/brw_nir.c
>      >>     >> +++ b/src/intel/compiler/brw_nir.c
>      >>     >> @@ -607,7 +607,7 @@ brw_nir_optimize(nir_shader *nir, const
>      >>     struct brw_compiler *compiler,
>      >>     >>            OPT(nir_copy_prop);
>      >>     >>            OPT(nir_opt_dce);
>      >>     >>         }
>      >>     >> -      OPT(nir_opt_if, false);
>      >>     >> +      OPT(nir_opt_if, false, false);
>      >>     >>         if (nir->options->max_unroll_iterations != 0) {
>      >>     >>            OPT(nir_opt_loop_unroll, indirect_mask);
>      >>     >>         }
>      >>     >> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp
>      >>     b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>      >>     >> index 97b2831b880..3f1f78e875b 100644
>      >>     >> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
>      >>     >> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>      >>     >> @@ -324,7 +324,7 @@ st_nir_opts(nir_shader *nir, bool scalar)
>      >>     >>            NIR_PASS(progress, nir, nir_copy_prop);
>      >>     >>            NIR_PASS(progress, nir, nir_opt_dce);
>      >>     >>         }
>      >>     >> -      NIR_PASS(progress, nir, nir_opt_if, false);
>      >>     >> +      NIR_PASS(progress, nir, nir_opt_if, false, false);
>      >>     >>         NIR_PASS(progress, nir, nir_opt_dead_cf);
>      >>     >>         NIR_PASS(progress, nir, nir_opt_cse);
>      >>     >>         NIR_PASS(progress, nir, nir_opt_peephole_select, 8,
>      >>     true, true);
>      >>     >> --
>      >>     >> 2.21.0
>      >>     >>
>      >>     >> _______________________________________________
>      >>     >> mesa-dev mailing list
>      >>     >> mesa-dev@lists.freedesktop.org
>     <mailto:mesa-dev@lists.freedesktop.org>
>      >>     <mailto:mesa-dev@lists.freedesktop.org
>     <mailto:mesa-dev@lists.freedesktop.org>>
>      >>     >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>      >>     _______________________________________________
>      >>     mesa-dev mailing list
>      >> mesa-dev@lists.freedesktop.org
>     <mailto:mesa-dev@lists.freedesktop.org>
>     <mailto:mesa-dev@lists.freedesktop.org
>     <mailto:mesa-dev@lists.freedesktop.org>>
>      >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>      >>
>      >
>      > _______________________________________________
>      > mesa-dev mailing list
>      > mesa-dev@lists.freedesktop.org
>     <mailto:mesa-dev@lists.freedesktop.org>
>      > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>      >
>