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?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
