On Tue, Jan 28, 2014 at 2:45 PM, Paul Berry <stereotype...@gmail.com> wrote:
> On 22 January 2014 09:16, Connor Abbott <cwabbo...@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@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. >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev