[Mesa-dev] [PATCH] mesa/st: add support for NIR as possible driver IR

[Rob Clark]

On Wed, Apr 20, 2016 at 9:40 PM, Eric Anholt <eric at anholt.net> wrote:
> Rob Clark <robdclark at gmail.com> writes:
>
>> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> new file mode 100644
>> index 0000000..c15c537
>> --- /dev/null
>> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>
>> +static void
>> +st_nir_assign_uniform_locations(struct gl_program *prog,
>> +                                struct gl_shader_program *shader_program,
>> +                                struct exec_list *uniform_list, unsigned *size)
>> +{
>> +   int max = 0;
>> +   int shaderidx = 0;
>> +
>> +   nir_foreach_variable(uniform, uniform_list) {
>> +      int loc;
>> +
>> +      if (uniform->type->is_sampler()) {
>> +         unsigned val;
>> +         bool found = shader_program->UniformHash->get(val, uniform->name);
>> +         loc = shaderidx++;
>> +         assert(found);
>> +         /* this ensure that nir_lower_samplers looks at the correct
>> +          * shader_program->UniformStorage[location]:
>> +          */
>> +         uniform->data.location = val;
>> +      } else if (strncmp(uniform->name, "gl_", 3) == 0) {
>> +         const gl_state_index *const stateTokens = (gl_state_index *)uniform->state_slots[0].tokens;
>> +         /* This state reference has already been setup by ir_to_mesa, but we'll
>> +          * get the same index back here.
>> +          */
>> +         loc = _mesa_add_state_reference(prog->Parameters, stateTokens);
>> +      } else {
>> +         loc = _mesa_lookup_parameter_index(prog->Parameters, uniform->name);
>> +      }
>> +
>> +      /* is there a better way to do this?  If we have something like:
>> +       *
>> +       *    struct S {
>> +       *           float f;
>> +       *           vec4 v;
>> +       *    };
>> +       *    uniform S color;
>> +       *
>> +       * Then what we get in prog->Parameters looks like:
>> +       *
>> +       *    0: Name=color.f, Type=6, DataType=1406, Size=1
>> +       *    1: Name=color.v, Type=6, DataType=8b52, Size=4
>> +       *
>> +       * So the name doesn't match up and _mesa_lookup_parameter_index()
>> +       * fails.  In this case just find the first matching "color.*"..
>> +       *
>> +       * Note for arrays you could end up w/ color[n].f, for example.
>> +       */
>> +      if (loc < 0) {
>> +         int namelen = strlen(uniform->name);
>> +         for (unsigned i = 0; i < prog->Parameters->NumParameters; i++) {
>> +            struct gl_program_parameter *p = &prog->Parameters->Parameters[i];
>> +            if ((strncmp(p->Name, uniform->name, namelen) == 0) &&
>> +                ((p->Name[namelen] == '.') || (p->Name[namelen] == '['))) {
>> +               loc = i;
>> +               break;
>> +            }
>> +         }
>> +      }
>
> Yep, this looks like what you're stuck with.  You could make a helper
> with a name related to _mesa_lookup_parameter_index(), or just always
> use the block instead of _mesa_lookup_parameter_index(), I think, but
> meh.
>
>> +      uniform->data.driver_location = loc;
>> +
>> +      /*
>> +       * UBO's have their own address spaces, so don't count them towards the
>> +       * number of global uniforms
>> +       */
>> +      if ((uniform->data.mode == nir_var_uniform || uniform->data.mode == nir_var_shader_storage) &&
>> +          uniform->interface_type != NULL)
>> +         continue;
>
> Why are we setting the driver_location according to a Parameters[]
> lookup for UBOs?  I would think would skip that step entirely.  i965
> also seemed to skip the driver_location setup for them.
>
> With this UBO question answered somehow, I think I'll just give this
> patch an acked-by.  I've read through it for quite some time, but it's a
> huge change and most of what I care about for review in it is "does a
> driver converting to this new path not regress?"

(opps, somehow I overlooked this reply completely..)

yeah, I believe that driver_location is unused, so we could probably
move the continue to before the driver_location assignment.  I'd need
to run it through piglit to double check.


BR,
-R