[Mesa-dev] [PATCH 12/24] glsl: Add predicate to determine if an IR node has side effects.

[Paul Berry]

On 15 September 2013 00:10, Francisco Jerez <currojerez at riseup.net> wrote:

> And fix the dead code elimination pass so atomic writes aren't
> optimized out in cases where the return value isn't used by the
> program.
>

As I mentioned in my comments on patch 9, I'd prefer if we went with a
different approach where we don't add ir_rvalue nodes that have side
effects at all, but instead stick with our current system where only
ir_instruction nodes can have side effects.

However, if we do wind up going with this approach, we'll need to make
has_side_effects() recurse through the expression tree, so that it can see
that expressions like (atomicCounterIncrement(c) + 1) and
foo[atomicCounterIncrement(c)] also have side effects.

We would also need to modify several other optimization passes to check for
side effects:

- opt_dead_code_local (which does the same job as opt_dead_code, but within
basic blocks)

- opt_tree_grafting - as is, this would try to transform:

uint x = atomicCounterIncrement(c);
uint y = atomicCounterIncrement(c);
uvec3 v = uvec3(y, x, y);

Into:

uint y = atomicCounterIncrement(c);
uvec3 v = uvec3(y, atomicCounterIncrement(c), y);

Which is not equivalent.

- lower_variable_index_to_cond_assign, lower_vec_index_to_cond_assign, and
lower_vector_insert (which sometimes clone arbitrary rvalues)

And possibly others I've missed.


There's also at least one contrived case which is propably not worth fixing:

- opt_flip_matrices (in principle this might try to rewrite
gl_TextureMatrix[atomicCounterIncrement(c)] *
foo[atomicCounterIncrement(c)] to foo[atomicCounterIncrement(c)] *
gl_TextureMatrixTranspose(c), which is technically not the same operation).


> ---
>  src/glsl/ir.h              | 16 ++++++++++++++++
>  src/glsl/opt_dead_code.cpp |  3 ++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index c4b4677..4f506a3 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -139,6 +139,17 @@ public:
>     virtual class ir_jump *              as_jump()             { return
> NULL; }
>     /*@}*/
>
> +   /**
> +    * Determine if an IR instruction has side effects other than its
> +    * returned value(s).  Optimization passes are expected to be
> +    * especially careful with reordering or removing these, unless
> +    * they know what they are doing.
> +    */
> +   virtual bool has_side_effects() const
> +   {
> +      return false;
> +   }
> +
>  protected:
>     ir_instruction()
>     {
> @@ -2120,6 +2131,11 @@ public:
>
>     virtual ir_visitor_status accept(ir_hierarchical_visitor *);
>
> +   virtual bool has_side_effects() const
> +   {
> +      return true;
> +   }
> +
>     /** Kind of atomic instruction. */
>     enum ir_atomic_opcode op;
>
> diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp
> index b65e5c2..fd05034 100644
> --- a/src/glsl/opt_dead_code.cpp
> +++ b/src/glsl/opt_dead_code.cpp
> @@ -81,7 +81,8 @@ do_dead_code(exec_list *instructions, bool
> uniform_locations_assigned)
>           */
>          if (entry->var->mode != ir_var_function_out &&
>              entry->var->mode != ir_var_function_inout &&
> -             entry->var->mode != ir_var_shader_out) {
> +             entry->var->mode != ir_var_shader_out &&
> +             !entry->assign->rhs->has_side_effects()) {
>             entry->assign->remove();
>             progress = true;
>
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130917/a74061b6/attachment.html>