[Mesa-dev] [PATCH] nir: Implement CSE on intrinsics that can be eliminated and reordered.

[Jason Ekstrand]

On Jan 21, 2015 3:29 AM, "Kenneth Graunke" <kenneth at whitecape.org> wrote:
>
> Matt and I noticed that one of the shaders hurt by INTEL_USE_NIR=1 had
> load_input and load_uniform intrinsics repeated several times, with the
> same parameters, but each one generating a distinct SSA value.  This
> made ALU operations on those values appear distinct as well.
>
> Generating distinct SSA values is silly - these are read only variables.
> CSE'ing them makes everything use a single SSA value, which then allows
> other operations to be CSE'd away as well.
>
> Generalizing a bit, it seems like we should be able to safely CSE any
> intrinsics that can be eliminated and reordered.  I didn't implement
> support for variables for the time being.

I think this is fine for now.  If it ever becomes a problem, we can add a
can_cse flag.  Couple comments below; otherwise,
Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

> --- NIR instruction statistics ---
> total instructions in shared programs: 2435936 -> 2023511 (-16.93%)
> instructions in affected programs:     2413496 -> 2001071 (-17.09%)
> helped:                                16872
>
> --- i965 instruction statistics ---
> total instructions in shared programs: 6028987 -> 6008427 (-0.34%)
> instructions in affected programs:     640654 -> 620094 (-3.21%)
> helped:                                2071
> HURT:                                  585
> GAINED:                                14
> LOST:                                  25
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/nir/nir_opt_cse.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/nir/nir_opt_cse.c b/src/glsl/nir/nir_opt_cse.c
> index e7dba1d..6ad78f6 100644
> --- a/src/glsl/nir/nir_opt_cse.c
> +++ b/src/glsl/nir/nir_opt_cse.c
> @@ -112,7 +112,32 @@ nir_instrs_equal(nir_instr *instr1, nir_instr
*instr2)
>
>        return true;
>     }
> -   case nir_instr_type_intrinsic:
> +   case nir_instr_type_intrinsic: {
> +      nir_intrinsic_instr *intrinsic1 = nir_instr_as_intrinsic(instr1);
> +      nir_intrinsic_instr *intrinsic2 = nir_instr_as_intrinsic(instr2);
> +      const nir_intrinsic_info *info =
> +         &nir_intrinsic_infos[intrinsic1->intrinsic];
> +
> +      if (intrinsic1->intrinsic != intrinsic2->intrinsic ||
> +          intrinsic1->num_components != intrinsic2->num_components)
> +         return false;
> +
> +      if (info->has_dest && intrinsic1->dest.ssa.num_components !=
> +                            intrinsic2->dest.ssa.num_components)
> +         return false;
> +
> +      for (unsigned i = 0; i < info->num_srcs; i++) {
> +         if (!nir_srcs_equal(intrinsic1->src[i], intrinsic2->src[i]))
> +            return false;
> +      }
> +
> +      for (unsigned i = 0; i < info->num_indices; i++) {
> +         if (intrinsic1->const_index[i] != intrinsic2->const_index[i])
> +            return false;
> +      }

Can we assert num_vars == 0 somewhere in here.  It's a comment to the
reader that the assumption is there and a placeholder for when/if we add
variable support.

> +
> +      return true;
> +   }
>     case nir_instr_type_call:
>     case nir_instr_type_jump:
>     case nir_instr_type_ssa_undef:
> @@ -147,7 +172,13 @@ nir_instr_can_cse(nir_instr *instr)
>               nir_foreach_src(instr, src_is_ssa, NULL);
>     case nir_instr_type_tex:
>        return false; /* TODO */
> -   case nir_instr_type_intrinsic:
> +   case nir_instr_type_intrinsic: {
> +      const nir_intrinsic_info *info =
> +         &nir_intrinsic_infos[nir_instr_as_intrinsic(instr)->intrinsic];
> +      return (info->flags & NIR_INTRINSIC_CAN_ELIMINATE) &&
> +             (info->flags & NIR_INTRINSIC_CAN_REORDER) &&
> +             info->num_variables == 0; /* not implemented yet */

We also need to ensure sources and destination are SSA here.  Maybe that
should be pulled out of the switch.

> +   }
>     case nir_instr_type_call:
>     case nir_instr_type_jump:
>     case nir_instr_type_ssa_undef:
> @@ -172,6 +203,9 @@ nir_instr_get_dest_ssa_def(nir_instr *instr)
>     case nir_instr_type_phi:
>        assert(nir_instr_as_phi(instr)->dest.is_ssa);
>        return &nir_instr_as_phi(instr)->dest.ssa;
> +   case nir_instr_type_intrinsic:
> +      assert(nir_instr_as_intrinsic(instr)->dest.is_ssa);
> +      return &nir_instr_as_intrinsic(instr)->dest.ssa;
>     default:
>        unreachable("We never ask for any of these");
>     }
> --
> 2.2.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150121/605e5cd8/attachment.html>