<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 23, 2019 at 7:39 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 >> <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a> <mailto:<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a>>> wrote: >> >> >> On 4/23/19 10:45 AM, Bas Nieuwenhuizen wrote: >> > On Tue, Apr 23, 2019 at 9:35 AM Samuel Pitoiset >> > <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a> <mailto:<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a>>> >> wrote: >> >> Signed-off-by: Samuel Pitoiset <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a> >> <mailto:<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a>>> >> >> --- >> >> 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 </blockquote><div> </div><div>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.</div><div> </div><div>--Jason</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > >> >> --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 >> >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> >> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a>> >> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> _______________________________________________ >> mesa-dev mailing list >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a>> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > </blockquote></div></div>