On Jan 21, 2015 3:29 AM, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> 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 <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> > --- 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 <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>