[Mesa-dev] [PATCH v2] glsl: compute lvalues of [in]out parameters before inlined function body

[Nicolai Hähnle]

Ping.

On 20.10.2016 11:00, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> This is required when an out argument involves an array index that is either
> a global variable modified by the function or another out argument in the
> same function call.
>
> Fixes the shaders/out-parameter-indexing/vs-inout-index-inout-* tests.
>
> v2:
> - modify the ir_dereference_array nodes in place
> - use ir_hierarchical_visitor
> --
>
> Once I changed the code to just modifying the existing nodes in-place during
> save_lvalue, it suddenly made much more sense to use an ir_hierarchical_visitor
> as well :)
>
> ---
>  src/compiler/glsl/opt_function_inlining.cpp | 93 +++++++++++++++++++++++++----
>  1 file changed, 83 insertions(+), 10 deletions(-)
>
> diff --git a/src/compiler/glsl/opt_function_inlining.cpp b/src/compiler/glsl/opt_function_inlining.cpp
> index 83534bf..26d451b 100644
> --- a/src/compiler/glsl/opt_function_inlining.cpp
> +++ b/src/compiler/glsl/opt_function_inlining.cpp
> @@ -55,20 +55,27 @@ public:
>
>     virtual ir_visitor_status visit_enter(ir_expression *);
>     virtual ir_visitor_status visit_enter(ir_call *);
>     virtual ir_visitor_status visit_enter(ir_return *);
>     virtual ir_visitor_status visit_enter(ir_texture *);
>     virtual ir_visitor_status visit_enter(ir_swizzle *);
>
>     bool progress;
>  };
>
> +class ir_save_lvalue_visitor : public ir_hierarchical_visitor {
> +public:
> +   virtual ir_visitor_status visit_enter(ir_dereference_array *);
> +
> +   ir_instruction *insert_before;
> +};
> +
>  } /* unnamed namespace */
>
>  bool
>  do_function_inlining(exec_list *instructions)
>  {
>     ir_function_inlining_visitor v;
>
>     v.run(instructions);
>
>     return v.progress;
> @@ -88,20 +95,51 @@ replace_return_with_assignment(ir_instruction *ir, void *data)
>        } else {
>  	 /* un-valued return has to be the last return, or we shouldn't
>  	  * have reached here. (see can_inline()).
>  	  */
>  	 assert(ret->next->is_tail_sentinel());
>  	 ret->remove();
>        }
>     }
>  }
>
> +/* Save the given lvalue before the given instruction.
> + *
> + * This is done by adding temporary variables into which the current value
> + * of any array indices are saved, and then modifying the dereference chain
> + * in-place to point to those temporary variables.
> + *
> + * The hierarchical visitor is only used to traverse the left-hand-side chain
> + * of derefs.
> + */
> +ir_visitor_status
> +ir_save_lvalue_visitor::visit_enter(ir_dereference_array *deref)
> +{
> +   if (deref->array_index->ir_type != ir_type_constant) {
> +      void *ctx = ralloc_parent(deref);
> +      ir_variable *index;
> +      ir_assignment *assignment;
> +
> +      index = new(ctx) ir_variable(deref->array_index->type, "saved_idx", ir_var_temporary);
> +      insert_before->insert_before(index);
> +
> +      assignment = new(ctx) ir_assignment(new(ctx) ir_dereference_variable(index),
> +                                          deref->array_index, 0);
> +      insert_before->insert_before(assignment);
> +
> +      deref->array_index = new(ctx) ir_dereference_variable(index);
> +   }
> +
> +   deref->array->accept(this);
> +   return visit_stop;
> +}
> +
>  void
>  ir_call::generate_inline(ir_instruction *next_ir)
>  {
>     void *ctx = ralloc_parent(this);
>     ir_variable **parameters;
>     unsigned num_parameters;
>     int i;
>     struct hash_table *ht;
>
>     ht = _mesa_hash_table_create(NULL, _mesa_hash_pointer, _mesa_key_pointer_equal);
> @@ -132,29 +170,64 @@ ir_call::generate_inline(ir_instruction *next_ir)
>
>  	 /* Remove the read-only decoration because we're going to write
>  	  * directly to this variable.  If the cloned variable is left
>  	  * read-only and the inlined function is inside a loop, the loop
>  	  * analysis code will get confused.
>  	  */
>  	 parameters[i]->data.read_only = false;
>  	 next_ir->insert_before(parameters[i]);
>        }
>
> -      /* Move the actual param into our param variable if it's an 'in' type. */
> -      if (parameters[i] && (sig_param->data.mode == ir_var_function_in ||
> -			    sig_param->data.mode == ir_var_const_in ||
> -			    sig_param->data.mode == ir_var_function_inout)) {
> -	 ir_assignment *assign;
> -
> -	 assign = new(ctx) ir_assignment(new(ctx) ir_dereference_variable(parameters[i]),
> -					 param, NULL);
> -	 next_ir->insert_before(assign);
> +      /* Section 6.1.1 (Function Calling Conventions) of the OpenGL Shading
> +       * Language 4.5 spec says:
> +       *
> +       *    "All arguments are evaluated at call time, exactly once, in order,
> +       *     from left to right. [...] Evaluation of an out parameter results
> +       *     in an l-value that is used to copy out a value when the function
> +       *     returns."
> +       *
> +       * I.e., we have to take temporary copies of any relevant array indices
> +       * before the function body is executed.
> +       *
> +       * This ensures that
> +       * (a) if an array index expressions refers to a variable that is
> +       *     modified by the execution of the function body, we use the
> +       *     original value as intended, and
> +       * (b) if an array index expression has side effects, those side effects
> +       *     are only executed once and at the right time.
> +       */
> +      if (parameters[i]) {
> +         if (sig_param->data.mode == ir_var_function_in ||
> +             sig_param->data.mode == ir_var_const_in) {
> +            ir_assignment *assign;
> +
> +            assign = new(ctx) ir_assignment(new(ctx) ir_dereference_variable(parameters[i]),
> +                                            param, NULL);
> +            next_ir->insert_before(assign);
> +         } else {
> +            assert(sig_param->data.mode == ir_var_function_out ||
> +                   sig_param->data.mode == ir_var_function_inout);
> +            assert(param->is_lvalue());
> +
> +            ir_save_lvalue_visitor v;
> +            v.insert_before = next_ir;
> +
> +            param->accept(&v);
> +
> +            if (sig_param->data.mode == ir_var_function_inout) {
> +               ir_assignment *assign;
> +
> +               assign = new(ctx) ir_assignment(new(ctx) ir_dereference_variable(parameters[i]),
> +                                               param->clone(ctx, NULL)->as_rvalue(), NULL);
> +               next_ir->insert_before(assign);
> +            }
> +         }
>        }
>
>        ++i;
>     }
>
>     exec_list new_instructions;
>
>     /* Generate the inlined body of the function to a new list */
>     foreach_in_list(ir_instruction, ir, &callee->body) {
>        ir_instruction *new_ir = ir->clone(ctx, ht);
> @@ -189,21 +262,21 @@ ir_call::generate_inline(ir_instruction *next_ir)
>     foreach_two_lists(formal_node, &this->callee->parameters,
>                       actual_node, &this->actual_parameters) {
>        ir_rvalue *const param = (ir_rvalue *) actual_node;
>        const ir_variable *const sig_param = (ir_variable *) formal_node;
>
>        /* Move our param variable into the actual param if it's an 'out' type. */
>        if (parameters[i] && (sig_param->data.mode == ir_var_function_out ||
>  			    sig_param->data.mode == ir_var_function_inout)) {
>  	 ir_assignment *assign;
>
> -	 assign = new(ctx) ir_assignment(param->clone(ctx, NULL)->as_rvalue(),
> +         assign = new(ctx) ir_assignment(param,
>  					 new(ctx) ir_dereference_variable(parameters[i]),
>  					 NULL);
>  	 next_ir->insert_before(assign);
>        }
>
>        ++i;
>     }
>
>     delete [] parameters;
>
>