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

[Tapani Pälli]

On 05/08/2015 10:35 PM, Jason Ekstrand wrote:
> 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.

Yes, makes sense.

>>>>             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?

Fair enough, I'll modify it this way. I'll try to add some more 
documentation too to make it more clear.

> --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