<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 28, 2014 at 2:45 PM, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>On 22 January 2014 09:16, Connor Abbott <span dir="ltr"><<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp<br>
index cb732a5..7075579 100644<br>
--- a/src/glsl/ir_clone.cpp<br>
+++ b/src/glsl/ir_clone.cpp<br>
@@ -40,7 +40,15 @@ ir_rvalue::clone(void *mem_ctx, struct hash_table *ht) const<br>
ir_variable *<br>
ir_variable::clone(void *mem_ctx, struct hash_table *ht) const<br>
{<br>
- ir_variable *var = new(mem_ctx) ir_variable(this->type, this->name,<br>
+ ir_variable *var;<br>
+<br>
+ if (ht) {<br>
+ var = (ir_variable *)hash_table_find(ht, this);<br>
+ if (var)<br>
+ return var;<br>
+ }<br></blockquote><div><br></div></div><div>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).<br>
</div></div></div></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ var = new(mem_ctx) ir_variable(this->type, this->name,<br>
(ir_variable_mode) this->data.mode);<br></blockquote><div><br></div></div><div>Let's fix up this line to align the opening paren with the initial "t" in "this->type".<br>
</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ir_phi *<br>
+ir_phi::clone(void *mem_ctx, struct hash_table *ht) const<br>
+{<br>
+ /* shouldn't get here */<br>
+ assert(0);<br>
+ return new(mem_ctx) ir_phi();<br>
+}<br></blockquote><div><br></div></div><div>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.<br>
</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+static ir_phi_jump_src *<br>
+clone_phi_jump_src(ir_phi_jump_src *src, void *mem_ctx, struct hash_table *ht)<br>
+{<br>
+ ir_phi_jump_src *new_src = new(mem_ctx) ir_phi_jump_src();<br>
+ new_src->src = src->src->clone(mem_ctx, ht);<br>
+ new_src->jump = src->jump->clone(mem_ctx, ht);<br>
+ return new_src;<br></blockquote><div><br></div></div><div>Let's make a constructor for ir_phi_jump_src that takes src and jump as arguments, so that we can just do:<br><br></div> return new(mem_ctx) ir_phi_jump_src(src->src->clone(mem_ctx, ht),<br>
src->jump->clone(mem_ctx, ht)); <br><div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
+void<br>
+ir_print_visitor::visit(ir_phi *ir)<br>
+{<br>
+ printf("error");<br>
+}<br></blockquote><div><br></div></div><div>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.<br>
</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
+void<br>
+ir_print_visitor::visit(ir_phi_if *ir)<br>
+{<br>
+ printf("(phi\n");<br></blockquote><div><br></div></div><div>For consistency with the class names, I'd prefer for this to be:<br><br></div><div> printf("(phi_if\n");<br></div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ indentation++;<br>
+ indent();<br>
+ ir->dest->accept(this);<br>
+ printf("\n");<br>
+<br>
+ indent();<br>
+ if (ir->if_src) {<br>
+ printf("%s ", unique_name(ir->if_src));<br>
+ } else {<br>
+ printf("(undefined) ");<br>
+ }<br>
+ if (ir->else_src) {<br>
+ printf("%s)", unique_name(ir->else_src));<br>
+ } else {<br>
+ printf("(undefined))");<br>
+ }<br></blockquote><div><br></div></div><div>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:<br>
<br></div><div> printf("%s %s", unique_name(ir->if_src), unique_name(ir->else_src));<br><br>As well as similar simplifications in the functions below.<br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ indentation--;<br>
+}<br>
+<br>
+void<br>
+ir_print_visitor::print_phi_jump_src(ir_phi_jump_src *src)<br>
+{<br>
+ printf("(%s ", unique_name(src->jump));<br></blockquote><div><br></div></div><div>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:<br>
<br></div><div> printf("(phi_jump_src %s ", unique_name(src->jump));<br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ if (src->src) {<br>
+ printf(" %s)\n", unique_name(src->src));<br>
+ } else {<br>
+ printf(" (undefined))\n");<br>
+ }<br>
+}<br>
+<br>
+void<br>
+ir_print_visitor::visit(ir_phi_loop_begin *ir)<br>
+{<br>
+ printf("(phi\n");<br></blockquote><div><br></div></div><div>As with ir_phi_if, I'd recommend changing this to:<br><br></div><div> printf("(phi_loop_begin\n");<br><br>A similar comment applies to ir_print_visitor::visit(ir_phi_loop_end *).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>
+<br>
+ indentation++;<br>
+ indent();<br>
+ ir->dest->accept(this);<br>
+ printf("\n");<br>
+<br>
+ indent();<br>
+ if (ir->enter_src) {<br>
+ printf("%s ", unique_name(ir->enter_src));<br>
+ } else {<br>
+ printf("(undefined) ");<br>
+ }<br>
+ if (ir->repeat_src) {<br>
+ printf("%s\n", unique_name(ir->repeat_src));<br>
+ } else {<br>
+ printf("(undefined)\n");<br>
+ }<br>
+<br>
+ foreach_list(n, &ir->continue_srcs) {<br>
+ ir_phi_jump_src *src = (ir_phi_jump_src *) n;<br>
+ indent();<br>
+ print_phi_jump_src(src);<br>
+ }<br>
+<br>
+ indentation--;<br>
+ indent();<br>
+ printf(")");<br>
+}<br>
+<br>
+<br></div></div><div>
+<br>
+<br>
+ir_visitor_status<br>
+ir_validate::visit(ir_phi_loop_begin *ir)<br>
+{<br>
+ validate_phi_dest(ir, this->ht);<br>
+<br>
+ /*<br>
+ * note: ir_phi_loop_begin is a special case in that variables may be, and in<br>
+ * fact normally are, defined *after* they are used in everything except<br>
+ * enter_src<br>
+ */<br>
+<br>
+ if (ir->enter_src) {<br>
+ validate_var_use(ir->enter_src, this->ht, (void *) ir, "ir_phi_loop_begin");<br>
+ }<br>
+<br>
+ if ((ir->repeat_src == NULL) && (ir->repeat_src->as_variable() == NULL)) {<br></div></blockquote><div><br></div><div>I think you mean "||" instead of "&&" here.<br></div><div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ printf("ir_phi_loop_begin @ %p does not specify a variable %p\n",<br>
+ (void *) ir, (void *) ir->repeat_src);<br>
+ abort();<br>
+ }<br>
+<br>
+ foreach_list(n, &ir->continue_srcs) {<br>
+ ir_phi_jump_src *src = (ir_phi_jump_src *) n;<br>
+<br>
+ if ((src->src == NULL) && (src->src->as_variable() == NULL)) {<br></blockquote><div><br></div></div><div>Similar problem here.<br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ printf("ir_phi_loop_begin @ %p does not specify a variable %p\n",<br>
+ (void *) ir, (void *) src->src);<br>
+ abort();<br>
+ }<br>
+ }<br>
+<br>
+ return visit_continue;<br>
+}<br></blockquote><div><br></div></div><div>I can think of a few other validation checks that would be nice to have:<br></div><div><br>- 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).<br>
<br></div><div>- 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.<br><br></div><div>How difficult would it be to add those checks?<br>
</div>
<div> </div><br></div><div class="gmail_quote">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!<br><br></div>
<div class="gmail_quote">I'll try to get through the rest of the series as soon as I can.<br></div></div></div>
</blockquote></div><br></div></div>