Re: [Mesa-dev] [PATCH] nir: fix sampler lowering pass for arrays
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 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
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
Re: [Mesa-dev] [PATCH] nir: fix sampler lowering pass for arrays
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 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
>>> 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
Re: [Mesa-dev] [PATCH] nir: fix sampler lowering pass for arrays
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
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);
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);
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
Re: [Mesa-dev] [PATCH] nir: fix sampler lowering pass for arrays
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
> 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);
> 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 :)
> +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);
> 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] [PATCH] nir: fix sampler lowering pass for arrays
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
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);
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. */
+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);
break;
--
2.1.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
