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

[Paul Berry]

On 21 September 2011 18:44, Ian Romanick <idr at freedesktop.org> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> On 09/20/2011 06:28 PM, Kenneth Graunke wrote:
> | This begins the process of cleaning up and un-muddling our IR.
> |
> | 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)
> |
> | Unfortunately, ir_call confused all this: - For void functions, we
> | placed ir_call directly in the instruction stream, treating it as
> | an untyped statement.  Yet, it was a subclass of ir_rvalue, and no
> | other ir_rvalue could be used in this way. - For functions with a
> | return value, ir_call could be placed in arbitrary expression
> | trees.  While this fit naturally with the source language, it meant
> | that expressions might not be pure, making it difficult to
> | transform and optimize them.  To combat this, we always emitted
> | ir_call directly in the RHS of an ir_assignment, only using a
> | temporary variable in expression trees.  Many passes relied on
> | this assumption; the acos and atan built-ins violated it.
> |
> | This patch makes ir_call a statement (ir_instruction) rather than
> | a value (ir_rvalue).  Non-void calls now take a ir_dereference of
> | a variable, and store the return value there---effectively a call
> | and assignment rolled into one.  They cannot be embedded in
> | expressions.
>
> It seems like a lot of the complexity in this patch (and the review
> comments) comes from having to duplicate ir_assignment code.  I
> haven't thought it through, but, is there a way to make an
> ir_call_assignment that shares a common parent with ir_assignment.
> This would necessitate an ir_call (without assignment), and that might
> make things worse.  Thoughts?


Do you mean "ir_call_assigment" to refer to the common parent class, or to
refer to a specific type of "ir_call" that also assigns its return value?  I
think I understand you to mean the latter.

I can see two ways the class hierarchy might work out.  Let's call the
common base class between calls and assignments "ir_callsignment":

A.
- ir_statement (base class)
    - ir_callsignment (base class)
        - ir_call_assignment (for calling functions that return non-void)
        - ir_assignment
    - ir_call_without_assignment (for calling functions that return void)

B.
- ir_statement (base class)
    - ir_callsignment (base class)
        - ir_call
        - ir_assignment

In both cases, ir_callsignment would contain the thing that calls and
assignments have in common (the variable that the return value is stored in,
in the case of calls, and the LHS, in the case of assignments).  In case A,
that thing would not be allowed to be NULL.  In case B, it would be allowed
to be NULL, but only when the derived class was an ir_call to a function
returning void.

My preference is for class hierarchy B.  The problem with A is that there
would be a lot of code duplication between visitors acting on
ir_call_assignment and visitors acting on ir_call_without_assignment.  I
don't think it would be worth it.

B, on the other hand, might be just crazy enough to work.  In fact, that
with appropriate cleverness in the ir_hierarchical_visitor base class, we
could probably set things up so that visitors that needed to treat calls and
assignments differently could do so, but visitors that didn't could just
implement visit_enter(ir_callsignment *) and visit_leave(ir_callsignment *),
and use the same code to handle both.  We might need to put some virtual
methods in ir_callsignment to allow those visitors that wanted to treat
calls and assignments the same to do so.  I would have to spend some time
experimenting to work out all the details.

IMHO, Ken's patch is still worth accepting on its own merits, in spite of
the code duplication issue, because it gets rid of an invariant that was
undocumented, unverified, occasionally violated, and relied upon by lots of
code, and because it makes the class hierarchy more like B (by making
ir_call a type of statement).  So I think we should accept it (once the
noted fixes are made), and consider Ian's idea as a possible follow-up
improvement.

Another related improvement we might consider is to see if there is a way to
unify the handling of out parameters with the handling of assignment LHS's.
 I suspect that we have some lurking bugs in how our optimization and
lowering passes treat out parameters, because they aren't anywhere near as
thoroughly tested as assignments.  If we can figure out a way to use the
same code on both out parameters and LHS's, that might be just as big a win
as unifying calls and assignments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110921/65e171f9/attachment.html>