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