<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Nov 16, 2016 at 9:11 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Wednesday, November 16, 2016 8:37:50 PM PST Jason Ekstrand wrote: > On Mon, Nov 14, 2016 at 5:41 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > wrote: > > > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > > --- > > src/compiler/Makefile.sources | 1 + > > src/compiler/nir/nir.h | 1 + > > .../nir/nir_lower_clip_cull_distance_arrays.c | 187 > > +++++++++++++++++++++ > > 3 files changed, 189 insertions(+) > > create mode 100644 src/compiler/nir/nir_lower_clip_cull_distance_arrays.c > > > > diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources > > index 08d93e0..6c331b0 100644 > > --- a/src/compiler/Makefile.sources > > +++ b/src/compiler/Makefile.sources > > @@ -202,6 +202,7 @@ NIR_FILES = \ > > nir/nir_lower_bitmap.c \ > > nir/nir_lower_clamp_color_outputs.c \ > > nir/nir_lower_clip.c \ > > + nir/nir_lower_clip_cull_distance_arrays.c \ > > nir/nir_lower_double_ops.c \ > > nir/nir_lower_double_packing.c \ > > nir/nir_lower_drawpixels.c \ > > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > > index 0b78242..f3aadb9 100644 > > --- a/src/compiler/nir/nir.h > > +++ b/src/compiler/nir/nir.h > > @@ -2441,6 +2441,7 @@ bool nir_lower_idiv(nir_shader *shader); > > > > void nir_lower_clip_vs(nir_shader *shader, unsigned ucp_enables); > > void nir_lower_clip_fs(nir_shader *shader, unsigned ucp_enables); > > +void nir_lower_clip_cull_distance_arrays(nir_shader *nir); > > > > void nir_lower_two_sided_color(nir_shader *shader); > > > > diff --git a/src/compiler/nir/nir_lower_clip_cull_distance_arrays.c > > b/src/compiler/nir/nir_lower_clip_cull_distance_arrays.c > > new file mode 100644 > > index 0000000..9f8a7de > > --- /dev/null > > +++ b/src/compiler/nir/nir_lower_clip_cull_distance_arrays.c > > @@ -0,0 +1,187 @@ > > +/* > > + * Copyright © 2015 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the > > "Software"), > > + * to deal in the Software without restriction, including without > > limitation > > + * the rights to use, copy, modify, merge, publish, distribute, > > sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the > > next > > + * paragraph) shall be included in all copies or substantial portions of > > the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > > SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > > DEALINGS > > + * IN THE SOFTWARE. > > + */ > > + > > +#include "nir.h" > > +#include "nir_builder.h" > > + > > +/** > > + * @file > > + * > > + * This pass combines separate clip and cull distance arrays into a > > + * single array that contains both. Clip distances come first, then > > + * cull distances. It also populates nir_shader_info with the size > > + * of the original arrays so the driver knows which are which. > > + */ > > + > > +/** > > + * Get the length of the clip/cull distance array, looking past > > + * any interface block arrays. > > + */ > > +static unsigned > > +get_unwrapped_array_length(nir_variable *var) > > +{ > > + if (!var) > > + return 0; > > + > > + /* Unwrap GS input and TCS input/output interfaces. We want the > > + * underlying clip/cull distance array length, not the per-vertex > > + * array length. > > + */ > > > > Probably not needed, but we could put some stage-based asserts in here. </div></div>Yeah...we should probably put competent helpers for "is this per-vertex IO?" and "get me the actual underlying type please" in nir.h where everybody can use them. </blockquote><div> </div><div>That would be nice. At least the nir_var_is_per_vertex_io. The "get me the correct type please" is just </div><div>if (is_per_vertex_io(var)) </div><div> return glsl_get_array_element(var->type); </div><div>else </div><div> return var->type. </div><div>Actually, I think we may even be able to use var->interface_type in most of those cases. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5"> > > + const struct glsl_type *type = var->type; > > + if (glsl_type_is_array_of_arrays(type)) > > + type = glsl_get_array_element(type); > > + > > + return glsl_get_length(type); > > +} > > + > > +/** > > + * Get the type of the combined array (including interface block nesting). > > + */ > > +static const struct glsl_type * > > +get_new_array_type(const struct glsl_type *old_type, unsigned length) > > +{ > > + const struct glsl_type *type = glsl_array_type(glsl_float_type(), > > length); > > + > > + if (glsl_type_is_array_of_arrays(old_type)) > > + type = glsl_array_type(type, glsl_get_length(type)); > > + > > + return type; > > +} > > + > > +/** > > + * Rewrite any clip/cull distances to refer to the new combined array. > > + */ > > +static void > > +rewrite_references(nir_instr *instr, > > + nir_variable *combined, > > + unsigned cull_offset) > > +{ > > + if (instr->type != nir_instr_type_intrinsic) > > + return; > > + > > + nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr); > > + > > + /* copy_var needs to be lowered to load/store before calling this pass > > */ > > + assert(intrin->intrinsic != nir_intrinsic_copy_var); > > > > Wouldn't be that hard to make it support it, but sure. > > > > + > > + if (intrin->intrinsic != nir_intrinsic_load_var && > > + intrin->intrinsic != nir_intrinsic_store_var) > > + return; > > + > > + nir_deref_var *var_ref = intrin->variables[0]; > > + if (var_ref->var->data.mode != combined->data.mode) > > + return; > > + > > + if (var_ref->var->data.location != VARYING_SLOT_CLIP_DIST0 && > > + var_ref->var->data.location != VARYING_SLOT_CULL_DIST0) > > + return; > > + > > + /* Update types along the deref chain */ > > + const struct glsl_type *type = combined->type; > > + nir_deref *deref = &var_ref->deref; > > + while (deref) { > > + deref->type = type; > > + deref = deref->child; > > + type = glsl_get_array_element(type); > > + } > > + > > + /* For cull distances, add an offset to the array index */ > > + if (var_ref->var->data.location == VARYING_SLOT_CULL_DIST0) { > > + nir_deref *tail = nir_deref_tail(&intrin->variables[0]->deref); > > + nir_deref_array *array_ref = nir_deref_as_array(tail); > > + > > + array_ref->base_offset += cull_offset; > > + } > > + > > + /* Point the deref at the combined array */ > > + var_ref->var = combined; > > + > > + /* There's no need to update writemasks; it's a scalar array. */ > > +} > > + > > +static void > > +combine_clip_cull(nir_shader *nir, > > + struct exec_list *vars, > > + bool store_info) > > +{ > > + nir_variable *cull = NULL; > > + nir_variable *clip = NULL; > > + > > + nir_foreach_variable(var, vars) { > > + if (var->data.location == VARYING_SLOT_CLIP_DIST0) > > + clip = var; > > + > > + if (var->data.location == VARYING_SLOT_CULL_DIST0) > > + cull = var; > > + } > > + > > + const unsigned clip_array_size = get_unwrapped_array_length(clip); > > + const unsigned cull_array_size = get_unwrapped_array_length(cull); > > + > > + if (store_info) { > > + nir->info->clip_distance_array_size = clip_array_size; > > + nir->info->cull_distance_array_size = cull_array_size; > > > > Why is this optional? If the sizes here and in the info ever differ, > you're going to have a problem. </div></div>We call this function twice - once for inputs, and once for outputs. The info values should come from the shader outputs. Though, looking below...... </blockquote><div> Right... I forgot about in vs. out. Maybe we could pick a better name like num_clip_out and num_cull_out? </div><div>I made a bunch of comments. I'm not extraordinarily set on any of them so do with it what you will. 1-11 and 13-14 are </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5"> > > + } > > + > > + if (clip) > > + clip->data.compact = true; > > + > > + if (cull) > > + cull->data.compact = true; > > + > > + if (cull_array_size > 0) { > > + if (clip_array_size == 0) { > > + /* No clip distances, just change the cull distance location */ > > + cull->data.location = VARYING_SLOT_CLIP_DIST0; > > + } else { > > + /* Turn the ClipDistance array into a combined one */ > > + clip->type = get_new_array_type(clip->type, > > + clip_array_size + > > cull_array_size); > > + > > + /* Rewrite CullDistance to reference the combined array */ > > + nir_foreach_function(function, nir) { > > + if (function->impl) { > > + nir_foreach_block(block, function->impl) { > > + nir_foreach_instr(instr, block) { > > + rewrite_references(instr, clip, clip_array_size); > > + } > > + } > > + } > > + } > > + > > + /* Delete the old CullDistance variable */ > > + exec_node_remove(&cull->node); > > + ralloc_free(cull); > > + } > > + } > > +} > > + > > +void > > +nir_lower_clip_cull_distance_arrays(nir_shader *nir) > > +{ > > + if (nir->stage <= MESA_SHADER_GEOMETRY) > > + combine_clip_cull(nir, &nir->outputs, &nir->info); > > + > > + if (nir->stage > MESA_SHADER_VERTEX) > > + combine_clip_cull(nir, &nir->inputs, NULL); > > +} </div></div>.....apparently I meant to pass "true" and "false" here. (I'd originally passed the shader_info pointer, rather than a flag, but once I started passing nir_shader as well, I changed it. Oops.) </blockquote></div> </div></div>