[Mesa-dev] [PATCH] glsl: Stop tree grafting if a variable is overwritten as an 'out' param.

[Paul Berry]

On 22 September 2011 15:21, Kenneth Graunke <kenneth at whitecape.org> wrote:

> Found by inspection.
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/opt_tree_grafting.cpp |   37
> ++++++++++++++++++++++++++-----------
>  1 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/src/glsl/opt_tree_grafting.cpp
> b/src/glsl/opt_tree_grafting.cpp
> index 22a1749..d9ab2e6 100644
> --- a/src/glsl/opt_tree_grafting.cpp
> +++ b/src/glsl/opt_tree_grafting.cpp
> @@ -148,18 +148,17 @@ ir_tree_grafting_visitor::visit_enter(ir_loop *ir)
>    return visit_stop;
>  }
>
> +/**
> + * Check if we can continue grafting after writing to a variable.  If the
> + * expression we're trying to graft references the variable, we must stop.
> + *
> + * \param ir   An instruction that writes to a variable.
> + * \param var  The variable being updated.
> + */
>  ir_visitor_status
> -ir_tree_grafting_visitor::visit_leave(ir_assignment *ir)
> +ir_tree_grafting_visitor::check_graft(ir_instruction *ir, ir_variable
> *var)
>  {
> -   if (do_graft(&ir->rhs) ||
> -       do_graft(&ir->condition))
> -      return visit_stop;
> -
> -   /* If this assignment updates a variable used in the assignment
> -    * we're trying to graft, then we're done.
> -    */
> -   if (dereferences_variable(this->graft_assign->rhs,
> -                            ir->lhs->variable_referenced())) {
> +   if (dereferences_variable(this->graft_assign->rhs, var)) {
>       if (debug) {
>         printf("graft killed by: ");
>         ir->print();
> @@ -172,6 +171,19 @@ ir_tree_grafting_visitor::visit_leave(ir_assignment
> *ir)
>  }
>
>  ir_visitor_status
> +ir_tree_grafting_visitor::visit_leave(ir_assignment *ir)
> +{
> +   if (do_graft(&ir->rhs) ||
> +       do_graft(&ir->condition))
> +      return visit_stop;
> +
> +   /* If this assignment updates a variable used in the assignment
> +    * we're trying to graft, then we're done.
> +    */
> +   return check_graft(ir, ir->lhs->variable_referenced());
> +}
> +
> +ir_visitor_status
>  ir_tree_grafting_visitor::visit_enter(ir_function *ir)
>  {
>    (void) ir;
> @@ -195,8 +207,11 @@ ir_tree_grafting_visitor::visit_enter(ir_call *ir)
>       ir_rvalue *ir = (ir_rvalue *)iter.get();
>       ir_rvalue *new_ir = ir;
>
> -      if (sig_param->mode != ir_var_in && sig_param->mode !=
> ir_var_const_in)
> +      if (sig_param->mode != ir_var_in && sig_param->mode !=
> ir_var_const_in) {
> +        if (check_graft(ir, sig_param) == visit_stop)
> +           return visit_stop;
>         continue;
> +      }
>
>       if (do_graft(&new_ir)) {
>         ir->replace_with(new_ir);
> --
> 1.7.6.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

It looks like you need to add this line to the class declaration to get it
to compile:

ir_visitor_status check_graft(ir_instruction *ir, ir_variable *var);

I'm assuming that you actually tried this code and it worked, and this was
just a rebasing error?  If so, then this code is

Reviewed-by: Paul Berry <stereotype441 at gmail.com>

BTW, any interest in trying to craft a Piglit test that validates this fix?
I'm guessing it shouldn't be too hard (famous last words, I know).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110923/f50f82ae/attachment.html>