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

Tue Jan 28 11:45:45 PST 2014

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).

> +
> +   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)) {
>