[Mesa-dev] [PATCH 3/5] glsl: Convert ir_call to be a statement rather than an rvalue.

Kenneth Graunke kenneth at whitecape.org
Wed Sep 21 16:50:16 PDT 2011

On 09/21/2011 01:47 PM, Paul Berry wrote:
> On 20 September 2011 18:28, Kenneth Graunke wrote:
>     diff --git a/src/glsl/ir_constant_expression.cpp
>     b/src/glsl/ir_constant_expression.cpp
>     index 4264847..bc0b589 100644
>     --- a/src/glsl/ir_constant_expression.cpp
>     +++ b/src/glsl/ir_constant_expression.cpp
>     @@ -983,9 +983,6 @@ ir_constant::constant_expression_value()
>      ir_constant *
>      ir_call::constant_expression_value()
>      {
>     -   if (this->type == glsl_type::error_type)
>     -      return NULL;
>     -
>        return
>     this->callee->constant_expression_value(&this->actual_parameters);
>      }
> I believe the deleted if-test needs to be moved into
> ir_constant::constant_expression_value().

Urgh.  Yes, it does need to be added in patch 1/5 to avoid changing
behavior (let's not constant fold error_type values).  I feel dirty
about that, making ir_constant::constant_expression_value() potentially
return NULL.  Seems wrong.  But I really don't want to clean up
error_type today.  So...probably just need to add it there.

>     diff --git a/src/glsl/ir_hv_accept.cpp b/src/glsl/ir_hv_accept.cpp
>     index d33fc85..f8cd776 100644
>     --- a/src/glsl/ir_hv_accept.cpp
>     +++ b/src/glsl/ir_hv_accept.cpp
>     @@ -317,6 +317,12 @@ ir_call::accept(ir_hierarchical_visitor *v)
>        if (s != visit_continue)
>           return (s == visit_continue_with_parent) ? visit_continue : s;
>     +   if (this->return_deref != NULL) {
>     +      s = this->return_deref->accept(v);
>     +      if (s != visit_continue)
>     +        return (s == visit_continue_with_parent) ? visit_continue : s;
>     +   }
>     +
> I think we need to set in_assignee here (in much the same way that
> ir_assignment::accept() does for the LHS of an assignment).  But I'm not
> 100% certain about that, since we don't do it for out and inout
> parameters (I wouldn't be surprised if there are some lurking bugs
> because of this).

Yeah, I agree...we ought to set that.  I didn't know it existed.  I've
updated the patch to do so.

>     diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
>     index 2d1c609..848305c 100644
>     --- a/src/glsl/ir_validate.cpp
>     +++ b/src/glsl/ir_validate.cpp
>     @@ -537,6 +537,18 @@ ir_validate::visit_enter(ir_call *ir)
>      {
>        ir_function_signature *const callee = ir->get_callee();
>     +   if (callee->return_type != glsl_type::void_type) {
>     +      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();
>     +      }
>     +   }
>     +
> Validation should also fail if callee->return_type ==
> glsl_type::void_type but ir->return_deref != NULL.

Yes, it should.  Updated, thanks.

> Are we making it a rule that an ir_call's return_deref needs to always
> be an ir_dereference_variable?  If so, we should probably use the more
> specific type in the class declaration (or at the very least add code to
> ir_validate.cpp to check this).  If not, then a few other visitors need
> to be updated:

I was wondering about that, actually.  I guess I left it as a general
dereference since there's no reason you couldn't:

(call (array_ref (var_ref foo) (constant int (0))) cos (...))

...but we certainly never generate that, so...maybe it's best to just
keep it simple.

I also was wondering if call should have a writemask, like assign.  I
guess I figured omitting, since in the backends, register coalescing &
allocation ought to remove the extra temporary & assignment anyway.

Paul, Eric, what do you think?

> - ir_vec_index_to_cond_assign_visitor: do the appropriate lowering on an
> ir_call's return_deref.
> - ir_vec_index_to_swizzle_visitor: do the appropriate lowering on an
> ir_call's return_deref.

Yeah, I suppose return_deref could be foo[i] a.k.a. (array_ref (var_ref
foo) (var_ref i)), and we would need to lower that.

Of course, if return_deref was an ir_dereference_variable as you
suggested, this wouldn't be necessary.  That's sounds more appealing
than adding complexity to these passes for something we don't really use

> Also, regardless of the type of return_deref, I think these visitors
> need to be updated:
> - find_assignment_visitor (in linker.cpp): needs to check ir_call's
> return_deref to see if it is the variable being sought.

Good catch.  Updated.

> - ir_tree_grafting_visitor: kill the graft if an ir_call's return_deref
> refers to a variable that's present in the expression being grafted.

I think you're right; I'll look into this.

> - loop_analysis: in the visitor for ir_dereference_variable, handle the
> case where in_assignee is true but we're inside a return_deref.

Ugh.  Good catch...I'll look into this as well.

> - discard_simplifier, ir_if_simplification_visitor, and
> redundant_jumps_visitor: add a visit_enter(ir_call *) function that
> returns visit_continue_with_parent, just like those visitors do for
> ir_assignment.

Not strictly required; this just avoids extra pointless walking around
the tree, making those passes run faster.  I'll add that in a follow-up
patch, or perhaps just fix the visitors.  I've got some ideas for that
which might be doable and even better.

> Finally, I was kind of surprised to discover that the visitor classes
> ir_to_mesa_visitor and glsl_to_tgsi_visitor have visit methods for
> ir_call.  I thought we inlined everything before converting to Mesa IR
> or TGSI.  So maybe these visit methods are dead code?  If they are, we
> should probably replace the dead code with assert(!"not reached").  If
> they aren't, then they need to be updated to handle return_deref properly.

Both IRs supposedly support function calls, so I imagine the ir_call
visiting code is trying to support that.  Of course, the comment in the
ir_function visiting code doesn't inspire much confidence:

/* Ignore function bodies other than main() -- we shouldn't see calls to
 * them since they should all be inlined before we get to [...]

I think this means that it's broken and doesn't work.  People should fix
this for TGSI someday.  Mesa IR is transitioning to be a i915/r200/nv20
IR, none of which support control flow, so we can just rip that out
without any qualms.

Not sure what to do about it right now.  Eric, Bryan?

More information about the mesa-dev mailing list