| Message ID | 20110624123045.GC3591@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Fri, 24 Jun 2011, Jan Hubicka wrote: > Hi, > this is yet another variant of the fix. This time we stream builtins decls as > usually, but at fixup time we copy the assembler names (if set) into the > builtin decls used by folders. Not sure if it is any better than breaking > memops-asm, but I can imagine that things like glibc actually rename string > functions into their internal variants (and thus with this version of patch we > would be able to LTO such library, but still we won't be able to LTO such > library into something else because something else would end up referncing the > internal versions of builtins). I doubt we could do any better, however. Not stream builtins with adjusted assembler names (I guess we'd need a flag for this, DECL_USER_ASSEMBLER_NAME_SET_P? Or just check for attributes?) as builtins but as new decls. Let lto symbol merging then register those as aliases. But which way around? probably similar to how we should handle re-defined extern inlines, the extern inline being the GCC builtin and the re-definition being the aliased one. > __attribute__ ((used)) is still needed in memops-asm-lib.c because LTO symtab > of course doesn't see the future references to builtins that we will emit > later via folding. I think it is resonable requirement, as discussed at the > time enabling the plugin. Yes, I think the testcase fix sounds reasonable. I suppose you can come up with a simpler testcase for this "feature" for gcc.dg/lto highlighting the different issues? I'm not sure if we are talking about my_memcpy () alias("memcpy") or memcpy () alias("my_memcpy"). I still like to stream unmodified builtins as builtins, as that is similar to pre-loading the streamer caches with things like void_type_node or sizetype. Richard.
> On Fri, 24 Jun 2011, Jan Hubicka wrote: > > > Hi, > > this is yet another variant of the fix. This time we stream builtins decls as > > usually, but at fixup time we copy the assembler names (if set) into the > > builtin decls used by folders. Not sure if it is any better than breaking > > memops-asm, but I can imagine that things like glibc actually rename string > > functions into their internal variants (and thus with this version of patch we > > would be able to LTO such library, but still we won't be able to LTO such > > library into something else because something else would end up referncing the > > internal versions of builtins). I doubt we could do any better, however. > > Not stream builtins with adjusted assembler names (I guess we'd need > a flag for this, DECL_USER_ASSEMBLER_NAME_SET_P? Or just check for Most of code just checks for '*' on begginign of assembler name. I suppose it is safe. > attributes?) as builtins but as new decls. Let lto symbol merging > then register those as aliases. But which way around? probably > similar to how we should handle re-defined extern inlines, the > extern inline being the GCC builtin and the re-definition being > the aliased one. I don't quite get your answer here. What we do now is: 1) stream in builtin as special kind of reference with decl assembler name associated to it 2) at streaming in time always resolve builtlin to the "official" builtin decls (no matter what types and other stuff builtin had at stream out time) and overwritting the official builtin assembler name into one specified. What i suggest is 1) Stream out builtins as usual decls just with the extra function code 2) Stream in builtins as usually 3) optionally set the assembler name of the "official" decl I see there are problems with i.e. one decl rule, but we do have same problems with normal frontends that also do use different decl for explicit builtin calls than for implicit, sadly. I am not quite sure what the proper fix for this problem is - it is very handy to have builtin decl in middle end where I know it is sane (i.e. it has the right types etc.). Since C allows to declare the builtins arbitrarily, it gets bit tricky to preserve one decl rule here. > > > __attribute__ ((used)) is still needed in memops-asm-lib.c because LTO symtab > > of course doesn't see the future references to builtins that we will emit > > later via folding. I think it is resonable requirement, as discussed at the > > time enabling the plugin. > > Yes, I think the testcase fix sounds reasonable. > > I suppose you can come up with a simpler testcase for this "feature" > for gcc.dg/lto highlighting the different issues? I'm not sure > if we are talking about my_memcpy () alias("memcpy") or > memcpy () alias("my_memcpy"). > > I still like to stream unmodified builtins as builtins, as that is > similar to pre-loading the streamer caches with things like > void_type_node or sizetype. Doing so will need us to solve the other one decl rules probly. I didn't really got what the preloading is useful for after all? Honza > > Richard.
Hi, On Mon, 27 Jun 2011, Jan Hubicka wrote: > > I still like to stream unmodified builtins as builtins, as that is > > similar to pre-loading the streamer caches with things like > > void_type_node or sizetype. > > Doing so will need us to solve the other one decl rules probly. I didn't > really got what the preloading is useful for after all? One important thing that really affects correctness which preloading does is to guarantee pointer equality for things like void_type_node or error_mark_node, which are used sometimes. If we weren't doing preloading we would have to forcibly merge all these trees over different units, and what's worse, also fill out the global tree arrays with that merged variant. And for that we'd need to note somehow which array slot a certain tree is coming from (and deal with the fact that different language frontends fill this array differently, sometimes with pointer-eqal tree nodes, sometimes only with semantically equal tree nodes, sometimes not at all). Or we could fix all places where we use pointer equality with some of the global trees, which I wouldn't like, even for abstract considerations. There's really no point in having different but equal void_type nodes, or error_mark nodes. preloading really is the easiest way to solve all this. It's just important that all .o files have the same idea about "tree at slot so-and-so" (e.g. meaning "error_mark_node"), which I fixed some weeks ago. And with early-debug-info we don't even then have the issue of e.g. the base integer types not being named like the frontend emitted them. Ciao, Michael.
On Mon, 27 Jun 2011, Jan Hubicka wrote: > > On Fri, 24 Jun 2011, Jan Hubicka wrote: > > > > > Hi, > > > this is yet another variant of the fix. This time we stream builtins decls as > > > usually, but at fixup time we copy the assembler names (if set) into the > > > builtin decls used by folders. Not sure if it is any better than breaking > > > memops-asm, but I can imagine that things like glibc actually rename string > > > functions into their internal variants (and thus with this version of patch we > > > would be able to LTO such library, but still we won't be able to LTO such > > > library into something else because something else would end up referncing the > > > internal versions of builtins). I doubt we could do any better, however. > > > > Not stream builtins with adjusted assembler names (I guess we'd need > > a flag for this, DECL_USER_ASSEMBLER_NAME_SET_P? Or just check for > > Most of code just checks for '*' on begginign of assembler name. I suppose it is safe. > > > attributes?) as builtins but as new decls. Let lto symbol merging > > then register those as aliases. But which way around? probably > > similar to how we should handle re-defined extern inlines, the > > extern inline being the GCC builtin and the re-definition being > > the aliased one. > > I don't quite get your answer here. What we do now is: > > 1) stream in builtin as special kind of reference with decl assembler name associated to it > 2) at streaming in time always resolve builtlin to the "official" builtin decls (no matter > what types and other stuff builtin had at stream out time) and overwritting the official builtin > assembler name into one specified. > > What i suggest is > > 1) Stream out builtins as usual decls just with the extra function code > 2) Stream in builtins as usually > 3) optionally set the assembler name of the "official" decl > > I see there are problems with i.e. one decl rule, but we do have same problems > with normal frontends that also do use different decl for explicit builtin > calls than for implicit, sadly. > > I am not quite sure what the proper fix for this problem is - it is very handy > to have builtin decl in middle end where I know it is sane (i.e. it has the > right types etc.). Since C allows to declare the builtins arbitrarily, it gets > bit tricky to preserve one decl rule here. Hm. I would suggest to do as now, stream in builtin specially if it does not have an assembler name attribute. If it does have it, stream it as usually and let lto symtab do its job (I suppose we need to register builtin functions with the symtab as well). > > > __attribute__ ((used)) is still needed in memops-asm-lib.c because LTO symtab > > > of course doesn't see the future references to builtins that we will emit > > > later via folding. I think it is resonable requirement, as discussed at the > > > time enabling the plugin. > > > > Yes, I think the testcase fix sounds reasonable. > > > > I suppose you can come up with a simpler testcase for this "feature" > > for gcc.dg/lto highlighting the different issues? I'm not sure > > if we are talking about my_memcpy () alias("memcpy") or > > memcpy () alias("my_memcpy"). > > > > I still like to stream unmodified builtins as builtins, as that is > > similar to pre-loading the streamer caches with things like > > void_type_node or sizetype. > > Doing so will need us to solve the other one decl rules probly. > I didn't really got what the preloading is useful for after all? Saving memory mostly, apart from the special singletons we have (as Micha already hinted). Richard.
Index: lto-streamer-out.c =================================================================== --- lto-streamer-out.c (revision 175350) +++ lto-streamer-out.c (working copy) @@ -484,8 +484,6 @@ pack_ts_function_decl_value_fields (stru { /* For normal/md builtins we only write the class and code, so they should never be handled here. */ - gcc_assert (!lto_stream_as_builtin_p (expr)); - bp_pack_enum (bp, built_in_class, BUILT_IN_LAST, DECL_BUILT_IN_CLASS (expr)); bp_pack_value (bp, DECL_STATIC_CONSTRUCTOR (expr), 1); @@ -1121,7 +1119,7 @@ lto_output_ts_binfo_tree_pointers (struc together large portions of programs making it harder to partition. Becuase devirtualization is interesting before inlining, only, there is no real need to ship it into ltrans partition. */ - lto_output_tree_or_ref (ob, flag_wpa ? NULL : BINFO_VIRTUALS (expr), ref_p); + lto_output_tree_or_ref (ob, flag_wpa || 1 ? NULL : BINFO_VIRTUALS (expr), ref_p); lto_output_tree_or_ref (ob, BINFO_VPTR_FIELD (expr), ref_p); output_uleb128 (ob, VEC_length (tree, BINFO_BASE_ACCESSES (expr))); @@ -1306,41 +1304,6 @@ lto_output_tree_header (struct output_bl } -/* Write the code and class of builtin EXPR to output block OB. IX is - the index into the streamer cache where EXPR is stored.*/ - -static void -lto_output_builtin_tree (struct output_block *ob, tree expr) -{ - gcc_assert (lto_stream_as_builtin_p (expr)); - - if (DECL_BUILT_IN_CLASS (expr) == BUILT_IN_MD - && !targetm.builtin_decl) - sorry ("gimple bytecode streams do not support machine specific builtin " - "functions on this target"); - - output_record_start (ob, LTO_builtin_decl); - lto_output_enum (ob->main_stream, built_in_class, BUILT_IN_LAST, - DECL_BUILT_IN_CLASS (expr)); - output_uleb128 (ob, DECL_FUNCTION_CODE (expr)); - - if (DECL_ASSEMBLER_NAME_SET_P (expr)) - { - /* When the assembler name of a builtin gets a user name, - the new name is always prefixed with '*' by - set_builtin_user_assembler_name. So, to prevent the - reader side from adding a second '*', we omit it here. */ - const char *str = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (expr)); - if (strlen (str) > 1 && str[0] == '*') - lto_output_string (ob, ob->main_stream, &str[1], true); - else - lto_output_string (ob, ob->main_stream, NULL, true); - } - else - lto_output_string (ob, ob->main_stream, NULL, true); -} - - /* Write a physical representation of tree node EXPR to output block OB. If REF_P is true, the leaves of EXPR are emitted as references via lto_output_tree_ref. IX is the index into the streamer cache @@ -1456,15 +1419,6 @@ lto_output_tree (struct output_block *ob lto_output_enum (ob->main_stream, LTO_tags, LTO_NUM_TAGS, lto_tree_code_to_tag (TREE_CODE (expr))); } - else if (lto_stream_as_builtin_p (expr)) - { - /* MD and NORMAL builtins do not need to be written out - completely as they are always instantiated by the - compiler on startup. The only builtins that need to - be written out are BUILT_IN_FRONTEND. For all other - builtins, we simply write the class and code. */ - lto_output_builtin_tree (ob, expr); - } else { /* This is the first time we see EXPR, write its fields Index: lto-streamer-in.c =================================================================== --- lto-streamer-in.c (revision 175350) +++ lto-streamer-in.c (working copy) @@ -1736,18 +1736,7 @@ unpack_ts_function_decl_value_fields (st DECL_PURE_P (expr) = (unsigned) bp_unpack_value (bp, 1); DECL_LOOPING_CONST_OR_PURE_P (expr) = (unsigned) bp_unpack_value (bp, 1); if (DECL_BUILT_IN_CLASS (expr) != NOT_BUILT_IN) - { - DECL_FUNCTION_CODE (expr) = (enum built_in_function) bp_unpack_value (bp, 11); - if (DECL_BUILT_IN_CLASS (expr) == BUILT_IN_NORMAL - && DECL_FUNCTION_CODE (expr) >= END_BUILTINS) - fatal_error ("machine independent builtin code out of range"); - else if (DECL_BUILT_IN_CLASS (expr) == BUILT_IN_MD) - { - tree result = targetm.builtin_decl (DECL_FUNCTION_CODE (expr), true); - if (!result || result == error_mark_node) - fatal_error ("target specific builtin not available"); - } - } + DECL_FUNCTION_CODE (expr) = (enum built_in_function) bp_unpack_value (bp, 11); if (DECL_STATIC_DESTRUCTOR (expr)) { priority_type p; @@ -2434,48 +2423,6 @@ lto_get_pickled_tree (struct lto_input_b } -/* Read a code and class from input block IB and return the - corresponding builtin. DATA_IN is as in lto_input_tree. */ - -static tree -lto_get_builtin_tree (struct lto_input_block *ib, struct data_in *data_in) -{ - enum built_in_class fclass; - enum built_in_function fcode; - const char *asmname; - tree result; - - fclass = lto_input_enum (ib, built_in_class, BUILT_IN_LAST); - gcc_assert (fclass == BUILT_IN_NORMAL || fclass == BUILT_IN_MD); - - fcode = (enum built_in_function) lto_input_uleb128 (ib); - - if (fclass == BUILT_IN_NORMAL) - { - if (fcode >= END_BUILTINS) - fatal_error ("machine independent builtin code out of range"); - result = built_in_decls[fcode]; - gcc_assert (result); - } - else if (fclass == BUILT_IN_MD) - { - result = targetm.builtin_decl (fcode, true); - if (!result || result == error_mark_node) - fatal_error ("target specific builtin not available"); - } - else - gcc_unreachable (); - - asmname = lto_input_string (data_in, ib); - if (asmname) - set_builtin_user_assembler_name (result, asmname); - - lto_streamer_cache_append (data_in->reader_cache, result); - - return result; -} - - /* Read the physical representation of a tree node with tag TAG from input block IB using the per-file context in DATA_IN. */ @@ -2495,10 +2442,6 @@ lto_read_tree (struct lto_input_block *i if (streamer_hooks.read_tree) streamer_hooks.read_tree (ib, data_in, result); - /* We should never try to instantiate an MD or NORMAL builtin here. */ - if (TREE_CODE (result) == FUNCTION_DECL) - gcc_assert (!lto_stream_as_builtin_p (result)); - /* end_marker = */ lto_input_1_unsigned (ib); #ifdef LTO_STREAMER_DEBUG @@ -2582,12 +2525,6 @@ lto_input_tree (struct lto_input_block * the reader cache. */ result = lto_get_pickled_tree (ib, data_in); } - else if (tag == LTO_builtin_decl) - { - /* If we are going to read a built-in function, all we need is - the code and class. */ - result = lto_get_builtin_tree (ib, data_in); - } else if (tag == lto_tree_code_to_tag (INTEGER_CST)) { /* For integer constants we only need the type and its hi/low Index: lto/lto.c =================================================================== --- lto/lto.c (revision 175350) +++ lto/lto.c (working copy) @@ -669,6 +669,41 @@ uniquify_nodes (struct data_in *data_in, if (!t) continue; + if (TREE_CODE (t) == FUNCTION_DECL + && DECL_BUILT_IN_CLASS (t) != NOT_BUILT_IN) + { + tree decl = NULL; + if (DECL_BUILT_IN_CLASS (t) == BUILT_IN_NORMAL) + { + if (DECL_FUNCTION_CODE (t) >= END_BUILTINS) + fatal_error ("machine independent builtin code out of range"); + else + decl = built_in_decls[DECL_FUNCTION_CODE (t)]; + } + else if (DECL_BUILT_IN_CLASS (t) == BUILT_IN_MD) + { + decl = targetm.builtin_decl (DECL_FUNCTION_CODE (t), true); + if (!decl || decl == error_mark_node) + fatal_error ("target specific builtin not available"); + } + /* When user assembler name is set, change the bultin decl used by + folder and rtl expansion. + ??? this is not always quite correct: when multiple units are merged + together, one of assembler names will win. However this solve at + least the memops-asm testcase. */ + if (decl + && DECL_ASSEMBLER_NAME_SET_P (t)) + { + /* When the assembler name of a builtin gets a user name, + the new name is always prefixed with '*' by + set_builtin_user_assembler_name. So, to prevent the + reader side from adding a second '*', we omit it here. */ + const char *str = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (t)); + if (strlen (str) > 1 && str[0] == '*') + set_builtin_user_assembler_name (decl, &str[1]); + } + } + /* First fixup the fields of T. */ lto_fixup_types (t); Index: lto-streamer.h =================================================================== --- lto-streamer.h (revision 175350) +++ lto-streamer.h (working copy) @@ -198,9 +198,6 @@ enum LTO_tags /* EH region holding the previous statement. */ LTO_eh_region, - /* An MD or NORMAL builtin. Only the code and class are streamed out. */ - LTO_builtin_decl, - /* Function body. */ LTO_function, @@ -1141,17 +1138,6 @@ emit_label_in_global_context_p (tree lab return DECL_NONLOCAL (label) || FORCED_LABEL (label); } -/* Return true if tree node EXPR should be streamed as a builtin. For - these nodes, we just emit the class and function code. */ -static inline bool -lto_stream_as_builtin_p (tree expr) -{ - return (TREE_CODE (expr) == FUNCTION_DECL - && DECL_IS_BUILTIN (expr) - && (DECL_BUILT_IN_CLASS (expr) == BUILT_IN_NORMAL - || DECL_BUILT_IN_CLASS (expr) == BUILT_IN_MD)); -} - DEFINE_DECL_STREAM_FUNCS (TYPE, type) DEFINE_DECL_STREAM_FUNCS (FIELD_DECL, field_decl) DEFINE_DECL_STREAM_FUNCS (FN_DECL, fn_decl)