| Message ID | 20130904160409.GF20687@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 04, 2013 at 06:04:09PM +0200, Jan Hubicka wrote: > * cgraphunit.c (analyze_functions): Populate assembler names once done > with early unreachable function removal. Please add some of the testcases from the PRs and mention the PRs in the ChangeLog entry. > --- cgraphunit.c (revision 202199) > +++ cgraphunit.c (working copy) > @@ -1074,6 +1086,7 @@ > bitmap_obstack_release (NULL); > pointer_set_destroy (reachable_call_targets); > ggc_collect (); > + symtab_initialize_asm_name_hash (); > } > > /* Translate the ugly representation of aliases as alias pairs into nice Jakub
On 09/04/2013 06:04 PM, Jan Hubicka wrote: > this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o > bodies I did not really anticipate. [...] > I would like to basically ask if it seems to make sense to go this route and > try to get rid of those declarations. I'm currently working on a new target, ptx, which uses a pseudo-assembler where functions (even extern ones) need to be declared with their arguments and return types. With my current code I have to look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite sure yet whether the change to delete them will break the backend. Bernd
> On 09/04/2013 06:04 PM, Jan Hubicka wrote: > > this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o > > bodies I did not really anticipate. > [...] > > I would like to basically ask if it seems to make sense to go this route and > > try to get rid of those declarations. > > I'm currently working on a new target, ptx, which uses a > pseudo-assembler where functions (even extern ones) need to be declared > with their arguments and return types. With my current code I have to > look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite > sure yet whether the change to delete them will break the backend. How do you support K&R functions here? My basic idea was that TYPE_ARG_TYPES should give enough information about external function calling convention anyone will ever need. I would hope that this will be sufficient for your use, too, despite the fact you no longer have parameter names at hand and you also lose info about external inline K&R-style delcared functions that has been optimized out. If not that indeed, you will not see DECL_ARGUMENTS for external function anytime after cgraph_remove_unreachable_functions is called. Honza
On 09/04/2013 07:09 PM, Jan Hubicka wrote: > How do you support K&R functions here? My basic idea was that TYPE_ARG_TYPES > should give enough information about external function calling convention > anyone will ever need. I would hope that this will be sufficient for your > use, too, despite the fact you no longer have parameter names at hand > and you also lose info about external inline K&R-style delcared functions > that has been optimized out. TYPE_ARG_TYPES doesn't exist for all functions, so right now the backend is using whichever of the two is available. It seems that TYPE_ARG_TYPES is actually NULL for K&R functions (gcc.c-torture/compile/20000403-1.c is the first one that fails if I try to use only TYPE_ARG_TYPES). Bernd
On 09/04/2013 10:49 AM, Bernd Schmidt wrote: > On 09/04/2013 06:04 PM, Jan Hubicka wrote: >> this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o >> bodies I did not really anticipate. > [...] >> I would like to basically ask if it seems to make sense to go this route and >> try to get rid of those declarations. > > I'm currently working on a new target, ptx, which uses a > pseudo-assembler where functions (even extern ones) need to be declared > with their arguments and return types. With my current code I have to > look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite > sure yet whether the change to delete them will break the backend. IIRC the PA had similar requirements as well -- in ASM_DECLARE_FUNCTION_NAME we have to peek at DECL_ARGUMENTS so we can pass to the assembler & linker which registers hold arguments. Jeff
Jan Hubicka <hubicka@ucw.cz> wrote: >> On 09/04/2013 06:04 PM, Jan Hubicka wrote: >> > this is third fallout of my change to remove >DECL_ARGUMENTS/DECL_RESULT for functions w/o >> > bodies I did not really anticipate. >> [...] >> > I would like to basically ask if it seems to make sense to go this >route and >> > try to get rid of those declarations. >> >> I'm currently working on a new target, ptx, which uses a >> pseudo-assembler where functions (even extern ones) need to be >declared >> with their arguments and return types. With my current code I have to >> look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite >> sure yet whether the change to delete them will break the backend. > >How do you support K&R functions here? My basic idea was that >TYPE_ARG_TYPES >should give enough information about external function calling >convention >anyone will ever need. I would hope that this will be sufficient for >your >use, too, despite the fact you no longer have parameter names at hand >and you also lose info about external inline K&R-style delcared >functions >that has been optimized out. > >If not that indeed, you will not see DECL_ARGUMENTS for external >function >anytime after cgraph_remove_unreachable_functions is called. In fact it has to work because of indirect calls and how we now handle gimple call abi via gimple_call_fntype. Richard. >Honza
> On 09/04/2013 10:49 AM, Bernd Schmidt wrote: > >On 09/04/2013 06:04 PM, Jan Hubicka wrote: > >>this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o > >>bodies I did not really anticipate. > >[...] > >>I would like to basically ask if it seems to make sense to go this route and > >>try to get rid of those declarations. > > > >I'm currently working on a new target, ptx, which uses a > >pseudo-assembler where functions (even extern ones) need to be declared > >with their arguments and return types. With my current code I have to > >look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite > >sure yet whether the change to delete them will break the backend. > IIRC the PA had similar requirements as well -- in > ASM_DECLARE_FUNCTION_NAME we have to peek at DECL_ARGUMENTS so we > can pass to the assembler & linker which registers hold arguments. This use should be safe, too. ASM_DECLARE_FUNCTION_NAME is called on function whose body is being output and there we do have DECL_ARGUMENTS (as we consider them part of the body) Honza
Hi,
this is the patch I commited after testing on x86_64-linux.
Honza
Index: ChangeLog
===================================================================
*** ChangeLog (revision 202271)
--- ChangeLog (working copy)
***************
*** 1,3 ****
--- 1,9 ----
+ 2013-09-04 Jan Hubicka <jh@suse.cz>
+
+ PR middle-end/58201
+ * cgraphunit.c (analyze_functions): Clear AUX fields
+ after processing; initialize assembler name has.
+
2013-09-04 Dodji Seketeli <dodji@redhat.com>
* tree.h (DECL_BUILT_IN): Fix typo in comment.
Index: cgraphunit.c
===================================================================
*** cgraphunit.c (revision 202271)
--- cgraphunit.c (working copy)
*************** analyze_functions (void)
*** 1064,1069 ****
--- 1064,1071 ----
}
node->symbol.aux = NULL;
}
+ for (;node; node = node->symbol.next)
+ node->symbol.aux = NULL;
first_analyzed = cgraph_first_function ();
first_analyzed_var = varpool_first_variable ();
if (cgraph_dump_file)
*************** analyze_functions (void)
*** 1074,1079 ****
--- 1076,1086 ----
bitmap_obstack_release (NULL);
pointer_set_destroy (reachable_call_targets);
ggc_collect ();
+ /* Initialize assembler name hash, in particular we want to trigger C++
+ mangling and same body alias creation before we free DECL_ARGUMENTS
+ used by it. */
+ if (!seen_error ())
+ symtab_initialize_asm_name_hash ();
}
/* Translate the ugly representation of aliases as alias pairs into nice
Index: testsuite/ChangeLog
===================================================================
*** testsuite/ChangeLog (revision 202297)
--- testsuite/ChangeLog (working copy)
***************
*** 1,3 ****
--- 1,10 ----
+ 2013-09-04 Jan Hubicka <jh@suse.cz>
+
+ PR middle-end/58201
+ * g++.dg/torture/pr58201_0.C: New testcase.
+ * g++.dg/torture/pr58201_1.C: New testcase.
+ * g++.dg/torture/pr58201.h: New testcase.
+
2013-09-05 Jan Hubicka <jh@suse.cz>
* gcc.dg/autopar/pr49960.c: Disable partial inlining
Index: testsuite/g++.dg/torture/pr58201_0.C
===================================================================
*** testsuite/g++.dg/torture/pr58201_0.C (revision 0)
--- testsuite/g++.dg/torture/pr58201_0.C (revision 0)
***************
*** 0 ****
--- 1,9 ----
+ #include "pr58201.h"
+
+ C::C2::C2(){ }
+ C::C2::~C2() { }
+
+ int main ()
+ {
+ return 0;
+ }
Index: testsuite/g++.dg/torture/pr58201_1.C
===================================================================
*** testsuite/g++.dg/torture/pr58201_1.C (revision 0)
--- testsuite/g++.dg/torture/pr58201_1.C (revision 0)
***************
*** 0 ****
--- 1,10 ----
+ /* { dg-do link } */
+ /* { dg-options "-O2" } */
+ /* { dg-additional-sources "pr58201_0.C" } */
+ #include "pr58201.h"
+
+ A::A() { }
+ A::~A() { }
+ B::B() { }
+ B::~B() { }
+
Index: testsuite/g++.dg/torture/pr58201.h
===================================================================
*** testsuite/g++.dg/torture/pr58201.h (revision 0)
--- testsuite/g++.dg/torture/pr58201.h (revision 0)
***************
*** 0 ****
--- 1,24 ----
+ class A
+ {
+ protected:
+ A();
+ virtual ~A();
+ };
+
+ class B : virtual public A
+ {
+ public:
+ B();
+ virtual ~B();
+ };
+
+ class C
+ {
+ private:
+ class C2 : public B
+ {
+ public:
+ C2();
+ virtual ~C2();
+ };
+ };
Hi Honza, On 09/06/2013 01:05 AM, Jan Hubicka wrote: > Hi, > this is the patch I commited after testing on x86_64-linux. > > Honza > > Index: ChangeLog > =================================================================== > *** ChangeLog (revision 202271) > --- ChangeLog (working copy) > *************** > *** 1,3 **** > --- 1,9 ---- > + 2013-09-04 Jan Hubicka <jh@suse.cz> > + > + PR middle-end/58201 > + * cgraphunit.c (analyze_functions): Clear AUX fields > + after processing; initialize assembler name has. > + I checked and double checked and with this commit a C++ test regressed: FAIL: g++.dg/template/cond2.C -std=gnu++98 (test for warnings, line 9) FAIL: g++.dg/template/cond2.C -std=gnu++11 (test for warnings, line 9) In practice we emit a message off by one line, line 10 instead of the correct line 9. Is it possible that you are clearing too much, so to speak? After the recent breakages, up to date testresults are arriving only now on gcc-testresults confirming my finding, for example Andreas Schwab already posted a couple. Thanks! Paolo.
> >+ 2013-09-04 Jan Hubicka <jh@suse.cz> > >+ > >+ PR middle-end/58201 > >+ * cgraphunit.c (analyze_functions): Clear AUX fields > >+ after processing; initialize assembler name has. > >+ > I checked and double checked and with this commit a C++ test regressed: > > FAIL: g++.dg/template/cond2.C -std=gnu++98 (test for warnings, line 9) > FAIL: g++.dg/template/cond2.C -std=gnu++11 (test for warnings, line 9) > > In practice we emit a message off by one line, line 10 instead of > the correct line 9. Is it possible that you are clearing too much, > so to speak? Amazing, this testcase was triggering an ICE and I had to disable initialization of assembler name hash when error arrived. That fixed the problem, but I did not notice it is still having wrong line info. Adding an alias into the unit will trigger wrong line info before my change, since assembler name hash is populated too. The error is output from DECL_ASSEMBLER_NAME hook that by itself is strange. #0 0x0000000001041490 in error(char const*, ...) () #1 0x0000000000735039 in write_expression(tree_node*) () at ../../gcc/cp/mangle.c:3008 #2 0x000000000073a70f in write_template_arg(tree_node*) () at ../../gcc/cp/mangle.c:3179 #3 0x000000000073ae69 in write_template_args(tree_node*) () at ../../gcc/cp/mangle.c:2551 #4 0x00000000007332a4 in write_name(tree_node*, int) () at ../../gcc/cp/mangle.c:821 #5 0x000000000073700c in write_type(tree_node*) () at ../../gcc/cp/mangle.c:2522 #6 0x0000000000737279 in write_type(tree_node*) () at ../../gcc/cp/mangle.c:2017 #7 0x0000000000738a18 in write_method_parms(tree_node*, int, tree_node*) () at ../../gcc/cp/mangle.c:2509 #8 0x0000000000738e2f in write_bare_function_type(tree_node*, int, tree_node*) () at ../../gcc/cp/mangle.c:2451 #9 0x0000000000733b9e in write_mangled_name(tree_node*, bool) () at ../../gcc/cp/mangle.c:689 #10 0x000000000073c5b6 in mangle_decl_string(tree_node*) () at ../../gcc/cp/mangle.c:3446 #11 0x000000000073c7e9 in mangle_decl(tree_node*) () at ../../gcc/cp/mangle.c:3468 #12 0x0000000000d1df01 in decl_assembler_name(tree_node*) () at ../../gcc/tree.c:546 #13 0x000000000083b1f5 in insert_to_assembler_name_hash(symtab_node_def*, bool) () #14 0x000000000083b352 in symtab_initialize_asm_name_hash() [clone .part.3] () It seems to be latent bug in C++ FE to not set location correctly to error - no one promise that the current locus will correspond to the declaration being mangled during the lazy call of assembler name. So I suppose error needs to be updated to get correct locus? Honza > > After the recent breakages, up to date testresults are arriving only > now on gcc-testresults confirming my finding, for example Andreas > Schwab already posted a couple. > > Thanks! > Paolo.
Index: cgraphunit.c =================================================================== --- cgraphunit.c (revision 202199) +++ cgraphunit.c (working copy) @@ -1074,6 +1086,7 @@ bitmap_obstack_release (NULL); pointer_set_destroy (reachable_call_targets); ggc_collect (); + symtab_initialize_asm_name_hash (); } /* Translate the ugly representation of aliases as alias pairs into nice
Hi, this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o bodies I did not really anticipate. Here removal of the arguments changes mangling algorithm if set_decl_assembler_name is invoked late. This is something I wanted to get rid of for a long time: we already compute assembler names for every symbol that lands symbol table after the early cleanups for every unit that is LTOed and every unit containing any alias directive. I think it will make things smoother if we computed it always early: other persistent source of problems are same body aliases that are created as a side effect of langhook of set_decl_assembler_name and that may happen at a time IPA code does not really expect new functions/variables to appear. So independently of DECL_ARGUMENTS/DECL_RESULT issues, I would like to propose the following patch that triggers unconditional computation of DECL_ASSEMBLER_NAME and the real symbol table construction. I already benchmarked it few months ago and the erformance implications seems in wash. Just to keep things linked, the other two fallouts are 1) problem with thunks needing DECL_ARGUMENTS when they are output in gimple way but these are not streamed, handled by http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00057.html 2) problem with variable sized arguments and return values "fixed" by http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00078.html (the fix restored old behaviour, but I do not think it fixes the whole issue as discussed in the thread and provided with another testcases that ICEs the compiler independently of my changes). I would like to basically ask if it seems to make sense to go this route and try to get rid of those declarations. For LTO it is really nice optimization - instead of streaming 4 decls at average for a function (function-decl, result-decl and parm-decls), we need just one. This change was originally motivated by memory analysis of firefox WPA build where PARM_DECL was the most common type of tree just after TREE_LIST. I also think it makes sense from backend point of view to consider these part of the function body representation, just as DECL_STRUCT_FUNCTOIN and DECL_INITIAL is. Even in non-LTO we save a lot of decls for all the external declarations that are kept in memory. I would be willing to try to analyze/fix the issues if they appear and I tried quite curefuly to examine the existing code dealing with arguments. There are obviously interesting scenarios, like this mangling issue, I missed. If this does not seem to make sense, I will prepare patch to revert the changes. If it does, I will commit those fixes and hope things will stabilize quickly. Honza * cgraphunit.c (analyze_functions): Populate assembler names once done with early unreachable function removal.