<div class="gmail_quote">On 21 September 2011 18:44, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA1<div class="im"><br>
<br>
On 09/20/2011 06:28 PM, Kenneth Graunke wrote:<br>
| This begins the process of cleaning up and un-muddling our IR.<br>
|<br>
| Aside from ir_call, our IR is cleanly split into two classes: -<br>
| Statements (typeless; used for side effects, control flow) - Values<br>
| (deeply nestable, pure, typed expression trees)<br>
|<br>
| Unfortunately, ir_call confused all this: - For void functions, we<br>
| placed ir_call directly in the instruction stream, treating it as<br>
| an untyped statement. Yet, it was a subclass of ir_rvalue, and no<br>
| other ir_rvalue could be used in this way. - For functions with a<br>
| return value, ir_call could be placed in arbitrary expression<br>
| trees. While this fit naturally with the source language, it meant<br>
| that expressions might not be pure, making it difficult to<br>
| transform and optimize them. To combat this, we always emitted<br>
| ir_call directly in the RHS of an ir_assignment, only using a<br>
| temporary variable in expression trees. Many passes relied on<br>
| this assumption; the acos and atan built-ins violated it.<br>
|<br>
| This patch makes ir_call a statement (ir_instruction) rather than<br>
| a value (ir_rvalue). Non-void calls now take a ir_dereference of<br>
| a variable, and store the return value there---effectively a call<br>
| and assignment rolled into one. They cannot be embedded in<br>
| expressions.<br>
<br></div>
It seems like a lot of the complexity in this patch (and the review<br>
comments) comes from having to duplicate ir_assignment code. I<br>
haven't thought it through, but, is there a way to make an<br>
ir_call_assignment that shares a common parent with ir_assignment.<br>
This would necessitate an ir_call (without assignment), and that might<br>
make things worse. Thoughts?</blockquote><div><br></div><div>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.</div>
<div><br></div><div>I can see two ways the class hierarchy might work out. Let's call the common base class between calls and assignments "ir_callsignment":</div><div><br></div><div>A.</div><div>- ir_statement (base class)</div>
<div> - ir_callsignment (base class)</div><div> - ir_call_assignment (for calling functions that return non-void)</div><div> - ir_assignment</div><div> - ir_call_without_assignment (for calling functions that return void)</div>
<div><br></div><div>B.</div><div>- ir_statement (base class)</div><div> - ir_callsignment (base class)</div><div> - ir_call</div><div> - ir_assignment</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
</div>