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.pa...@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.pa...@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@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
