<div dir="ltr">On 27 September 2013 14:32, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There are some bits of this patch I'm trying to understand.  I think<br>
they can be cleared up by one question below...<br>
<div><div class="h5"><br>
On 09/14/2013 01:00 PM, Paul Berry wrote:<br>
> From: Bryan Cain <<a href="mailto:bryancain3@gmail.com">bryancain3@gmail.com</a>><br>
><br>
> This corresponds to the lowering of gl_ClipDistance to<br>
> gl_ClipDistanceMESA for vertex and geometry shader outputs.  Since<br>
> this lowering pass occurs after lower_named_interface blocks, it deals<br>
> with 2D arrays (gl_ClipDistance[vertex][clip_plane]) rather than 1D<br>
> arrays in an interface block<br>
> (gl_in[vertex].gl_ClipDistance[clip_plane]).<br>
><br>
> v2 (Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>): Fix indexing order for<br>
> gl_ClipDistance input lowering.  Properly lower bulk assignment of<br>
> gl_ClipDistance inputs.  Rework for GLSL 1.50 style geometry shaders.<br>
> ---<br>
>  src/glsl/lower_clip_distance.cpp | 223 +++++++++++++++++++++++++++++----------<br>
>  1 file changed, 165 insertions(+), 58 deletions(-)<br>
><br>
> diff --git a/src/glsl/lower_clip_distance.cpp b/src/glsl/lower_clip_distance.cpp<br>
> index d6cf944..16eaef6 100644<br>
> --- a/src/glsl/lower_clip_distance.cpp<br>
> +++ b/src/glsl/lower_clip_distance.cpp<br>
> @@ -53,13 +53,16 @@<br>
>  class lower_clip_distance_visitor : public ir_rvalue_visitor {<br>
>  public:<br>
>     lower_clip_distance_visitor()<br>
> -      : progress(false), old_clip_distance_var(NULL),<br>
> -        new_clip_distance_var(NULL)<br>
> +      : progress(false), old_clip_distance_1d_var(NULL),<br>
> +        old_clip_distance_2d_var(NULL), new_clip_distance_1d_var(NULL),<br>
> +        new_clip_distance_2d_var(NULL)<br>
>     {<br>
>     }<br>
><br>
>     virtual ir_visitor_status visit(ir_variable *);<br>
>     void create_indices(ir_rvalue*, ir_rvalue *&, ir_rvalue *&);<br>
> +   bool is_clip_distance_vec8(ir_rvalue *ir);<br>
> +   ir_rvalue *lower_clip_distance_vec8(ir_rvalue *ir);<br>
>     virtual ir_visitor_status visit_leave(ir_assignment *);<br>
>     void visit_new_assignment(ir_assignment *ir);<br>
>     virtual ir_visitor_status visit_leave(ir_call *);<br>
> @@ -73,12 +76,14 @@ public:<br>
>     /**<br>
>      * Pointer to the declaration of gl_ClipDistance, if found.<br>
>      */<br>
> -   ir_variable *old_clip_distance_var;<br>
> +   ir_variable *old_clip_distance_1d_var;<br>
> +   ir_variable *old_clip_distance_2d_var;<br>
><br>
>     /**<br>
>      * Pointer to the newly-created gl_ClipDistanceMESA variable.<br>
>      */<br>
> -   ir_variable *new_clip_distance_var;<br>
> +   ir_variable *new_clip_distance_1d_var;<br>
> +   ir_variable *new_clip_distance_2d_var;<br>
>  };<br>
><br>
><br>
> @@ -89,30 +94,60 @@ public:<br>
>  ir_visitor_status<br>
>  lower_clip_distance_visitor::visit(ir_variable *ir)<br>
>  {<br>
> -   /* No point in looking for the declaration of gl_ClipDistance if<br>
> -    * we've already found it.<br>
> -    */<br>
> -   if (this->old_clip_distance_var)<br>
> +   if (!ir->name || strcmp(ir->name, "gl_ClipDistance") != 0)<br>
>        return visit_continue;<br>
> +   assert (ir->type->is_array());<br>
> +<br>
> +   if (!ir->type->element_type()->is_array()) {<br>
> +      /* 1D gl_ClipDistance (used for vertex and geometry output, and fragment<br>
> +       * input).<br>
> +       */<br>
> +      if (this->old_clip_distance_1d_var)<br>
> +         return visit_continue;<br>
><br>
> -   if (ir->name && strcmp(ir->name, "gl_ClipDistance") == 0) {<br>
>        this->progress = true;<br>
> -      this->old_clip_distance_var = ir;<br>
> -      assert (ir->type->is_array());<br>
> +      this->old_clip_distance_1d_var = ir;<br>
>        assert (ir->type->element_type() == glsl_type::float_type);<br>
>        unsigned new_size = (ir->type->array_size() + 3) / 4;<br>
><br>
>        /* Clone the old var so that we inherit all of its properties */<br>
> -      this->new_clip_distance_var = ir->clone(ralloc_parent(ir), NULL);<br>
> +      this->new_clip_distance_1d_var = ir->clone(ralloc_parent(ir), NULL);<br>
><br>
>        /* And change the properties that we need to change */<br>
> -      this->new_clip_distance_var->name<br>
> -         = ralloc_strdup(this->new_clip_distance_var, "gl_ClipDistanceMESA");<br>
> -      this->new_clip_distance_var->type<br>
> +      this->new_clip_distance_1d_var->name<br>
> +         = ralloc_strdup(this->new_clip_distance_1d_var,<br>
> +                         "gl_ClipDistanceMESA");<br>
> +      this->new_clip_distance_1d_var->type<br>
>           = glsl_type::get_array_instance(glsl_type::vec4_type, new_size);<br>
> -      this->new_clip_distance_var->max_array_access = ir->max_array_access / 4;<br>
> +      this->new_clip_distance_1d_var->max_array_access<br>
> +         = ir->max_array_access / 4;<br>
> +<br>
> +      ir->replace_with(this->new_clip_distance_1d_var);<br>
> +   } else {<br>
> +      /* 2D gl_ClipDistance (used for geometry input). */<br>
> +      assert(ir->mode == ir_var_shader_in);<br>
> +      if (this->old_clip_distance_2d_var)<br>
> +         return visit_continue;<br>
><br>
> -      ir->replace_with(this->new_clip_distance_var);<br>
> +      this->progress = true;<br>
> +      this->old_clip_distance_2d_var = ir;<br>
> +      assert (ir->type->element_type()->element_type() == glsl_type::float_type);<br>
> +      unsigned new_size = (ir->type->element_type()->array_size() + 3) / 4;<br>
> +<br>
> +      /* Clone the old var so that we inherit all of its properties */<br>
> +      this->new_clip_distance_2d_var = ir->clone(ralloc_parent(ir), NULL);<br>
> +<br>
> +      /* And change the properties that we need to change */<br>
> +      this->new_clip_distance_2d_var->name<br>
> +         = ralloc_strdup(this->new_clip_distance_2d_var, "gl_ClipDistanceMESA");<br>
> +      this->new_clip_distance_2d_var->type = glsl_type::get_array_instance(<br>
> +         glsl_type::get_array_instance(glsl_type::vec4_type,<br>
> +            new_size),<br>
> +         ir->type->array_size());<br>
> +      this->new_clip_distance_2d_var->max_array_access<br>
> +         = ir->max_array_access / 4;<br>
> +<br>
> +      ir->replace_with(this->new_clip_distance_2d_var);<br>
>     }<br>
>     return visit_continue;<br>
>  }<br>
> @@ -177,39 +212,99 @@ lower_clip_distance_visitor::create_indices(ir_rvalue *old_index,<br>
>  }<br>
><br>
><br>
> +/**<br>
> + * Determine whether the given rvalue describes an array of 8 floats that<br>
> + * needs to be lowered to an array of 2 vec4's; that is, determine whether it<br>
> + * matches one of the following patterns:<br>
> + *<br>
> + * - gl_ClipDistance (if gl_ClipDistance is 1D)<br>
> + * - gl_ClipDistance[i] (if gl_ClipDistance is 2D)<br>
> + */<br>
> +bool<br>
> +lower_clip_distance_visitor::is_clip_distance_vec8(ir_rvalue *ir)<br>
> +{<br>
<br>
</div></div>Is it ever possible for both of these to be taken?<br></blockquote><div><br></div><div>Yes.  If we are in a geometry shader, then there's both a 1D clip distance variable (for the gl_ClipDistance output) and a 2D clip distance variable (for the gl_in[i].gl_ClipDistance input).  Note that this pass runs after lower_named_interface_blocks(), so the clip distance input just looks like a bare 2D array.  So if, for example, ir represents the gl_ClipDistance[i] part of the gl_ClipDistance[i][j] input, then:<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> +   if (this->old_clip_distance_1d_var) {<br></div></div></blockquote><div><br></div><div>This branch will be taken<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
> +      ir_dereference_variable *var_ref = ir->as_dereference_variable();<br></div></div></blockquote><div><br></div><div>But var_ref will be NULL because ir is not a variable dereference, so the following if test will skip:<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> +      if (var_ref && var_ref->var == this->old_clip_distance_1d_var)<br>
> +         return true;<br>
> +   }<br></div></div></blockquote><div><br></div><div>Then we'll fall through to here<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb">
<div class="h5">
> +   if (this->old_clip_distance_2d_var) {<br></div></div></blockquote><div><br></div><div>And this branch will be taken.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
> +      ir_dereference_array *array_ref = ir->as_dereference_array();<br></div></div></blockquote><div><br></div><div>Now array_ref will be non-NULL because the ir is an array dereference.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
> +      if (array_ref) {<br>
> +         ir_dereference_variable *var_ref =<br>
> +            array_ref->array->as_dereference_variable();<br></div></div></blockquote><div><br></div><div>So then we'll get here and var_ref will be a reference to the gl_ClipDistance (2D) input variable.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> +         if (var_ref && var_ref->var == this->old_clip_distance_2d_var)<br>
> +            return true;<br>
> +      }<br>
> +   }<br>
> +   return false;<br>
> +}<br></div></div></blockquote><div><br></div>Hopefully that helped?<br></div><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb">
<div class="h5">
> +<br>
> +<br>
> +/**<br>
> + * If the given ir satisfies is_clip_distance_vec8(), return new ir<br>
> + * representing its lowered equivalent.  That is, map:<br>
> + *<br>
> + * - gl_ClipDistance    => gl_ClipDistanceMESA    (if gl_ClipDistance is 1D)<br>
> + * - gl_ClipDistance[i] => gl_ClipDistanceMESA[i] (if gl_ClipDistance is 2D)<br>
> + *<br>
> + * Otherwise return NULL.<br>
> + */<br>
> +ir_rvalue *<br>
> +lower_clip_distance_visitor::lower_clip_distance_vec8(ir_rvalue *ir)<br>
> +{<br>
> +   if (this->old_clip_distance_1d_var) {<br>
> +      ir_dereference_variable *var_ref = ir->as_dereference_variable();<br>
> +      if (var_ref && var_ref->var == this->old_clip_distance_1d_var) {<br>
> +         return new(ralloc_parent(ir))<br>
> +            ir_dereference_variable(this->new_clip_distance_1d_var);<br>
> +      }<br>
> +   }<br>
> +   if (this->old_clip_distance_2d_var) {<br>
> +      ir_dereference_array *array_ref = ir->as_dereference_array();<br>
> +      if (array_ref) {<br>
> +         ir_dereference_variable *var_ref =<br>
> +            array_ref->array->as_dereference_variable();<br>
> +         if (var_ref && var_ref->var == this->old_clip_distance_2d_var) {<br>
> +            return new(ralloc_parent(ir))<br>
> +               ir_dereference_array(this->new_clip_distance_2d_var,<br>
> +                                    array_ref->array_index);<br>
> +         }<br>
> +      }<br>
> +   }<br>
> +   return NULL;<br>
> +}<br>
> +<br>
> +<br>
>  void<br>
>  lower_clip_distance_visitor::handle_rvalue(ir_rvalue **rv)<br>
>  {<br>
> -   /* If the gl_ClipDistance var hasn't been declared yet, then<br>
> -    * there's no way this deref can refer to it.<br>
> -    */<br>
> -   if (!this->old_clip_distance_var || *rv == NULL)<br>
> +   if (*rv == NULL)<br>
>        return;<br>
><br>
>     ir_dereference_array *const array_deref = (*rv)->as_dereference_array();<br>
>     if (array_deref == NULL)<br>
>        return;<br>
><br>
> -   /* Replace any expression that indexes into the gl_ClipDistance array<br>
> +   /* Replace any expression that indexes one of the floats in gl_ClipDistance<br>
>      * with an expression that indexes into one of the vec4's in<br>
>      * gl_ClipDistanceMESA and accesses the appropriate component.<br>
>      */<br>
> -   ir_dereference_variable *old_var_ref =<br>
> -      array_deref->array->as_dereference_variable();<br>
> -   if (old_var_ref && old_var_ref->var == this->old_clip_distance_var) {<br>
> +   ir_rvalue *lowered_vec8 =<br>
> +      this->lower_clip_distance_vec8(array_deref->array);<br>
> +   if (lowered_vec8 != NULL) {<br>
>        this->progress = true;<br>
>        ir_rvalue *array_index;<br>
>        ir_rvalue *swizzle_index;<br>
>        this->create_indices(array_deref->array_index, array_index, swizzle_index);<br>
>        void *mem_ctx = ralloc_parent(array_deref);<br>
><br>
> -      ir_dereference_array *const ClipDistanceMESA_deref =<br>
> -         new(mem_ctx) ir_dereference_array(this->new_clip_distance_var,<br>
> -                                           array_index);<br>
> +      ir_dereference_array *const new_array_deref =<br>
> +         new(mem_ctx) ir_dereference_array(lowered_vec8, array_index);<br>
><br>
>        ir_expression *const expr =<br>
>           new(mem_ctx) ir_expression(ir_binop_vector_extract,<br>
> -                                    ClipDistanceMESA_deref,<br>
> +                                    new_array_deref,<br>
>                                      swizzle_index);<br>
><br>
>        *rv = expr;<br>
> @@ -243,30 +338,34 @@ lower_clip_distance_visitor::fix_lhs(ir_assignment *ir)<br>
>  }<br>
><br>
>  /**<br>
> - * Replace any assignment having gl_ClipDistance (undereferenced) as its LHS<br>
> - * or RHS with a sequence of assignments, one for each component of the array.<br>
> - * Each of these assignments is lowered to refer to gl_ClipDistanceMESA as<br>
> - * appropriate.<br>
> + * Replace any assignment having the 1D gl_ClipDistance (undereferenced) as<br>
> + * its LHS or RHS with a sequence of assignments, one for each component of<br>
> + * the array.  Each of these assignments is lowered to refer to<br>
> + * gl_ClipDistanceMESA as appropriate.<br>
> + *<br>
> + * We need to do a similar replacement for 2D gl_ClipDistance, however since<br>
> + * it's an input, the only case we need to address is where a 1D slice of it<br>
> + * is the entire RHS of an assignment, e.g.:<br>
> + *<br>
> + *     foo = gl_in[i].gl_ClipDistance<br>
>   */<br>
>  ir_visitor_status<br>
>  lower_clip_distance_visitor::visit_leave(ir_assignment *ir)<br>
>  {<br>
> -   ir_dereference_variable *lhs_var = ir->lhs->as_dereference_variable();<br>
> -   ir_dereference_variable *rhs_var = ir->rhs->as_dereference_variable();<br>
> -   if ((lhs_var && lhs_var->var == this->old_clip_distance_var)<br>
> -       || (rhs_var && rhs_var->var == this->old_clip_distance_var)) {<br>
> -      /* LHS or RHS of the assignment is the entire gl_ClipDistance array.<br>
> -       * Since we are reshaping gl_ClipDistance from an array of floats to an<br>
> -       * array of vec4's, this isn't going to work as a bulk assignment<br>
> -       * anymore, so unroll it to element-by-element assignments and lower<br>
> -       * each of them.<br>
> +   if (this->is_clip_distance_vec8(ir->lhs) ||<br>
> +       this->is_clip_distance_vec8(ir->rhs)) {<br>
> +      /* LHS or RHS of the assignment is the entire 1D gl_ClipDistance array<br>
> +       * (or a 1D slice of a 2D gl_ClipDistance input array).  Since we are<br>
> +       * reshaping gl_ClipDistance from an array of floats to an array of<br>
> +       * vec4's, this isn't going to work as a bulk assignment anymore, so<br>
> +       * unroll it to element-by-element assignments and lower each of them.<br>
>         *<br>
>         * Note: to unroll into element-by-element assignments, we need to make<br>
>         * clones of the LHS and RHS.  This is safe because expressions and<br>
>         * l-values are side-effect free.<br>
>         */<br>
>        void *ctx = ralloc_parent(ir);<br>
> -      int array_size = this->old_clip_distance_var->type->array_size();<br>
> +      int array_size = ir->lhs->type->array_size();<br>
>        for (int i = 0; i < array_size; ++i) {<br>
>           ir_dereference_array *new_lhs = new(ctx) ir_dereference_array(<br>
>              ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i));<br>
> @@ -326,11 +425,17 @@ lower_clip_distance_visitor::visit_new_assignment(ir_assignment *ir)<br>
><br>
><br>
>  /**<br>
> - * If gl_ClipDistance appears as an argument in an ir_call expression, replace<br>
> - * it with a temporary variable, and make sure the ir_call is preceded and/or<br>
> - * followed by assignments that copy the contents of the temporary variable to<br>
> - * and/or from gl_ClipDistance.  Each of these assignments is then lowered to<br>
> - * refer to gl_ClipDistanceMESA.<br>
> + * If a 1D gl_ClipDistance variable appears as an argument in an ir_call<br>
> + * expression, replace it with a temporary variable, and make sure the ir_call<br>
> + * is preceded and/or followed by assignments that copy the contents of the<br>
> + * temporary variable to and/or from gl_ClipDistance.  Each of these<br>
> + * assignments is then lowered to refer to gl_ClipDistanceMESA.<br>
> + *<br>
> + * We need to do a similar replacement for 2D gl_ClipDistance, however since<br>
> + * it's an input, the only case we need to address is where a 1D slice of it<br>
> + * is passed as an "in" parameter to an ir_call, e.g.:<br>
> + *<br>
> + *     foo(gl_in[i].gl_ClipDistance)<br>
>   */<br>
>  ir_visitor_status<br>
>  lower_clip_distance_visitor::visit_leave(ir_call *ir)<br>
> @@ -349,12 +454,12 @@ lower_clip_distance_visitor::visit_leave(ir_call *ir)<br>
>        formal_param_node = formal_param_node->next;<br>
>        actual_param_node = actual_param_node->next;<br>
><br>
> -      ir_dereference_variable *deref = actual_param->as_dereference_variable();<br>
> -      if (deref && deref->var == this->old_clip_distance_var) {<br>
> -         /* User is trying to pass the whole gl_ClipDistance array to a<br>
> -          * function call.  Since we are reshaping gl_ClipDistance from an<br>
> -          * array of floats to an array of vec4's, this isn't going to work<br>
> -          * anymore, so use a temporary array instead.<br>
> +      if (this->is_clip_distance_vec8(actual_param)) {<br>
> +         /* User is trying to pass the whole 1D gl_ClipDistance array (or a 1D<br>
> +          * slice of a 2D gl_ClipDistance array) to a function call.  Since we<br>
> +          * are reshaping gl_ClipDistance from an array of floats to an array<br>
> +          * of vec4's, this isn't going to work anymore, so use a temporary<br>
> +          * array instead.<br>
>            */<br>
>           ir_variable *temp_clip_distance = new(ctx) ir_variable(<br>
>              actual_param->type, "temp_clip_distance", ir_var_temporary);<br>
> @@ -370,7 +475,7 @@ lower_clip_distance_visitor::visit_leave(ir_call *ir)<br>
>               */<br>
>              ir_assignment *new_assignment = new(ctx) ir_assignment(<br>
>                 new(ctx) ir_dereference_variable(temp_clip_distance),<br>
> -               new(ctx) ir_dereference_variable(old_clip_distance_var));<br>
> +               actual_param->clone(ctx, NULL));<br>
>              this->base_ir->insert_before(new_assignment);<br>
>              this->visit_new_assignment(new_assignment);<br>
>           }<br>
> @@ -382,7 +487,7 @@ lower_clip_distance_visitor::visit_leave(ir_call *ir)<br>
>               * afterwards to make sure it gets lowered.<br>
>               */<br>
>              ir_assignment *new_assignment = new(ctx) ir_assignment(<br>
> -               new(ctx) ir_dereference_variable(old_clip_distance_var),<br>
> +               actual_param->clone(ctx, NULL),<br>
>                 new(ctx) ir_dereference_variable(temp_clip_distance));<br>
>              this->base_ir->insert_after(new_assignment);<br>
>              this->visit_new_assignment(new_assignment);<br>
> @@ -401,8 +506,10 @@ lower_clip_distance(gl_shader *shader)<br>
><br>
>     visit_list_elements(&v, shader->ir);<br>
><br>
> -   if (v.new_clip_distance_var)<br>
> -      shader->symbols->add_variable(v.new_clip_distance_var);<br>
> +   if (v.new_clip_distance_1d_var)<br>
> +      shader->symbols->add_variable(v.new_clip_distance_1d_var);<br>
> +   if (v.new_clip_distance_2d_var)<br>
> +      shader->symbols->add_variable(v.new_clip_distance_2d_var);<br>
><br>
>     return v.progress;<br>
>  }<br>
><br>
<br>
</div></div></blockquote></div><br></div></div>