Re: [PATCH]: GVN-PRE

[Daniel Berlin <dberlin at dberlin dot org>]

On Jun 15, 2004, at 11:15 AM, Mark Mitchell wrote:

> >    For debugging reasons, the value handle is internally more than
> >    just a number, it is a VAR_DECL named "value.x", where x is a
> >    unique number for each value number in use.  This allows
> >    expressions with SSA_NAMES replaced by value handles to still be
> >    pretty printed in a sane way.  They simply print as "value.3 *
> >    value.5", etc.
>
> This is a bad idea in the longer term.  VAR_DECLs are big; we're 
> wasting a lot of memory here.
Yes, i'm aware.  It was still using a lot less memory than SSAPRE used 
to (since it used to possibly create multiple tree nodes per variable), 
and it was a first step.

> The debugging feature is nice, but either this should only be done in 
> debug builds, or we should use a smarter data structure.  All you 
> really need is a two-element struct with a pointer to the VAR_DECL and 
> the value number.
>
> > /* A value set element.  Basically a single linked list of
> >    expressions/constants/values.  */
> > typedef struct value_set_node
> > {
> >   tree expr;
> >   struct value_set_node *next;
> > } *value_set_node_t;
>
> All these structure definitions should have documentation for the 
> fields.  Yes, sometimes it's pretty obvious.  It's still useful.

Okeydokey.

> This comment is insufficient:
>
> > /* Are {e2,v2} and {e1,v1} the same?  Again, only the expression
> >    matters.  */
>
> You need to explain what each parameter and the return value are.  Is 
> this returning a boolean value or an integer?  What are "e2" and "v2" 
> -- I only see "p1" and "p2", until looking at the body of the code.

I'll update this comment, and verify the others are still sane/not 
missing.
I had gone through most of them before it was committed, but it's 
obvious i must have missed a few.
--Dan