From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16554 invoked by alias); 26 Jun 2010 13:00:16 -0000 Received: (qmail 16532 invoked by uid 22791); 26 Jun 2010 13:00:14 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 26 Jun 2010 13:00:08 +0000 Received: (qmail 17807 invoked from network); 26 Jun 2010 13:00:06 -0000 Received: from unknown (HELO digraph.polyomino.org.uk) (joseph@127.0.0.2) by mail.codesourcery.com with ESMTPA; 26 Jun 2010 13:00:06 -0000 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.69) (envelope-from ) id 1OSUzN-0007bu-DJ; Sat, 26 Jun 2010 13:00:05 +0000 Date: Sat, 26 Jun 2010 15:51:00 -0000 From: "Joseph S. Myers" To: amylaar@spamcop.net cc: gcc-patches@gcc.gnu.org Subject: Re: RFA: Fix other/44566 In-Reply-To: <20100626082716.9ovcpv5qyso0sss8-nzlynne@webmail.spamcop.net> Message-ID: References: <20100626061802.45a3nzzz4088o840-nzlynne@webmail.spamcop.net> <20100626082716.9ovcpv5qyso0sss8-nzlynne@webmail.spamcop.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2010-06/txt/msg02670.txt.bz2 On Sat, 26 Jun 2010, amylaar@spamcop.net wrote: > In order to get declarations into the right scope for TARGET_SEPCIFIC / > EXTRA_TARGET code, I have moved around a few lines, but in general I have > preferred to put in more markers - that results in smaller diffs and is > thus easier to merge forward and review. I think it would be useful to > group things -including function definitions - better together in the > future, but that could only be done in small chunks - like one file at > a time. Trying to do that in a global patch is not feasible. Such rearrangements should come *first* as preparatory patches (many small patches rather than one large patch). > > How have you validated this patch? Have you for example built a large > > body of code with two single-target compilers and for both targets of a > > two-target compiler and verified that the code is identical for both ways > > of building for a given target? > > No, that is not even intended to work. The idea is that you have a main > target, and code compiled for this should be the same as if it was the only > target, unless specific optimizations are enabled that shift code from one > target to the other. > Because the main target governs things like type sizes, the other targets > are in general not even expected to work as if they were the sole target. The closer they work to how they work as the sole target, the less likely multi-target configurations are to break when people change one of the targets without testing multi-target. So you should clearly delineate and document the ways in which multi-target configurations will behave differently from one of the targets as sole target. For example, you might enumerate every target macro used in parts of the compiler that are built only once, and list them in groups with each groups marked with the aspect of compiler behavior that will differ in multi-target configurations (type sizes, say). That exercise of listing target macros (including undocumented target macros) will also identify cases where such a behavior difference is inappropriate. For example, tree-inline.c uses MOVE_RATIO and MOVE_MAX_PIECES. I see no good reason for the secondary target to use those settings from the main target. So you should convert those macros to hooks (as separate, preparatory patches). (Making tree-inline.c target-specific would be a worse approach; it's much better to have a simple rule about which files are target-specific, e.g. only RTL passes, than to have some files randomly needing to be target-specific because of target macro uses hidden in the middle of the file. Certainly, if anything not obviously fitting a general rule is marked target-specific, there should be comments explaining why, just like the comments on various #include directives explaining why some files include headers that it seems they shouldn't need to include.) I expect there are many other such cases; you should show how you have searched for and identified which files are using target macros so that reviewers can effectively verify that all files that should be marked target-specific are so marked. You also need to explain how the assembler and linker fit in your model, given that the GNU assembler is rather further from multi-target than GCC at present (working entirely with macros rather than any sort of vector of functions) and that ELF doesn't have the concept of a single object file with code for multiple architectures. > > Ideally this would go in the installation, user and internals manuals, as > > applicable. The information in install.texi in this patch seems extremely > > limited. Random README files for bits of internals are strongly > > deprecated; put the information in README-multi-target in the internals > > manual instead. It needs to be clear from the internals manual what > > maintainers need to do with START_TARGET_SPECIFIC, EXTRA_TARGET, > > EXTRA_TARGETS_DECL, etc., when making changes to the compiler: the > > circumstances in which they need to include or change such code so as not > > to break the invariants of multi-target configurations working and > > generating identical code when you select any one of the multiple targets > > to that generated by a single-target configuration for that target. > > I agree that such detailed documentation would be good to have (except that > as stated above, not all the targets are really expected to work the same > as a single-target configuration). > However, that is significant extra work, and I doubt that I'll find time to > do that, because I have to look for work that someone is willing to pay for > in the meantime. If a feature is going to be problematic to maintain in future, that generally indicates it should not be accepted on trunk. That is, sufficient documentation is needed for other people to keep the feature working as they make unrelated changes, and the feature needs to be designed and implemented in a way that makes this no more complicated than necessary. > As noted above, globally rearranging all the GCC code to get a better > delineation between target-specific and target independent code, and thus > also reduce the number of START_TARGET_SPECIFIC / END_TARGET_SPECIFIC > markers, would make the patch impossible to maintain or review. That > has to be done piecemeal as follow-on work. No, as preparatory work, not follow-on. A patch where whole files are either all target-specific or have nothing target-specific is far more reviewable, and creates a far more maintainable source tree, than one with markers scattered in the middle of files. The natural model I think of is the unified Cell debugger support in GDB. The feature is similar - support for debugging programs where some call frames are on one architecture and some on another. The patch submissions included *many* patch series each of which had *many* patches in it. It's not that it's good in itself to have large numbers of patches - it's that a lot of preparation is needed to get the source tree in a state such that such a feature fits in in a clean and maintainable way, and such preparation naturally splits up into a large number of pieces each of which is an improvement to the source tree in its own right. I think the unified Cell debugger patch submissions are an excellent example to try to emulate. -- Joseph S. Myers joseph@codesourcery.com