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

[Rob Clark]

On Mon, May 16, 2016 at 2:50 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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.

yeah, piglit agrees with me, we can actually just move the
'if(ubo||ssbo) continue' up to the top of the loop

BR,
-R


>
> BR,
> -R