On 11/15/12, Michael Matz <m...@suse.de> wrote:
> On Wed, 14 Nov 2012, Lawrence Crowl wrote:
> > Diego and I seek your comments on the following (loose) proposal.
>
> In principle I agree with the goal, I'm not sure I like the
> specific way yet, and even if I do I have some suggestions:
>
> > We will add a set of helper classes to be used as local variables
> > to manage the details of handling the existing types.
>
> I think one goal should be to minimize the number of those
> helper classes if we can. And use clear names, for the statement
> builder e.g. stmt_builder, or the like, not just ssa_seq.
The helper classes provide benefits.
They allow us to keep state needed to tie actions together.
Without that state, we would either require the user to create their
own extra variables, or require extending the representation of
the primary data structures. The former is a programming burden,
the latter is a space problem.
They allow us to use the same name for the same actions in two
different contexts. In particular, distinguishing between statement
construction in SSA and non-SSA.
> > We propose a simplified form using new build helper classes
> > ssa_seq and ssa_stmt that would allow the above code to be
> > written as follows.
> >
> > ssa_seq q;
> > ssa_stmt t = q.stmt (NE_EXPR, shadow, 0);
> > ssa_stmt a = q.stmt (BIT_AND_EXPR, base_addr, 7);
> > ssa_stmt b = q.stmt (shadow_type, a);
>
> I think consistency should trump brevity here, so also add a tree
> code for the converter, i.e.
>
> ssa_stmt b = q.stmt (NOP_EXPR, shadow_type, a);
Personally, I found that "NOP_EXPR" was confusing, because I was
expecting a cast operation. My expectation is that folks learning
GCC would have a lower hurdle without the misdirection. However,
I don't have strong feelings here.
> The method name should imply the action, e.g. 'add_stmt' or
> append_stmt or the like.
I was thinking more declaratively, but making it a verb is okay
with me.
> I'm not sure if we need the ssa_stmt class. We could use
> overloading to accept 'gimple' as operands, with the understanding
> that those will be implicitely converted to 'tree' by accessing
> the LHS:
>
> gimple append_stmt (gimple g, tree_code code, gimple op1, tree op2)
> {
> return append_stmt (g, code, gimple_lhs (op1), op2);
> }
>
> (where gimple_lhs would ICE when the stmt in question doesn't have
> one). As gimple statements are their own iterators meanwhile I'm
> not even sure if you need the ssa_seq (or ssa_stmt_builder) class.
> The above append_stmt would simply add the newly created statement
> to after 'g'.
Yes, simplifications like that were the intent.
> That is, I'm not yet convinced that you really need any local
> data for which you'd have to add helper classes.
We also need to ask if we will ever need local data. If we plan
for it now, future changes might be possible without affecting
existing code. Otherwise, we might require more substantial patches.
> So, you solve multiple problems (and I agree that they are
> problems), and these are:
>
> > There are a few important things to note about this example.
> >
> > .. We have a new class (ssa_seq) that knows how to sequence
> > statements automatically and can build expressions out of types.
>
> 1) automatic sequencing; I think we don't need a helper class
> for that, just new helper functions.
>
> > .. Every statement created produces an SSA name. Referencing an
> > ssa_stmt instance in a an argument to ssa_seq::stmt retrieves
> > the SSA name generated by that statement.
>
> 2) gimple with LHS and their LHS are isomorphic; I think overloads
> will solve this as well without a wrapper class.
>
> > .. The statement result type is that of the arguments.
>
> 3) easy stmt building
> 3a) type inference; I agree with all your rules
>
> > .. The type of integral constant arguments is that of the
> > other argument. (Which implies that only one argument can
> > be constant.)
> >
> > .. The 'stmt' method handles linking the statement into the
> > sequence.
>
> See 1)
>
> > .. The 'set_location' method iterates over all statements.
>
> So it iterates from the starting point of the ssa_seq to its end
> (presumably only overriding locations that don't yet have one).
> This can also be implemented by remembering the starting point
> of the gimple sequence (I'd remember a gimple stmt, you have to
> remember the ssa_seq object, so it's the same there).
>
> All in all I think we can severely improve on building gimple
> statements without introduction of any helper class. Basically,
> whenever a helper class merely contains one member of a different
> type, then I think that other type should be improved so that
> the wrapper class isn't necessary anymore. Fewer types -> good :)
While I would agree that unnecessary types are bad, I have found
that having types to represent distinct concepts is a good way to
structure my thinking and catch my errors.
> > Generating a Type
>
> Apart from naming of some methods to imply the action done,
> I don't have any opinion about this at this point. Though I
> agree that again the boilerplate sequencing (_CONTEXT and _CHAIN)
> should be improved.
--
Lawrence Crowl
