[Mesa-dev] [PATCH 1/2] nir: add an option for skipping split_alu_of_phi
Timothy Arceri
tarceri at itsqueeze.com
Wed Apr 24 01:27:05 UTC 2019
On 24/4/19 10:59 am, Jason Ekstrand wrote:
> On Tue, Apr 23, 2019 at 7:39 PM Timothy Arceri <tarceri at itsqueeze.com
> <mailto:tarceri at 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 at gmail.com <mailto:samuel.pitoiset at gmail.com>
> <mailto: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> <mailto: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>
> >> <mailto: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
>
>
> 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 at lists.freedesktop.org
> <mailto:mesa-dev at lists.freedesktop.org>
> >> <mailto: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>
> <mailto: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
> >
>
More information about the mesa-dev
mailing list