From: kenner@vlsi1.ultra.nyu.edu (Richard Kenner)
To: mark@codesourcery.com
Cc: gcc-patches@gcc.gnu.org
Subject: Re: Patch to allow Ada to work with tree-ssa
Date: Tue, 22 Jun 2004 16:33:00 -0000 [thread overview]
Message-ID: <10406221307.AA05385@vlsi1.ultra.nyu.edu> (raw)
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.
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.
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.
You also didn't add accessor macros for these additional operands, so
the only place to look is at the documentation you added in tree.def,
which suggests that these additional operands are redundant;
I added accessor *functions* for each of these:
array_ref_low_bound,
array_ref_element_size, and
component_ref_field_offset
They are supposed to be used (and are used) whenever those fields
are needed.
We don't have accessor macros for any other fields of references or
expressions, so I didn't add them for this. I would be in favor of
adding accesor macros for all expressions (e.g. MODIFY_EXPR_RHS) to
include these, but I think it would be very peculiar to have an
accessor macro for operands 2 and 3 of an ARRAY_REF when we don't have
them for operands 0 and 1! That's why I didn't add them.
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.
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.
If that's what this is about, then I suggest you find a way to make
these nodes bigger in Ada than it is in C/C++ and only look for the
additional fields there, or have a flag in ARRAY_REF that indicates
whether these additional fields are present.
It's not C vs. Ada but fixed vs. variable. Most arrays and components in Ada
are fixed size, just like in C. It's more an issue of *frequency*. In C and
C++, variable-sized stuff is quite rare, while in Ada it's more common.
We don't have a mechanism to have a tree code be of two different sizes
depending on the setting of a flag, so we'd have to add one. We could have
two different tree codes, but that would complicate the optimizers and is
exactly what this mechanism is trying to avoid.
Please revert this patch until we've got consensus here.
I don't want to do that. It's a large patch and I have further changes that
I'll be making on top of it and reverting it at this point would bring my
work to a stop for an indefinite amount of time.
We *had* concensus. This approach was discussed *weeks* ago and nobody
objected, including you. I wouldn't have spent weeks implementing and
testing this if I didn't feel everybody agreed on it.
Look, for example, at http://gcc.gnu.org/ml/gcc/2004-06/msg00459.html
That was over two weeks ago and was the time to object.
Yes, it adds some more memory utilization, and yes, that's not good. But
it's very small. According to Nathan and Zack's paper, expression nodes in
total amount to 19% of memory utilization in C++ programs.
An expression node has quite a large number of fields. The common part is
four words. The expression adds another four words plus the operands. So
ARRAY_REF used to be 12 words and is now 14, an increase of 17%.
COMPONENT_REF used to be 12 and is now 13, an increase of 8%. I don't know
how common those nodes are in expressions, but in GIMPLE, almost half of all
expr nodes are MODIFY_EXPR.
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.
Nathan and Zack are working on redoing the tree structure anyway so this can
get taken care of then.
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.
next reply other threads:[~2004-06-22 13:06 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-06-22 16:33 Richard Kenner [this message]
2004-06-22 17:46 ` Mark Mitchell
2004-06-22 18:17 ` Joseph S. Myers
2004-06-22 18:52 ` Richard Henderson
2004-06-22 19:37 ` Zack Weinberg
-- strict thread matches above, loose matches on Subject: below --
2004-07-29 19:02 Richard Kenner
2004-07-29 18:43 Richard Kenner
2004-07-29 17:12 Richard Kenner
2004-07-29 17:13 ` Diego Novillo
2004-07-29 17:57 ` Richard Henderson
2004-06-25 15:28 Richard Kenner
2004-06-28 15:09 ` Paolo Bonzini
2004-06-24 15:58 Richard Kenner
2004-06-24 16:04 ` Nathan Sidwell
2004-06-25 14:43 ` Joseph S. Myers
2004-06-23 5:54 Richard Kenner
2004-06-23 2:32 Richard Kenner
2004-06-23 4:54 ` Bryce McKinlay
2004-06-23 0:16 Richard Kenner
2004-06-22 23:05 Richard Kenner
2004-06-23 11:42 ` Nathan Sidwell
2004-06-22 22:18 Richard Kenner
2004-06-23 1:07 ` Richard Henderson
2004-06-22 21:30 Richard Kenner
2004-06-22 22:04 ` Paul Brook
2004-06-22 21:27 Richard Kenner
2004-06-22 21:29 ` Mark Mitchell
2004-06-22 21:10 Richard Kenner
2004-06-22 21:12 ` Mark Mitchell
2004-06-22 22:16 ` Daniel Berlin
2004-06-22 21:07 Richard Kenner
2004-06-22 21:12 ` Bryce McKinlay
2004-06-22 21:05 Richard Kenner
2004-06-22 21:01 Richard Kenner
2004-06-22 20:54 Richard Kenner
2004-06-22 21:06 ` Paul Brook
2004-06-22 21:37 ` Richard Henderson
2004-06-22 20:44 Richard Kenner
2004-06-22 21:03 ` Mark Mitchell
2004-06-23 20:58 ` Geoffrey Keating
2004-06-22 20:40 Richard Kenner
2004-06-22 22:02 ` Nathan Sidwell
2004-06-22 22:27 ` Joseph S. Myers
2004-06-22 20:23 Richard Kenner
2004-06-22 20:37 ` Joseph S. Myers
2004-06-23 20:47 ` Geoffrey Keating
2004-06-24 14:59 ` Joseph S. Myers
2004-06-24 16:26 ` Richard Earnshaw
2004-06-24 19:57 ` Laurent GUERBY
2004-06-24 20:06 ` Diego Novillo
2004-06-24 20:24 ` Andrew Pinski
2004-06-24 22:35 ` Laurent GUERBY
2004-06-24 21:33 ` Laurent GUERBY
2004-06-24 21:01 ` Joseph S. Myers
2004-06-25 14:51 ` Richard Earnshaw
2004-06-22 20:22 Richard Kenner
2004-06-22 20:30 ` Mark Mitchell
2004-06-22 19:05 Richard Kenner
2004-06-22 20:10 ` Joseph S. Myers
2004-06-22 20:27 ` Andrew Haley
[not found] <10406221359.AA05860@vlsi1.ultra.nyu.edu>
2004-06-22 18:47 ` Richard Henderson
2004-06-22 18:33 Richard Kenner
2004-06-22 18:19 Richard Kenner
2004-06-22 18:36 ` Nathan Sidwell
2004-06-22 18:37 ` Nathan Sidwell
2004-06-22 18:45 ` Mark Mitchell
2004-06-22 17:05 Richard Kenner
2004-06-22 17:21 ` Andrew Haley
2004-06-22 19:01 ` Richard Henderson
2004-06-22 16:34 Richard Kenner
2004-06-22 7:05 Richard Kenner
2004-06-22 7:29 ` Mark Mitchell
2004-06-22 9:14 ` Andrew Pinski
2004-06-22 14:16 ` Andrew Haley
2004-06-22 11:00 ` Richard Henderson
2004-06-22 13:52 ` Ranjit Mathew
2004-07-29 17:02 ` 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=10406221307.AA05385@vlsi1.ultra.nyu.edu \
--to=kenner@vlsi1.ultra.nyu.edu \
--cc=gcc-patches@gcc.gnu.org \
--cc=mark@codesourcery.com \
/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).