On Mon, May 16, 2016 at 2:50 PM, Rob Clark <robdcl...@gmail.com> wrote:
> On Wed, Apr 20, 2016 at 9:40 PM, Eric Anholt <e...@anholt.net> wrote:
>> Rob Clark <robdcl...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
