[Mesa-dev] glsl: only match gl_FragData and not gl_SecondaryFragDataEXT

Submitted by Ilia Mirkin on June 21, 2016, 7:01 p.m.

Details

Message ID 1466535676-3050-1-git-send-email-imirkin@alum.mit.edu
State New, archived
Headers show
Series "glsl: only match gl_FragData and not gl_SecondaryFragDataEXT" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Ilia Mirkin June 21, 2016, 7:01 p.m.
There's special logic around finding gl_FragData. It latches onto any
array with FRAG_RESULT_DATA0. However gl_SecondaryFragDataEXT[], added
by GL_EXT_blend_func_extended, fits those parameters as well. The real
frag data array should have index 0 though, so we can use that to
distinguish them.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96617
Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: "11.1 11.2 12.0" <mesa-stable@lists.freedesktop.org>
---

TBH I don't fully understand all this code. However this makes a hacked-up piglit test pass. This should go through a full testing cycle before being pushed to make sure it doesn't break anything in the process.

 src/compiler/glsl/opt_dead_builtin_varyings.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/glsl/opt_dead_builtin_varyings.cpp b/src/compiler/glsl/opt_dead_builtin_varyings.cpp
index 37bcbcc..0f9fdb3 100644
--- a/src/compiler/glsl/opt_dead_builtin_varyings.cpp
+++ b/src/compiler/glsl/opt_dead_builtin_varyings.cpp
@@ -88,7 +88,9 @@  public:
       if (!var || var->data.mode != this->mode || !var->type->is_array())
          return visit_continue;
 
-      if (this->find_frag_outputs && var->data.location == FRAG_RESULT_DATA0) {
+      /* Only match gl_FragData[], not gl_SecondaryFragDataEXT[] */
+      if (this->find_frag_outputs && var->data.location == FRAG_RESULT_DATA0 &&
+          var->data.index == 0) {
          this->fragdata_array = var;
 
          ir_constant *index = ir->array_index->as_constant();

Comments

On Tuesday, June 21, 2016 3:01:16 PM PDT Ilia Mirkin wrote:
> There's special logic around finding gl_FragData. It latches onto any
> array with FRAG_RESULT_DATA0. However gl_SecondaryFragDataEXT[], added
> by GL_EXT_blend_func_extended, fits those parameters as well. The real
> frag data array should have index 0 though, so we can use that to
> distinguish them.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96617
> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: "11.1 11.2 12.0" <mesa-stable@lists.freedesktop.org>
> ---
> 
> TBH I don't fully understand all this code. However this makes a hacked-up piglit test pass. This should go through a full testing cycle before being pushed to make sure it doesn't break anything in the process.
> 
>  src/compiler/glsl/opt_dead_builtin_varyings.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/glsl/opt_dead_builtin_varyings.cpp b/src/compiler/glsl/opt_dead_builtin_varyings.cpp
> index 37bcbcc..0f9fdb3 100644
> --- a/src/compiler/glsl/opt_dead_builtin_varyings.cpp
> +++ b/src/compiler/glsl/opt_dead_builtin_varyings.cpp
> @@ -88,7 +88,9 @@ public:
>        if (!var || var->data.mode != this->mode || !var->type->is_array())
>           return visit_continue;
>  
> -      if (this->find_frag_outputs && var->data.location == FRAG_RESULT_DATA0) {
> +      /* Only match gl_FragData[], not gl_SecondaryFragDataEXT[] */
> +      if (this->find_frag_outputs && var->data.location == FRAG_RESULT_DATA0 &&
> +          var->data.index == 0) {
>           this->fragdata_array = var;
>  
>           ir_constant *index = ir->array_index->as_constant();
> 

This looks good to me.  This code is trying to break up the
gl_FragData[] array and throw away unused components.  Things will go
very poorly if we recognize the wrong variable.

It would make sense to update the pass to also modify
gl_SecondaryFragDataEXT[] in the same way as it handles gl_FragData[].
However, Ryan's code for that hardcodes the array to length 1 anyway
(as no driver supports more than 1 RT with dual color blending), so
it's kind of a moot point.  We'd just need to update it if some driver
ever gained support for MRT + dual color blending.

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>