[Mesa-dev] [PATCH 06/12] nir/opt_peephole_select: Don't try to remove flow control around indirect loads

[Rob Clark]

On Thu, Jun 28, 2018 at 12:46 AM, Ian Romanick <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> That flow control may be trying to avoid invalid loads.  On at least
> some platforms, those loads can also be expensive.

So for adreno, indirect uniform loads aren't really that expensive (it
takes a few cycles to get the value from an alu instruction into the
address register for indirect register addressing but that latency can
usually be hidden).. and invalid access won't cause a fault or
anything.  I think it just wraps around, so it will be some undefined
value but won't cause a fault.

This is completely different from UBO's were we could cause a fault,
and loads are real memory access, so more expensive.

So as long as this is just about uniforms (I think i965 calls them
'push constants'?), and not UBOs, then maybe we want
nir_shader_compiler_options flag?

BR,
-R

>
> No shader-db changes on any Intel platform (even with the later patch
> "intel/compiler: More peephole select").
>
> NOTE: I've tried to CC everyone whose drive might be affected by this
> change.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Cc: Eric Anholt <eric at anholt.net>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Marek Olšák <marek.olsak at amd.com>
> ---
>  src/compiler/nir/nir_opt_peephole_select.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_peephole_select.c b/src/compiler/nir/nir_opt_peephole_select.c
> index 4ca4f80d788..920ced2137c 100644
> --- a/src/compiler/nir/nir_opt_peephole_select.c
> +++ b/src/compiler/nir/nir_opt_peephole_select.c
> @@ -58,7 +58,8 @@
>   */
>
>  static bool
> -block_check_for_allowed_instrs(nir_block *block, unsigned *count, bool alu_ok)
> +block_check_for_allowed_instrs(nir_block *block, unsigned *count,
> +                               gl_shader_stage stage, bool alu_ok)
>  {
>     nir_foreach_instr(instr, block) {
>        switch (instr->type) {
> @@ -70,6 +71,13 @@ block_check_for_allowed_instrs(nir_block *block, unsigned *count, bool alu_ok)
>              switch (intrin->variables[0]->var->data.mode) {
>              case nir_var_shader_in:
>              case nir_var_uniform:
> +               /* Don't try to remove flow control around an indirect load
> +                * because that flow control may be trying to avoid invalid
> +                * loads.
> +                */
> +               if (nir_deref_has_indirect(stage, intrin->variables[0]))
> +                  return false;
> +
>                 break;
>
>              default:
> @@ -168,8 +176,10 @@ nir_opt_peephole_select_block(nir_block *block, nir_shader *shader,
>
>     /* ... and those blocks must only contain "allowed" instructions. */
>     unsigned count = 0;
> -   if (!block_check_for_allowed_instrs(then_block, &count, limit != 0) ||
> -       !block_check_for_allowed_instrs(else_block, &count, limit != 0))
> +   if (!block_check_for_allowed_instrs(then_block, &count, shader->info.stage,
> +                                       limit != 0) ||
> +       !block_check_for_allowed_instrs(else_block, &count, shader->info.stage,
> +                                       limit != 0))
>        return false;
>
>     if (count > limit)
> --
> 2.14.4
>