<div>A valid criticism. It hadn't occured to me at the time, but you are right.</div>
<div> </div>
<div>The problem I was trying to sovle was gitting visibility to the test expression IR and the fall through state IR during case label processing.</div>
<div> </div>
<div>Another solution is to to add those IR pointers to struct _mesa_glsl_parse_state. I removed one such pointer (switch_or_loop_nesting) for other reasons, but adding other IR pointers to struct _mesa_glsl_parse_state doesn't seem to be an issue (at least not to previous implementors :).</div>
<div> </div>
<div>I'll look at it in more detail when I continue that work on Friday.</div>
<div> </div>
<div>Thanks.</div>
<div> </div>
<div>cheers, danm<br><br></div>
<div class="gmail_quote">On Wed, Jun 15, 2011 at 11:59 AM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div class="im">On 06/15/2011 09:33 AM, Dan McCabe wrote:<br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">Data structures for switch statement and case label are created that parallel<br>the structure of other AST data.<br>
---<br> src/glsl/ast.h | 27 +++++++++++++++++++++++----<br> 1 files changed, 23 insertions(+), 4 deletions(-)<br></blockquote><br></div>Dan, thanks for sending this out! A few comments below...
<div>
<div></div>
<div class="h5"><br><br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">diff --git a/src/glsl/ast.h b/src/glsl/ast.h<br>index 878f48b..48d1795 100644<br>--- a/src/glsl/ast.h<br>+++ b/src/glsl/ast.h<br>
@@ -628,13 +628,19 @@ public:<br><br> class ast_case_label : public ast_node {<br> public:<br>+ ast_case_label(ast_expression *test_value);<br>+ virtual void print(void) const;<br>+<br>+ virtual ir_rvalue *hir(exec_list *instructions,<br>
+ struct _mesa_glsl_parse_state *state);<br><br> /**<br>- * An expression of NULL means 'default'.<br>+ * An test value of NULL means 'default'.<br> */<br>- ast_expression *expression;<br>
+ ast_expression *test_value;<br> };<br><br>+<br> class ast_selection_statement : public ast_node {<br> public:<br> ast_selection_statement(ast_expression *condition,<br>@@ -653,8 +659,21 @@ public:<br><br> class ast_switch_statement : public ast_node {<br>
public:<br>- ast_expression *expression;<br>- exec_list statements;<br>+ ast_switch_statement(ast_expression *test_expression,<br>+ ast_node *body);<br>+ virtual void print(void) const;<br>+<br>
+ virtual ir_rvalue *hir(exec_list *instructions,<br>+ struct _mesa_glsl_parse_state *state);<br>+<br>+ ast_expression *test_expression;<br>+ ast_node *body;<br>+<br>+ ir_variable *test_var;<br>
</blockquote><br></div></div>Presumably this is "x" in "switch (x) { ... }"? That can be an arbitrary scalar integer expression, not just a variable.
<div class="im"><br><br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">+ ir_variable *fallthru_var;<br>+<br>+protected:<br>+ void test_to_hir(class ir_loop *, struct _mesa_glsl_parse_state *);<br>
};<br></blockquote><br></div>I don't think we should be putting ir_variables (or any HIR code!) in the AST structures. The AST should really just be an abstract syntax tree, representing the text and structure of the program, but independent of any IR code we might generate.<br>
<br>I had imagined creating some kind of IR structure along the lines of:<br><br>class ir_switch : public ir_instruction<br>{<br> ir_rvalue *switch_value; // switch (...) - arbitrary expression<br><br> exec_list cases; // list of ir_case<br>
};<br><br>class ir_case : public exec_node<br>{<br> ir_rvalue *test_value; // case X: ... NULL means "default:"<br> exec_list body; // statements inside this case<br> // possibly a bool has_break or falls_through, if helpful<br>
};<br><br>This seems pretty straightforward to generate at AST->HIR time.<br><br>Then there would be a lowering pass (see lower_*.cpp) based on an ir_hierarchical_visitor that visits ir_switch, replacing it with the loop/if structure. ir_variable *fallthru_var would fit nicely as a member of that visitor. visit(ir_switch *) would create/emit it, and visit(ir_case *) would reference it.<br>
<br>Of course, there may be another solution; it's just an idea.<br><font color="#888888"><br>--Kenneth<br></font></blockquote></div><br>