From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32480 invoked by alias); 22 Jun 2004 13:06:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 32343 invoked from network); 22 Jun 2004 13:05:59 -0000 Received: from unknown (HELO vlsi1.ultra.nyu.edu) (128.122.140.213) by sourceware.org with SMTP; 22 Jun 2004 13:05:59 -0000 Received: by vlsi1.ultra.nyu.edu (4.1/1.34) id AA05385; Tue, 22 Jun 04 09:07:30 EDT Date: Tue, 22 Jun 2004 16:33:00 -0000 From: kenner@vlsi1.ultra.nyu.edu (Richard Kenner) Message-Id: <10406221307.AA05385@vlsi1.ultra.nyu.edu> To: mark@codesourcery.com Subject: Re: Patch to allow Ada to work with tree-ssa Cc: gcc-patches@gcc.gnu.org X-SW-Source: 2004-06/txt/msg01745.txt.bz2 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.