On Wed, 28 Mar 2012 20:33:06 -0700, Kenneth Graunke <kenn...@whitecape.org>
wrote:
> Aside from ir_call, our IR is cleanly split into two classes:
> - Statements (typeless; used for side effects, control flow)
> - Values (deeply nestable, pure, typed expression trees)
> diff --git a/src/glsl/ir_basic_block.cpp b/src/glsl/ir_basic_block.cpp
> index a833825..5ebbf6f 100644
> --- a/src/glsl/ir_basic_block.cpp
> +++ b/src/glsl/ir_basic_block.cpp
> @@ -122,24 +97,9 @@ void call_for_basic_blocks(exec_list *instructions,
>
> call_for_basic_blocks(&ir_sig->body, callback, data);
> }
> - } else if (ir->as_assignment()) {
> - /* If there's a call in the expression tree being assigned,
> - * then that ends the BB too.
> - *
> - * The assumption is that any consumer of the basic block
> - * walker is fine with the fact that the call is somewhere in
> - * the tree even if portions of the tree may be evaluated
> - * after the call.
> - *
> - * A consumer that has an issue with this could not process
> - * the last instruction of the basic block. If doing so,
> - * expression flattener may be useful before using the basic
> - * block finder to get more maximal basic blocks out.
> - */
> - if (ir_has_call(ir)) {
> - callback(leader, ir, data);
> - leader = NULL;
> - }
> + } else if (ir->as_call()) {
> + callback(leader, ir, data);
> + leader = NULL;
There was already a block checking for ir->as_call() above, so this can
just be dropped.
> diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
> index 101d999..385ef12 100644
> --- a/src/glsl/ir_validate.cpp
> +++ b/src/glsl/ir_validate.cpp
> @@ -548,6 +548,23 @@ ir_validate::visit_enter(ir_call *ir)
> abort();
> }
>
> + if (callee->return_type == glsl_type::void_type) {
> + if (ir->return_deref != NULL) {
> + printf("ir_call to void function has storage for a return value\n");
> + abort();
> + }
> + } else {
> + if (ir->return_deref == NULL) {
> + printf("ir_call has non-void callee but no return storage\n");
> + abort();
> + }
> + if (callee->return_type != ir->return_deref->type) {
> + printf("callee type %s does not match return storage type %s\n",
> + callee->return_type->name, ir->return_deref->type->name);
> + abort();
> + }
> + }
I think this could be slightly simplified:
if (ir->return_deref) {
if (ir->return_deref->type != callee->return_type) {
printf("callee type %s does not match return storage type %s\n",
callee->return_type->name, ir->return_deref->type->name);
abort();
}
} else if (callee->return_type != glsl_type::void_type) {
printf("ir_call has non-void callee but no return storage\n");
abort();
}
but either way.
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 09ffdff..d56eb97 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -117,6 +117,15 @@ public:
> sig_iter.next();
> }
>
> + if (ir->return_deref != NULL) {
> + ir_variable *const var = ir->return_deref->variable_referenced();
= ir->return_deref->var;
Every time I see variable_referenced(), I think about what kind of
rvalue tree it is and the possibility of a NULL return.
Actually, I think there's a later patch opportunity to simplify this
visitor a bunch of the optimization passes by setting in_assignee
during function out/inout parameter visiting, and then dropping a
bunch of ir_call special cases in favor of the
ir_dereference_variable() visit function with an in_assignee check.
You can bump my Reviewed-by up to the current version.
pgp8XWDy60kzI.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
