[Mesa-dev] [PATCH 1/2] nir: add an option for skipping split_alu_of_phi
Timothy Arceri
tarceri at itsqueeze.com
Wed Apr 24 00:39:38 UTC 2019
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 at gmail.com <mailto:samuel.pitoiset at 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 at gmail.com <mailto:samuel.pitoiset at gmail.com>>
>> wrote:
>> >> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com
>> <mailto:samuel.pitoiset at 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 at lists.freedesktop.org
>> <mailto:mesa-dev at lists.freedesktop.org>
>> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list