[Mesa-dev] [PATCH RFC 07/11] glsl: add SSA infrastructure

[Connor Abbott]

On Tue, Jan 28, 2014 at 2:45 PM, Paul Berry <stereotype441 at gmail.com> wrote:

> On 22 January 2014 09:16, Connor Abbott <cwabbott0 at gmail.com> wrote:
>
>> diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
>> index cb732a5..7075579 100644
>> --- a/src/glsl/ir_clone.cpp
>> +++ b/src/glsl/ir_clone.cpp
>> @@ -40,7 +40,15 @@ ir_rvalue::clone(void *mem_ctx, struct hash_table *ht)
>> const
>>  ir_variable *
>>  ir_variable::clone(void *mem_ctx, struct hash_table *ht) const
>>  {
>> -   ir_variable *var = new(mem_ctx) ir_variable(this->type, this->name,
>> +   ir_variable *var;
>> +
>> +   if (ht) {
>> +      var = (ir_variable *)hash_table_find(ht, this);
>> +      if (var)
>> +        return var;
>> +   }
>>
>
> Can you add a comment explaining why it's necessary to do a hashtable
> lookup now?  (I'm guessing it's because a phi node can refer to an SSA
> temporary that occurs later in the instruction stream, and therefore when
> we go to clone an SSA temporary variable, it's possible that we've already
> cloned it, but I'm not 100% certain about that).
>

Yeah, that's right. Phi nodes at the beginning of loops must be able to
refer to SSA temporaries created during the loop, which means that
variables may be referenced by ir_phi_loop_begin before they are defined.
So, if we clone an ir_phi_loop_begin object, and therefore clone a variable
that it references that's defined inside the loop, we need to make sure
that we use the same variable when we clone it's definition.


>
>
>> +
>> +   var = new(mem_ctx) ir_variable(this->type, this->name,
>>                                                (ir_variable_mode)
>> this->data.mode);
>>
>
> Let's fix up this line to align the opening paren with the initial "t" in
> "this->type".
>
>
>> +
>> +ir_phi *
>> +ir_phi::clone(void *mem_ctx, struct hash_table *ht) const
>> +{
>> +   /* shouldn't get here */
>> +   assert(0);
>> +   return new(mem_ctx) ir_phi();
>> +}
>>
>
> There's no need to implement this function.  If we leave it out (and leave
> out the corresponding declaration of clone() in ir.h's declaration of the
> ir_phi class), then ir_phi will be an abstract base class, and the compiler
> will make sure we never instantiate it.
>
>
>> +static ir_phi_jump_src *
>> +clone_phi_jump_src(ir_phi_jump_src *src, void *mem_ctx, struct
>> hash_table *ht)
>> +{
>> +   ir_phi_jump_src *new_src = new(mem_ctx) ir_phi_jump_src();
>> +   new_src->src = src->src->clone(mem_ctx, ht);
>> +   new_src->jump = src->jump->clone(mem_ctx, ht);
>> +   return new_src;
>>
>
> Let's make a constructor for ir_phi_jump_src that takes src and jump as
> arguments, so that we can just do:
>
>    return new(mem_ctx) ir_phi_jump_src(src->src->clone(mem_ctx, ht),
>                                        src->jump->clone(mem_ctx, ht));
>
> +
>> +
>> +void
>> +ir_print_visitor::visit(ir_phi *ir)
>> +{
>> +   printf("error");
>> +}
>>
>
> I think if we make ir_phi an abstract base class (like I suggested
> earlier) we can drop this function entirely, as well as all the other
> visit(class ir_phi *) functions introduced in this patch.
>
>
>> +
>> +
>> +void
>> +ir_print_visitor::visit(ir_phi_if *ir)
>> +{
>> +   printf("(phi\n");
>>
>
> For consistency with the class names, I'd prefer for this to be:
>
>    printf("(phi_if\n");
>
>
>> +
>> +   indentation++;
>> +   indent();
>> +   ir->dest->accept(this);
>> +   printf("\n");
>> +
>> +   indent();
>> +   if (ir->if_src) {
>> +      printf("%s ", unique_name(ir->if_src));
>> +   } else {
>> +      printf("(undefined) ");
>> +   }
>> +   if (ir->else_src) {
>> +      printf("%s)", unique_name(ir->else_src));
>> +   } else {
>> +      printf("(undefined))");
>> +   }
>>
>
> Would it be worth modifying unique_name() so that it returns "(undefined)"
> if it's passed a null pointer?  That would allow us to simplify the 10
> lines above to:
>
>    printf("%s %s", unique_name(ir->if_src), unique_name(ir->else_src));
>
> As well as similar simplifications in the functions below.
>
>
>> +   indentation--;
>> +}
>> +
>> +void
>> +ir_print_visitor::print_phi_jump_src(ir_phi_jump_src *src)
>> +{
>> +   printf("(%s ", unique_name(src->jump));
>>
>
> I think this is going to make the output IR difficult to follow, since it
> won't be obvious that "(break at 1 ...)" represents a phi node rather than a
> break instruction.  I'd recommend changing this to:
>
>    printf("(phi_jump_src %s ", unique_name(src->jump));
>
>
>> +   if (src->src) {
>> +      printf(" %s)\n", unique_name(src->src));
>> +   } else {
>> +      printf(" (undefined))\n");
>> +   }
>> +}
>> +
>> +void
>> +ir_print_visitor::visit(ir_phi_loop_begin *ir)
>> +{
>> +   printf("(phi\n");
>>
>
> As with ir_phi_if, I'd recommend changing this to:
>
>    printf("(phi_loop_begin\n");
>
> A similar comment applies to ir_print_visitor::visit(ir_phi_loop_end *).
>
>
>> +
>> +   indentation++;
>> +   indent();
>> +   ir->dest->accept(this);
>> +   printf("\n");
>> +
>> +   indent();
>> +   if (ir->enter_src) {
>> +      printf("%s ", unique_name(ir->enter_src));
>> +   } else {
>> +      printf("(undefined) ");
>> +   }
>> +   if (ir->repeat_src) {
>> +      printf("%s\n", unique_name(ir->repeat_src));
>> +   } else {
>> +      printf("(undefined)\n");
>> +   }
>> +
>> +   foreach_list(n, &ir->continue_srcs) {
>> +      ir_phi_jump_src *src = (ir_phi_jump_src *) n;
>> +      indent();
>> +      print_phi_jump_src(src);
>> +   }
>> +
>> +   indentation--;
>> +   indent();
>> +   printf(")");
>> +}
>> +
>> +
>> +
>> +
>> +ir_visitor_status
>> +ir_validate::visit(ir_phi_loop_begin *ir)
>> +{
>> +   validate_phi_dest(ir, this->ht);
>> +
>> +   /*
>> +    * note: ir_phi_loop_begin is a special case in that variables may
>> be, and in
>> +    * fact normally are, defined *after* they are used in everything
>> except
>> +    * enter_src
>> +    */
>> +
>> +   if (ir->enter_src) {
>> +      validate_var_use(ir->enter_src, this->ht, (void *) ir,
>> "ir_phi_loop_begin");
>> +   }
>> +
>> +   if ((ir->repeat_src == NULL) && (ir->repeat_src->as_variable() ==
>> NULL)) {
>>
>
> I think you mean "||" instead of "&&" here.
>
>
>> +      printf("ir_phi_loop_begin @ %p does not specify a variable %p\n",
>> +           (void *) ir, (void *) ir->repeat_src);
>> +      abort();
>> +   }
>> +
>> +   foreach_list(n, &ir->continue_srcs) {
>> +      ir_phi_jump_src *src = (ir_phi_jump_src *) n;
>> +
>> +      if ((src->src == NULL) && (src->src->as_variable() == NULL)) {
>>
>
> Similar problem here.
>
>
>> +        printf("ir_phi_loop_begin @ %p does not specify a variable %p\n",
>> +              (void *) ir, (void *) src->src);
>> +        abort();
>> +      }
>> +   }
>> +
>> +   return visit_continue;
>> +}
>>
>
> I can think of a few other validation checks that would be nice to have:
>
> - Verify that variables of type ir_var_temporary_ssa can only appear
> inside an ir_assignment, ir_phi, or an ir_call (they don't appear isolated
> in the instruction stream like non-SSA variables).
>
> - Verify that variables whose type is not ir_var_temporary_ssa have all of
> the fields ssa_assignment, ssa_phi, and ssa_call set to null.
>
> How difficult would it be to add those checks?
>
>
> That completes my review of this patch.  Incidentally, although I've had a
> lot of comments, I really like what I've seen so far in this series.  Nice
> work!
>
> I'll try to get through the rest of the series as soon as I can.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140205/c12d7223/attachment.html>