[Mesa-dev,1/2] glsl/opt_array_splitting: Fix crash when doing array indexing into other arrays

Submitted by Iago Toral Quiroga on Sept. 14, 2015, 11:49 a.m.

Details

Message ID 1442231388-23734-1-git-send-email-itoral@igalia.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Sept. 14, 2015, 11:49 a.m.
When we find indirect indexing into an array, the current implementation
of the array spliiting optimization pass does not look further into the
expression tree. However, if the variable expression involves variable
indexing into other arrays, we can miss that these other arrays also have
variable indexing. If that happens, the pass will crash later on after
hitting an assertion put there to ensure that split arrays are in fact
always indexed via constants:

shader_runner: opt_array_splitting.cpp:296:
void ir_array_splitting_visitor::split_deref(ir_dereference**): Assertion `constant' failed.

This patch fixes the problem by letting the pass step into the variable
index expression to identify these cases properly.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89607
---
 src/glsl/opt_array_splitting.cpp | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/glsl/opt_array_splitting.cpp b/src/glsl/opt_array_splitting.cpp
index 9e73f3c..1fdd013 100644
--- a/src/glsl/opt_array_splitting.cpp
+++ b/src/glsl/opt_array_splitting.cpp
@@ -185,8 +185,18 @@  ir_array_reference_visitor::visit_enter(ir_dereference_array *ir)
    /* If the access to the array has a variable index, we wouldn't
     * know which split variable this dereference should go to.
     */
-   if (entry && !ir->array_index->as_constant())
-      entry->split = false;
+   if (!ir->array_index->as_constant()) {
+      if (entry)
+         entry->split = false;
+      /* This variable indexing could come from a different array dereference
+       * that also has variable indexing, that is, something like a[b[a[b[0]]]].
+       * If we return visit_continue_with_parent here for the first appearence
+       * of a, then we can miss that b also has indirect indexing (if this is
+       * the only place in the program where such indirect indexing into b
+       * happens), so keep going.
+       */
+      return visit_continue;
+   }
 
    return visit_continue_with_parent;
 }

Comments

I think this never got a review, anyone willing to take it?

Iago

On Mon, 2015-09-14 at 13:49 +0200, Iago Toral Quiroga wrote:
> When we find indirect indexing into an array, the current implementation
> of the array spliiting optimization pass does not look further into the
> expression tree. However, if the variable expression involves variable
> indexing into other arrays, we can miss that these other arrays also have
> variable indexing. If that happens, the pass will crash later on after
> hitting an assertion put there to ensure that split arrays are in fact
> always indexed via constants:
> 
> shader_runner: opt_array_splitting.cpp:296:
> void ir_array_splitting_visitor::split_deref(ir_dereference**): Assertion `constant' failed.
> 
> This patch fixes the problem by letting the pass step into the variable
> index expression to identify these cases properly.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89607
> ---
>  src/glsl/opt_array_splitting.cpp | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glsl/opt_array_splitting.cpp b/src/glsl/opt_array_splitting.cpp
> index 9e73f3c..1fdd013 100644
> --- a/src/glsl/opt_array_splitting.cpp
> +++ b/src/glsl/opt_array_splitting.cpp
> @@ -185,8 +185,18 @@ ir_array_reference_visitor::visit_enter(ir_dereference_array *ir)
>     /* If the access to the array has a variable index, we wouldn't
>      * know which split variable this dereference should go to.
>      */
> -   if (entry && !ir->array_index->as_constant())
> -      entry->split = false;
> +   if (!ir->array_index->as_constant()) {
> +      if (entry)
> +         entry->split = false;
> +      /* This variable indexing could come from a different array dereference
> +       * that also has variable indexing, that is, something like a[b[a[b[0]]]].
> +       * If we return visit_continue_with_parent here for the first appearence
> +       * of a, then we can miss that b also has indirect indexing (if this is
> +       * the only place in the program where such indirect indexing into b
> +       * happens), so keep going.
> +       */
> +      return visit_continue;
> +   }
>  
>     return visit_continue_with_parent;
>  }
On Mon, 2015-09-14 at 13:49 +0200, Iago Toral Quiroga wrote:
> When we find indirect indexing into an array, the current
> implementation
> of the array spliiting optimization pass does not look further into
> the
> expression tree. However, if the variable expression involves
> variable
> indexing into other arrays, we can miss that these other arrays also
> have
> variable indexing. If that happens, the pass will crash later on
> after
> hitting an assertion put there to ensure that split arrays are in
> fact
> always indexed via constants:
> 
> shader_runner: opt_array_splitting.cpp:296:
> void ir_array_splitting_visitor::split_deref(ir_dereference**):
> Assertion `constant' failed.
> 
> This patch fixes the problem by letting the pass step into the
> variable
> index expression to identify these cases properly.

Makes sense to me. Both patches are:

Reviewed-by: Timothy Arceri <timothy.arceri@collabora.com>

Will you be pushing the piglit test in the bug report when pushing this
also?


> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89607
> ---
On Thu, 2016-03-03 at 09:38 +1100, Timothy Arceri wrote:
> On Mon, 2015-09-14 at 13:49 +0200, Iago Toral Quiroga wrote:
> > When we find indirect indexing into an array, the current
> > implementation
> > of the array spliiting optimization pass does not look further into
> > the
> > expression tree. However, if the variable expression involves
> > variable
> > indexing into other arrays, we can miss that these other arrays also
> > have
> > variable indexing. If that happens, the pass will crash later on
> > after
> > hitting an assertion put there to ensure that split arrays are in
> > fact
> > always indexed via constants:
> > 
> > shader_runner: opt_array_splitting.cpp:296:
> > void ir_array_splitting_visitor::split_deref(ir_dereference**):
> > Assertion `constant' failed.
> > 
> > This patch fixes the problem by letting the pass step into the
> > variable
> > index expression to identify these cases properly.
> 
> Makes sense to me. Both patches are:
> 
> Reviewed-by: Timothy Arceri <timothy.arceri@collabora.com>
> 
> Will you be pushing the piglit test in the bug report when pushing this
> also?

Sure, thanks for the review!

Iago

> 
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89607
> > ---
> 
>