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

[Kenneth Graunke]

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
anyway.

> 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?