public inbox for gcc@gcc.gnu.org
 help / color / mirror / Atom feed
From: Michael Matz <matz@suse.de>
To: Lawrence Crowl <crowl@googlers.com>
Cc: "gcc@gcc.gnu.org" <gcc@gcc.gnu.org>
Subject: Re: Simplifying Gimple Generation
Date: Thu, 15 Nov 2012 14:48:00 -0000	[thread overview]
Message-ID: <alpine.LNX.2.00.1211151523550.21868@wotan.suse.de> (raw)
In-Reply-To: <CAGqM8fZbnET=4GGrBt-sXJ-jweTxYYLAGdaYuzK2HfiwCi7K=A@mail.gmail.com>

Hi Lawrence,

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.

> 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);

The method name should imply the action, e.g. 'add_stmt' or append_stmt 
or the like.  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'.

That is, I'm not yet convinced that you really need any local data for 
which you'd have to add helper classes.

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 :)

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


Ciao,
Michael.

  parent reply	other threads:[~2012-11-15 14:48 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-15  1:13 Lawrence Crowl
2012-11-15  6:06 ` Basile Starynkevitch
2012-11-15 19:34   ` Lawrence Crowl
2012-11-16 13:15   ` Diego Novillo
2012-11-15  7:32 ` Xinliang David Li
2012-11-16 13:13   ` Diego Novillo
2012-11-17  1:05     ` Xinliang David Li
2012-11-15 14:48 ` Michael Matz [this message]
2012-11-15 16:52   ` Gabriel Dos Reis
2012-11-15 17:01     ` Michael Matz
2012-11-15 17:07       ` Xinliang David Li
2012-11-15 19:59   ` Lawrence Crowl
2012-11-16 12:44     ` Michael Matz
2012-11-16 13:59   ` Diego Novillo
2012-11-16 14:30     ` Michael Matz
2012-11-16 17:31       ` Andrew Pinski
2012-11-20 16:49 ` Andrew MacLeod
2012-11-25  1:35 ` Diego Novillo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LNX.2.00.1211151523550.21868@wotan.suse.de \
    --to=matz@suse.de \
    --cc=crowl@googlers.com \
    --cc=gcc@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).