[Mesa-dev] [PATCH 2/2] nir: add opt_if_loop_terminator()

[Ian Romanick]

On 06/03/2018 11:30 PM, Timothy Arceri wrote:
> On 02/06/18 04:34, Ian Romanick wrote:
> 
>> On 05/31/2018 10:37 PM, Timothy Arceri wrote:
>>> This pass detects potential loop terminators and moves intructions
>>> from the non breaking branch after the if.
>>>
>>> This enables both the new opt_if_simplification() pass and loop
>>> unrolling to potentially progress further.
>>>
>>> Unexpectedly this change speed up shader-db run times by ~3%
>>>
>>> Ivy Bridge shader-db results (all changes in dolphin/ubershaders):
>> I was going to look at the changes in the generated code, but the
>> smallest of these shaders is > 2300 instructions.
>>
>> Do you have any idea if any piglit or CTS tests hit this optimization
>> path?
> 
> Ok I've sent a loop unrolling piglit test for this:
> 
> https://patchwork.freedesktop.org/patch/227292/

Cool.  This series is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Which also found a loop unrolling bug which requires the following mesa
> patch to pass:
> 
> https://patchwork.freedesktop.org/patch/227299/
> 
>>
>> One tiny nit below...
>>
>>> total instructions in shared programs: 9995662 -> 9995338 (-0.00%)
>>> instructions in affected programs: 87845 -> 87521 (-0.37%)
>>> helped: 27
>>> HURT: 0
>>>
>>> total cycles in shared programs: 230931495 -> 230925015 (-0.00%)
>>> cycles in affected programs: 56391385 -> 56384905 (-0.01%)
>>> helped: 27
>>> HURT: 0
>>> ---
>>>   src/compiler/nir/nir_opt_if.c | 68 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 68 insertions(+)
>>>
>>> diff --git a/src/compiler/nir/nir_opt_if.c
>>> b/src/compiler/nir/nir_opt_if.c
>>> index b03657a4244..1edeefdd5d1 100644
>>> --- a/src/compiler/nir/nir_opt_if.c
>>> +++ b/src/compiler/nir/nir_opt_if.c
>>> @@ -24,6 +24,7 @@
>>>   #include "nir.h"
>>>   #include "nir/nir_builder.h"
>>>   #include "nir_control_flow.h"
>>> +#include "nir_loop_analyze.h"
>>>     /**
>>>    * This optimization detects if statements at the tops of loops
>>> where the
>>> @@ -283,6 +284,72 @@ opt_if_simplification(nir_builder *b, nir_if *nif)
>>>      return true;
>>>   }
>>>   +/**
>>> + * This optimization simplifies potential loop terminators which
>>> then allows
>>> + * other passes such as opt_if_simplification() and loop unrolling
>>> to progress
>>> + * further:
>>> + *
>>> + *     if (cond) {
>>> + *        ... then block instructions ...
>>> + *     } else {
>>> + *         ...
>>> + *        break;
>>> + *     }
>>> + *
>>> + * into:
>>> + *
>>> + *     if (cond) {
>>> + *     } else {
>>> + *         ...
>>> + *        break;
>>> + *     }
>>> + *     ... then block instructions ...
>>> + */
>>> +static bool
>>> +opt_if_loop_terminator(nir_if *nif)
>>> +{
>>> +   nir_block *break_blk = NULL;
>>> +   nir_block *continue_from_blk = NULL;
>>> +   bool continue_from_then = true;
>>> +
>>> +   nir_block *last_then = nir_if_last_then_block(nif);
>>> +   nir_block *last_else = nir_if_last_else_block(nif);
>>> +
>>> +   if (nir_block_ends_in_break(last_then)) {
>>> +      break_blk = last_then;
>>> +      continue_from_blk = last_else;
>>> +      continue_from_then = false;
>>> +   } else if (nir_block_ends_in_break(last_else)) {
>>> +      break_blk = last_else;
>>> +      continue_from_blk = last_then;
>>> +   }
>>> +
>>> +   /* Continue if the if contained no jumps at all */
>> In prose, I like to use if-statement because I think it reads more
>> easily.  "Continue if the if-statement..."
>>
>>> +   if (!break_blk)
>>> +      return false;
>>> +
>>> +   /* If the continue from block is empty then return as there is
>>> nothing to
>>> +    * move.
>>> +    */
>>> +   nir_block *first_continue_from_blk = continue_from_then ?
>>> +      nir_if_first_then_block(nif) :
>>> +      nir_if_first_else_block(nif);
>>> +   if (is_block_empty(first_continue_from_blk))
>>> +      return false;
>>> +
>>> +   if (!nir_is_trivial_loop_if(nif, break_blk))
>>> +      return false;
>>> +
>>> +   /* Finally, move the continue from branch after the if. */
>>> +   nir_cf_list tmp;
>>> +   nir_cf_extract(&tmp, nir_before_block(first_continue_from_blk),
>>> +                        nir_after_block(continue_from_blk));
>>> +   nir_cf_reinsert(&tmp, nir_after_cf_node(&nif->cf_node));
>>> +   nir_cf_delete(&tmp);
>>> +
>>> +   return true;
>>> +}
>>> +
>>>   static bool
>>>   opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
>>>   {
>>> @@ -296,6 +363,7 @@ opt_if_cf_list(nir_builder *b, struct exec_list
>>> *cf_list)
>>>            nir_if *nif = nir_cf_node_as_if(cf_node);
>>>            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_simplification(b, nif);
>>>            break;
>>>         }
>>>