[Mesa-dev] [PATCH 2/2] nir: merge some basic consecutive ifs
Timothy Arceri
tarceri at itsqueeze.com
Tue Nov 27 22:21:49 UTC 2018
On Tue, Nov 27, 2018, at 10:27 AM, Ian Romanick wrote:
> On 11/26/2018 09:31 PM, Timothy Arceri wrote:
> > After trying multiple times to merge if-statements with phis
> > between them I've come to the conclusion that it cannot be done
> > without regressions. The problem is for some shaders we end up
> > with a whole bunch of phis for the merged ifs resulting in
> > increased register pressure.
> >
> > So this patch just merges ifs that have no phis between them.
> > This seems to be consistent with what LLVM does so for radeonsi
> > we only see a change (although its a large change) in a single
> > shader.
> >
> > Shader-db results i965 (SKL):
> >
> > total instructions in shared programs: 13098176 -> 13098152 (<.01%)
> > instructions in affected programs: 1326 -> 1302 (-1.81%)
> > helped: 4
> > HURT: 0
> >
> > total cycles in shared programs: 332032989 -> 332037583 (<.01%)
> > cycles in affected programs: 60665 -> 65259 (7.57%)
> > helped: 0
> > HURT: 4
> >
> > The cycles estimates reported by shader-db for i965 seem inaccurate
> > as the only difference in the final code is the removal of the
> > redundent condition evaluations and jumps.
>
> The scheduling doesn't even change?
Nope. But I've attached two full dumps from one of the shaders for comparision if you are interested in taking a look.
> Whenever I've encountered things
> like this, the scheduler has been to blame. It seems surprising that
> only 4 shaders are affected. Are these all different instances of the
> same shader? :)
Two shaders it seems:
instructions helped: shaders/closed/steam/deus-ex-mankind-divided/530.shader_test FS SIMD8: 337 -> 331 (-1.78%)
instructions helped: shaders/closed/steam/deus-ex-mankind-divided/530.shader_test FS SIMD16: 337 -> 331 (-1.78%)
instructions helped: shaders/closed/steam/deus-ex-mankind-divided/2354.shader_test FS SIMD8: 326 -> 320 (-1.84%)
instructions helped: shaders/closed/steam/deus-ex-mankind-divided/2354.shader_test FS SIMD16: 326 -> 320 (-1.84%)
cycles HURT: shaders/closed/steam/deus-ex-mankind-divided/530.shader_test FS SIMD16: 15850 -> 16990 (7.19%)
cycles HURT: shaders/closed/steam/deus-ex-mankind-divided/2354.shader_test FS SIMD16: 15698 -> 16840 (7.27%)
cycles HURT: shaders/closed/steam/deus-ex-mankind-divided/530.shader_test FS SIMD8: 14652 -> 15808 (7.89%)
cycles HURT: shaders/closed/steam/deus-ex-mankind-divided/2354.shader_test FS SIMD8: 14465 -> 15621 (7.99%)
>
> > Also the biggest code reduction (~7%) for radeonsi was in a tomb
> > raider tressfx shader but for some reason this does not get merged
> > for i965.
> >
> > Shader-db results radeonsi (VEGA):
> >
> > Totals from affected shaders:
> > SGPRS: 232 -> 232 (0.00 %)
> > VGPRS: 164 -> 164 (0.00 %)
> > Spilled SGPRs: 59 -> 59 (0.00 %)
> > Spilled VGPRs: 0 -> 0 (0.00 %)
> > Private memory VGPRs: 0 -> 0 (0.00 %)
> > Scratch size: 0 -> 0 (0.00 %) dwords per thread
> > Code Size: 14584 -> 13520 (-7.30 %) bytes
> > LDS: 0 -> 0 (0.00 %) blocks
> > Max Waves: 13 -> 13 (0.00 %)
> > Wait states: 0 -> 0 (0.00 %)
> > ---
> > src/compiler/nir/nir_opt_if.c | 93 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 93 insertions(+)
> >
> > diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> > index 62566eb403..dd488b1787 100644
> > --- a/src/compiler/nir/nir_opt_if.c
> > +++ b/src/compiler/nir/nir_opt_if.c
> > @@ -585,6 +585,98 @@ opt_if_evaluate_condition_use(nir_builder *b, nir_if *nif)
> > return progress;
> > }
> >
> > +static void
> > +simple_merge_if(nir_if *dest_if, nir_if *src_if, bool dest_if_then,
> > + bool src_if_then)
> > +{
> > + /* Now merge the if branch */
> > + nir_block *dest_blk = dest_if_then ? nir_if_last_then_block(dest_if)
> > + : nir_if_last_else_block(dest_if);
> > +
> > + struct exec_list *list = src_if_then ? &src_if->then_list
> > + : &src_if->else_list;
> > +
> > + nir_cf_list if_cf_list;
> > + nir_cf_extract(&if_cf_list, nir_before_cf_list(list),
> > + nir_after_cf_list(list));
> > + nir_cf_reinsert(&if_cf_list, nir_after_block(dest_blk));
> > +}
> > +
> > +static bool
> > +opt_if_merge(nir_if *nif)
> > +{
> > + bool progress = false;
> > +
> > + nir_block *next_blk = nir_cf_node_cf_tree_next(&nif->cf_node);
> > + if (next_blk && nif->condition.is_ssa) {
> > + nir_if *next_if = nir_block_get_following_if(next_blk);
> > + if (next_if && next_if->condition.is_ssa) {
> > +
> > + /* Here we merge two consecutive ifs that have the same
> > + * condition e.g:
> > + *
> > + * if ssa_12 {
> > + * ...
> > + * } else {
> > + * ...
> > + * }
> > + * if ssa_12 {
> > + * ...
> > + * } else {
> > + * ...
> > + * }
> > + *
> > + * Note: This only merges if-statements when the block between them
> > + * is empty. The reason we don't try to merge ifs that just have phis
> > + * between them is because this can results in increased register
> > + * pressure. For example when merging if ladders created by indirect
> > + * indexing.
> > + */
> > + if (nif->condition.ssa == next_if->condition.ssa &&
> > + exec_list_is_empty(&next_blk->instr_list)) {
> > +
> > + simple_merge_if(nif, next_if, true, true);
> > + simple_merge_if(nif, next_if, false, false);
> > +
> > + nir_block *new_then_block = nir_if_last_then_block(nif);
> > + nir_block *new_else_block = nir_if_last_else_block(nif);
> > +
> > + nir_block *old_then_block = nir_if_last_then_block(next_if);
> > + nir_block *old_else_block = nir_if_last_else_block(next_if);
> > +
> > + /* Rewrite the predecessor block for any phis following the second
> > + * if-statement.
> > + */
> > + rewrite_phi_predecessor_blocks(next_if, old_then_block,
> > + old_else_block,
> > + new_then_block,
> > + new_else_block);
> > +
> > + /* Move phis after merged if to avoid them being deleted when we
> > + * remove the merged if-statement.
> > + */
> > + nir_block *after_next_if_block =
> > + nir_cf_node_as_block(nir_cf_node_next(&next_if->cf_node));
> > +
> > + nir_foreach_instr_safe(instr, after_next_if_block) {
> > + if (instr->type != nir_instr_type_phi)
> > + break;
> > +
> > + exec_node_remove(&instr->node);
> > + exec_list_push_tail(&next_blk->instr_list, &instr->node);
> > + instr->block = next_blk;
> > + }
> > +
> > + nir_cf_node_remove(&next_if->cf_node);
> > +
> > + progress = true;
> > + }
> > + }
> > + }
> > +
> > + return progress;
> > +}
> > +
> > static bool
> > opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
> > {
> > @@ -599,6 +691,7 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
> > progress |= opt_if_cf_list(b, &nif->then_list);
> > progress |= opt_if_cf_list(b, &nif->else_list);
> > progress |= opt_if_loop_terminator(nif);
> > + progress |= opt_if_merge(nif);
> > progress |= opt_if_simplification(b, nif);
> > break;
> > }
> >
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dump4.txt
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181127/bb9c0edc/attachment-0002.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dump3.txt
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181127/bb9c0edc/attachment-0003.txt>
More information about the mesa-dev
mailing list