[Mesa-dev] [PATCH] nir: fix sampler lowering pass for arrays

[Jason Ekstrand]

Over-all, I think this is on the right track, but I still don't think
it's 100% correct.

On Fri, May 8, 2015 at 12:04 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 05/08/2015 09:56 AM, Pohjolainen, Topi wrote:
>>
>> On Fri, May 08, 2015 at 09:51:54AM +0300, Tapani P?lli wrote:
>>>
>>> This fixes bugs with special cases where we have arrays of
>>> structures containing samplers or arrays of samplers.
>>>
>>> I've verified that patch results in calculating same index value as
>>> returned by _mesa_get_sampler_uniform_value for IR. Patch makes
>>> following ES3 conformance test pass:
>>>
>>>         ES3-CTS.shaders.struct.uniform.sampler_array_fragment
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90114
>>> ---
>>>   src/glsl/nir/nir_lower_samplers.cpp | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/glsl/nir/nir_lower_samplers.cpp
>>> b/src/glsl/nir/nir_lower_samplers.cpp
>>> index cf8ab83..9859cc0 100644
>>> --- a/src/glsl/nir/nir_lower_samplers.cpp
>>> +++ b/src/glsl/nir/nir_lower_samplers.cpp
>>> @@ -78,7 +78,11 @@ lower_sampler(nir_tex_instr *instr, const struct
>>> gl_shader_program *shader_progr
>>>            instr->sampler_index *= glsl_get_length(deref->type);

We really should get rid of the multiply since the sampler index is
zero up until the end and the multiply does nothing but confuse
people.

>>>            switch (deref_array->deref_array_type) {
>>>            case nir_deref_array_type_direct:
>>> -            instr->sampler_index += deref_array->base_offset;
>>> +
>>> +            /* If this is an array of samplers. */
>>
>>
>> Above the case is for arrays and below you check for the sampler. This
>> comment does not tell much extra :)
>
>
> Yeah, not sure how to change it. What I want to state here is that only for
> arrays of samplers we need to do this, otherwise we don't. The only other
> case really is array of structs that contain sampler so maybe I should state
> that instead?
>
>
>
>>> +            if (deref->child->type->base_type == GLSL_TYPE_SAMPLER)
>>> +               instr->sampler_index += deref_array->base_offset;
>>> +
>>>               if (deref_array->deref.child)
>>>                  ralloc_asprintf_append(&name, "[%u]",
>>> deref_array->base_offset);

The two conditionals above should be combined.  If the deref has a
child, it should not have type SAMPLER and vice-versa.  A better way
to do it would be

if (deref_array->deref.child) {
   ralloc_asprintf_append(&name, "[%u]", deref_array->base_offset);
} else {
   assert(deref->child->type->bbase_type == GLSL_TYPE_SAMPLER);
   instr->sampler_index = deref_array->base_offset;
}

Also, it may be better to do that outside of the switch and turn the
switch into an "if (deref_array->deref_array_type ==
deref_array_type_indirect)" to handle indirects.  Right now, I don't
think that we correctly handle an indirect with a base offset other
than 0.

Does that make sense?
--Jason

>>>               break;
>>> --
>>> 2.1.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev