On December 19, 2014 2:44:00 PM CET, Andrew MacLeod <amacl...@redhat.com> wrote: >On 12/19/2014 06:20 AM, Richard Biener wrote: >> On Fri, Dec 19, 2014 at 1:37 AM, Michael Collison >> <michael.colli...@linaro.org> wrote: >>> This patch flattens tree.h and tree-core.h. This work is part of the >GCC >>> Re-Architecture effort being led by Andrew MacLeod. >>> >>> I removed the includes in tree.h and tree-core.h except for the >include of >>> tree-core.h in tree.h. >>> >>> I modified genattrtab.c, genautomata.c, genemit.c, gengtype.c, >gengtype.c, >>> genoptinit.c, genoutput.c, >>> genpeep.c, genpreds.c, and optc-save-gen-awk to include the the >necessary >>> include files removed from >>> tree.h and tree-core.h when generating their respective files. >>> >>> All other changes include the necessary include files removed from >tree.h >>> and tree-core.h. Note the patches modifies all the front-ends. >>> >>> I bootstrapped on x86 with all languages. I also bootstrapped on all >targets >>> listed in contrib/config-list.mk with c and c++ enabled. >>> >>> Is this okay for trunk? >> No - you need to rework it (substantially?). Nothing but tree.h (and >gimple.h) >> may include tree-core.h directly. Instead where you added includes >of >> tree-core.h you need to include tree.h. Basically tree-core.h is an >> implementation >> detail introduced to hide layer violations where we understand them >(gimple.h). >Hmm, yeah. tree-core.h is included in tree.h as planned, but then it's >copy out to all the include locations anyway... maybe you just have to >remove the #include "tree-core.h"'s? > >It also looks to me like the include ordering is still "off". ie: > >tree.h: > > #include "tree-core.h" >-#include "hash-set.h" <--- 1 >-#include "wide-int.h" <--- 2 >-#include "inchash.h" <---3 >- >-/* These includes are required here because they provide declarations >- used by inline functions in this file. >- >- FIXME - Move these users elsewhere? */ >-#include "fold-const.h" <--- 4 > >and when the includes are flattened: > >--- a/gcc/varasm.c >+++ b/gcc/varasm.c >@@ -30,15 +30,22 @@ along with GCC; see the file COPYING3. If not see > #include "coretypes.h" > #include "tm.h" > #include "rtl.h" >+#include "hash-set.h" <--- 1 included earlier by tree-core.h, >ordering still OK >+#include "machmode.h" >+#include "vec.h" >+#include "double-int.h" >+#include "input.h" >+#include "alias.h" >+#include "symtab.h" >+#include "tree-core.h" >+#include "fold-const.h" <--- 4 included earlier >+#include "wide-int.h" <--- 2 >+#include "inchash.h" <---3 > #include "tree.h" > #include "stor-layout.h" > > >Now it may not matter for these particular includes since I doubt there > >are any cross dependencies, but the net result is this will see files >included in a different order. I raise the point because for some files > >going forward it does matter... tm.h in particular is critical since >other files do condition compilation on macros defined in that file. >We >don't eliminate unnecessary include files any more because we haven't >done the analysis to know which ones are "important" yet and changing >their order may change code generation, but not compilation success. > >we really need to get that tool going so we can reduce the include >list... :-) > >Maybe there is a small bug in the include replicating tool? Just >wondering why the ordering is arbitrarily different here. > >That said, we wont be able to keep it exact around tree-core.h. if we > >remove the #include "tree-core.h" from varasm.c those 3 includes I >tagged will be seen *before* any of the code in tree-core.h is seen, >but >that case should be OK. Before we split tree-core.h from tree.h they >were all included before the contents were seen, so we are in fact just > >restoring the previous ordering :-) > >> I suppose we should add >> >> #ifndef I_MAY_INCLUDE_TREE_CORE_H >> #error Bad guy! >> #endif >> >> to tree-core.h and define/undef that around the very few includes of >tree-core.h >> we want to allow. I see we already include it in more places than >desired. >> >> So - can you do a preparatory patch doing that and removing the >tree-core.h >> include from everything but tree.h (allow it from expr.h until that >> got flattened)? >> >> Andrew, I suppose my recollection of how we architected tree.h and >tree-core.h >> is correct? >Pretty much. tree-core.h was simply a split of tree.h to separate the >structure definitions away from the accessors macros so we could >provide >alternative means of getting at the bits. It was never intended to be >used directly, although I suppose there are occasions where it could be > >useful.... In theory those places that include tree-core.h directly >now, and don't include tree.h could simply include tree.h. I guess a >good question would be why are those files caring about the contents of > >tree-core.h but not tree.h... there might be something that could be >factored out so they don't even need tree-core.h. (maybe they don't >now....?! ) using tree.h ought to be fine tho. > >gimple.h doesnt need to include tree-core.h either.
That's because it still exposes interfaces with trees, thus tree.h is needed anyways. Your idea was to abstract that away. My point then was that we should disallow tree.h from files using gimple.h. For that to work without actually implementing non-tree data structures Gimple.h needs to be able to look at tree Implementation details. Thus tree-core.h. At least if that still us your plan.. Richard. At the moment, no >one except tree.h should have to. maybe we just clamp on that? >#ifndef GCC_TREE_H >#error tree-core.h should only be included from tree.h >#endif > >That doesn't actually prevent someone from including it, but if it is >included, it would have to be included after tree.h, making it a no-op >anyway. we ought to be able to police ourselves for those. > >If we allow it in a few other places temporarily like expr.h until >flattened, just code that into the #ifndef.. ie >#if !defined GCC_TREE_H && !defined GCC_EXPR_H >and remove the clause when flattening happens. > >Hows that seem? It just leaves all the onus in tree-core.h instead of >pushing it out to the place(s) that use it. > >Andrew
