[Mesa-dev] [PATCH 1/7] Create AST data structures for switch statement and case label

[Dan McCabe]

A valid criticism. It hadn't occured to me at the time, but you are right.

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.

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

I'll look at it in more detail when I continue that work on Friday.

Thanks.

cheers, danm

On Wed, Jun 15, 2011 at 11:59 AM, Kenneth Graunke <kenneth at whitecape.org>wrote:

> On 06/15/2011 09:33 AM, Dan McCabe wrote:
>
>> Data structures for switch statement and case label are created that
>> parallel
>> the structure of other AST data.
>> ---
>>  src/glsl/ast.h |   27 +++++++++++++++++++++++----
>>  1 files changed, 23 insertions(+), 4 deletions(-)
>>
>
> Dan, thanks for sending this out!  A few comments below...
>
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
>> index 878f48b..48d1795 100644
>> --- a/src/glsl/ast.h
>> +++ b/src/glsl/ast.h
>> @@ -628,13 +628,19 @@ public:
>>
>>  class ast_case_label : public ast_node {
>>  public:
>> +   ast_case_label(ast_expression *test_value);
>> +   virtual void print(void) const;
>> +
>> +   virtual ir_rvalue *hir(exec_list *instructions,
>> +                         struct _mesa_glsl_parse_state *state);
>>
>>     /**
>> -    * An expression of NULL means 'default'.
>> +    * An test value of NULL means 'default'.
>>      */
>> -   ast_expression *expression;
>> +   ast_expression *test_value;
>>  };
>>
>> +
>>  class ast_selection_statement : public ast_node {
>>  public:
>>     ast_selection_statement(ast_expression *condition,
>> @@ -653,8 +659,21 @@ public:
>>
>>  class ast_switch_statement : public ast_node {
>>  public:
>> -   ast_expression *expression;
>> -   exec_list statements;
>> +   ast_switch_statement(ast_expression *test_expression,
>> +                       ast_node *body);
>> +   virtual void print(void) const;
>> +
>> +   virtual ir_rvalue *hir(exec_list *instructions,
>> +                         struct _mesa_glsl_parse_state *state);
>> +
>> +   ast_expression *test_expression;
>> +   ast_node *body;
>> +
>> +   ir_variable *test_var;
>>
>
> Presumably this is "x" in "switch (x) { ... }"?  That can be an arbitrary
> scalar integer expression, not just a variable.
>
>
> +   ir_variable *fallthru_var;
>> +
>> +protected:
>> +   void test_to_hir(class ir_loop *, struct _mesa_glsl_parse_state *);
>>  };
>>
>
> 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.
>
> I had imagined creating some kind of IR structure along the lines of:
>
> class ir_switch : public ir_instruction
> {
>        ir_rvalue *switch_value; // switch (...) - arbitrary expression
>
>        exec_list cases; // list of ir_case
> };
>
> class ir_case : public exec_node
> {
>        ir_rvalue *test_value; // case X: ... NULL means "default:"
>        exec_list body; // statements inside this case
>        // possibly a bool has_break or falls_through, if helpful
> };
>
> This seems pretty straightforward to generate at AST->HIR time.
>
> 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.
>
> Of course, there may be another solution; it's just an idea.
>
> --Kenneth
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110615/c6432c10/attachment.htm>