Re: Patch to allow Ada to work with tree-ssa

[Mark Mitchell <mark at codesourcery dot com>]

Richard Kenner wrote:

>    In particular, you've just added an extra entry to ARRAY_REF and 
>    COMPONENT_REF, making these two common tree nodes bigger.  
>    (COMPONENT_REF, in particular, is used extensively in C++; every access 
>    to a class member variable is a COMPONENT_REF.)  These extra entries are 
>    unnecessary in C and C++, meaning that for GCC's two most-used languages 
>    you've just increased memory usage with no benefit.  If you need
>    some additional stuff for Ada, please find a way that doesn't penalize
>    C and C++.
>
> I can show you a GNU C program that needs them as well.  We support
> variable-sized types.
>  
I'm not disputing that you fixed a bug with these patches; what I'm 
saying is that there must be a better way to fix it.

> This issue was discussed extensively on the GCC list and nobody had a
> better idea as to how to do it.  Moreover, I sent email yesterday morning
> saying what my plans were and nobody objected.
>  
Many of us cannot read GCC mail on a continuous basis.  In any case, the 
fact that you provided notice doesn't alter the fact that you've 
increased the size of these nodes.

>    Furthermore, you didn't update the c-tree.texi documentation.  
>
> That's a C-specific file: these are language-independent nodes.
> I did document them in tree.def.
>  
It's was originally a C and C++-specific file; now, it's the closest 
thing we have to tree documentation.  It certainly documents ARRAY_REF 
and COMPONENT_REF.

>    for example, the third operand in a COMPONENT_REF is defined in terms
>    of the DECL_FIELD_OFFSET of the second operand; if that's true, why do
>    we need to have it there explicitly?  Presumably, this has something
>    to do with variable-sized types, but the documentation does not say
>    what that is.
>
> I'm not sure what you want and where.  The nodes are defined even for
> fixed records.
>  
In their plain English meaning, the words you wrote in tree.def make 
these fields redundant; you have defined the values of these fields in 
terms of information already available in the tree.  You've suggested 
that this is not actually true; that they are conveying some information 
not already present -- but the words you've written do not make clear 
what that is.  For example, "Operand 2 is a copy of TYPE_MIN_VALUE of 
the index."  OK, then, why don't we just do TYPE_MIN_VALUE (TREE_TYE 
(TREE_OEPRAND (x, 1)))?  Why store this in the node?  That is not 
explained in your documentation.

> As to getting the data from DECL_FIELD_OFFSET (or TYPE_MIN_VALUE), that was
> my original plan since I also didn't like the idea of of increasing the size
> of the node.  But *lots* of people were very much against that idea,
> including Diego, RTH, Jeff, and some I can't remember now.  I still was
> hoping for an approach like that, but then I realized that PLACEHOLDER_EXPR
> makes it *not* redundant in any case, so I did what others wanted me to
> do, which is perhaps this approach.
>  
I am very much against increasing the size of COMPONENT_REF and 
ARRAY_REF for C++.  I've suggested a technical solution: add a flag to 
the nodes that says whether they have all the fields, or just the old 
ones.  Presumably, it will be very rare in C/C++ to have all the 
fields.  True, we don't have that yet -- but, I'm confident you can find 
a way.

> Let's make the very pessimistic assumption that COMPONENT_REF are half of the
> remaining nodes and ARRAY_REF are half of what's left.  So 25% of expr nodes
> grow 8% and 12% grow 17%.  That's an increase of 4% in memory for expr nodes.
> Since they are 19% of all nodes, that's an overall memory increase of under
> 1%.  I think we can live with that.
>  
No, we can't.  It is precisely the 1% increases that have gotten us to 
the bloated state we are in now.  A patch a week with a 1% increase 
means that we get close to doubling the memory usage every year.  I just 
spent a few days eliminating roughly 1% of the memory used by C++, and 
I'm going to be aggressive aboiut preventing memory usage from increasing. 

I recognize that you estimated conservatively, but we should not be 
making changes that increase memory usage, especially not for permanent, 
uncollectable nodes which these will be; in C++, most expressions live 
for a long time, due to inlining and templates.

> If there is consensus after discussion that this patch is bad, I will, of
> course, revert it, but I'd *much* rather use it as a base and make
> improvements to it as needed, so progress can go forward, not backward.
>  
That's fine with me if you will agree to implement the optimization 
where these nodes are smaller when the extra fields are not needed, or 
implement new tree nodes for VARIABLE_ARRAY_REF or 
VARIABLE_COMPONENT_REF.  An even easier change would be to embed the 
additional fields in one of the existing operands; have the second 
operand be a TREE_LIST, or some such, in the case where there is a need 
for this additional information.  Ugly, but effective.

--
Mark Mitchell
CodeSourcery, LLC
(916) 791-8304
mark@codesourcery.com