On Thu, 2016-11-10 at 02:46 -0800, Kenneth Graunke wrote:
> On Thursday, October 27, 2016 1:00:46 PM PST Timothy Arceri wrote:
> >
> > This is based on the code from the GLSL IR pass however unlike the
> > GLSL IR
> > pass it also supports arrays of arrays.
> >
> > As well as implementing the logic from the GLSL IR pass we add some
> > additional intrinsic cases to catch more system values.
> > ---
> > src/compiler/nir/nir_gather_info.c | 262
> > +++++++++++++++++++++++++++++--------
> > 1 file changed, 209 insertions(+), 53 deletions(-)
>
> This looks really nice. Patches 2 and 4 are
> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
>
> One question and a couple of small suggestions below...
>
> >
> >
> > diff --git a/src/compiler/nir/nir_gather_info.c
> > b/src/compiler/nir/nir_gather_info.c
> > index 380140a..e5cedd9 100644
> > --- a/src/compiler/nir/nir_gather_info.c
> > +++ b/src/compiler/nir/nir_gather_info.c
> > @@ -21,9 +21,191 @@
> > * IN THE SOFTWARE.
> > */
> >
> > +#include "main/mtypes.h"
> > #include "nir.h"
> >
> > static void
> > +set_io_mask(nir_shader *shader, nir_variable *var, int offset, int
> > len)
> > +{
> > + for (int i = 0; i < len; i++) {
> > + assert(var->data.location != -1);
> > +
> > + int idx = var->data.location + offset + i;
> > + bool is_patch_generic = var->data.patch &&
> > + idx != VARYING_SLOT_TESS_LEVEL_INNER
> > &&
> > + idx != VARYING_SLOT_TESS_LEVEL_OUTER
> > &&
> > + idx != VARYING_SLOT_BOUNDING_BOX0 &&
> > + idx != VARYING_SLOT_BOUNDING_BOX1;
> > + uint64_t bitfield;
> > +
> > + if (is_patch_generic) {
> > + assert(idx >= VARYING_SLOT_PATCH0 && idx <
> > VARYING_SLOT_TESS_MAX);
> > + bitfield = BITFIELD64_BIT(idx - VARYING_SLOT_PATCH0);
> > + }
> > + else {
> > + assert(idx < VARYING_SLOT_MAX);
> > + bitfield = BITFIELD64_BIT(idx);
> > + }
> > +
> > + if (var->data.mode == nir_var_shader_in) {
> > + if (is_patch_generic)
> > + shader->info->patch_inputs_read |= bitfield;
> > + else
> > + shader->info->inputs_read |= bitfield;
> > +
> > + /* double inputs read is only for vertex inputs */
> > + if (shader->stage == MESA_SHADER_VERTEX &&
> > + glsl_type_is_dual_slot(glsl_without_array(var-
> > >type)))
> > + shader->info->double_inputs_read |= bitfield;
> > +
> > + if (shader->stage == MESA_SHADER_FRAGMENT) {
> > + shader->info->fs.uses_sample_qualifier |= var-
> > >data.sample;
> > + }
> > + } else {
> > + assert(var->data.mode == nir_var_shader_out);
> > + if (is_patch_generic) {
> > + shader->info->patch_outputs_written |= bitfield;
> > + } else if (!var->data.read_only) {
> > + shader->info->outputs_written |= bitfield;
> > + }
> > +
> > + if (var->data.fb_fetch_output)
> > + shader->info->outputs_read |= bitfield;
> > + }
> > + }
> > +}
> > +
> > +/**
> > + * Mark an entire variable as used. Caller must ensure that the
> > variable
> > + * represents a shader input or output.
> > + */
> > +static void
> > +mark_whole_variable(nir_shader *shader, nir_variable *var)
> > +{
> > + const struct glsl_type *type = var->type;
> > + bool is_vertex_input = false;
> > +
> > + if (nir_is_per_vertex_io(var, shader->stage)) {
> > + assert(glsl_type_is_array(type));
> > + type = glsl_get_array_element(type);
> > + }
> > +
> > + if (shader->stage == MESA_SHADER_VERTEX &&
> > + var->data.mode == nir_var_shader_in)
> > + is_vertex_input = true;
> > +
> > + set_io_mask(shader, var, 0,
> > + glsl_count_attribute_slots(type, is_vertex_input));
> > +}
> > +
> > +static unsigned
> > +get_io_offset(nir_deref_var *deref, bool is_vertex_input)
> > +{
> > + unsigned offset = 0;
> > +
> > + nir_deref *tail = &deref->deref;
> > + while (tail->child != NULL) {
> > + tail = tail->child;
> > +
> > + if (tail->deref_type == nir_deref_type_array) {
> > + nir_deref_array *deref_array = nir_deref_as_array(tail);
> > +
> > + if (deref_array->deref_array_type ==
> > nir_deref_array_type_indirect) {
> > + return -1;
> > + }
> > +
> > + offset += glsl_count_attribute_slots(tail->type,
> > is_vertex_input) *
> > + deref_array->base_offset;
> > + }
>
> Do we need any dual slot handling when computing the offset?
> Or is that OK?
Should all be ok count_attribute_slots() already handles doubles
correctly.
This get_io_offset() function is a cut down copy of the one
in nir_lower_io().
>
> >
> > + /* TODO: we can get the offset for structs here see
> > nir_lower_io() */
> > + }
> > +
> > + return offset;
> > +}
> > +
> > +/**
> > + * Try to mark a portion of the given varying as used. Caller
> > must ensure
> > + * that the variable represents a shader input or output.
> > + *
> > + * If the index can't be interpreted as a constant, or some other
> > problem
> > + * occurs, then nothing will be marked and false will be returned.
> > + */
> > +static bool
> > +try_mask_partial_io(nir_shader *shader, nir_deref_var *deref)
> > +{
> > + nir_variable *var = deref->var;
> > + const struct glsl_type *type = var->type;
> > +
> > + if (nir_is_per_vertex_io(var, shader->stage)) {
> > + assert(glsl_type_is_array(type));
> > + type = glsl_get_array_element(type);
> > + }
> > +
> > + /* The code below only handles:
> > + *
> > + * - Indexing into matrices
> > + * - Indexing into arrays of (arrays, matrices, vectors, or
> > scalars)
> > + *
> > + * For now, we just give up if we see varying structs and
> > arrays of structs
> > + * here marking the entire variable as used.
> > + */
> > + if (!(glsl_type_is_matrix(type) ||
> > + (glsl_type_is_array(type) &&
> > + (glsl_type_is_numeric(glsl_without_array(type)) ||
> > + glsl_type_is_boolean(glsl_without_array(type)))))) {
>
> The last three lines should be indented one space further.
> (Looks like the bad indentation was copied from
> ir_set_program_inouts.cpp.)
>
> >
> > +
> > + /* If we don't know how to handle this case, give up and let
> > the
> > + * caller mark the whole variable as used.
> > + */
> > + return false;
> > + }
> > +
> > + bool is_vertex_input = false;
> > + if (shader->stage == MESA_SHADER_VERTEX &&
> > + var->data.mode == nir_var_shader_in)
> > + is_vertex_input = true;
> > +
> > + unsigned offset = get_io_offset(deref, is_vertex_input);
> > + if (offset == -1)
> > + return false;
> > +
> > + unsigned num_elems;
> > + unsigned elem_width = 1;
> > + unsigned mat_cols = 1;
> > + if (glsl_type_is_array(type)) {
> > + num_elems = glsl_get_aoa_size(type);
> > + if (glsl_type_is_matrix(glsl_without_array(type)))
> > + mat_cols =
> > glsl_get_matrix_columns(glsl_without_array(type));
> > + } else {
> > + num_elems = glsl_get_matrix_columns(type);
> > + }
> > +
> > + /* double element width for double types that takes two slots
> > */
> > + if (shader->stage != MESA_SHADER_VERTEX ||
> > + var->data.mode != nir_var_shader_in) {
> > + if (glsl_type_is_dual_slot(glsl_without_array(type))) {
> > + elem_width *= 2;
> > + }
> > + }
>
> You've got a boolean for this, so you can simplify to:
>
> if (!is_vertex_input &&
> glsl_type_is_dual_slot(glsl_without_array(type))) {
> elem_width *= 2;
> }
>
> (looks like ir_set_program_inouts didn't have that boolean)
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
