Re: [lto] Refactor streamer (1/N) (issue4809083)
Hi,
On Fri, 26 Aug 2011, Jakub Jelinek wrote:
> While you are touching it, I think we should also optimize it as in the
> patch below. I'm afraid no string length optimization would be able to
> figure out that it doesn't have to call strlen twice, because the
> htab_find_slot isn't pure.
Sure. Regstrapped the below patch and checked in as r178118.
Ciao,
Michael.
--
Index: lto-streamer-in.c
===
--- lto-streamer-in.c (revision 178117)
+++ lto-streamer-in.c (revision 178118)
@@ -98,21 +98,22 @@ canon_file_name (const char *string)
{
void **slot;
struct string_slot s_slot;
+ size_t len = strlen (string);
+
s_slot.s = string;
- s_slot.len = strlen (string);
+ s_slot.len = len;
slot = htab_find_slot (file_name_hash_table, &s_slot, INSERT);
if (*slot == NULL)
{
- size_t len;
char *saved_string;
struct string_slot *new_slot;
- len = strlen (string);
saved_string = (char *) xmalloc (len + 1);
new_slot = XCNEW (struct string_slot);
- strcpy (saved_string, string);
+ memcpy (saved_string, string, len + 1);
new_slot->s = saved_string;
+ new_slot->len = len;
*slot = new_slot;
return saved_string;
}
Re: [lto] Refactor streamer (1/N) (issue4809083)
On Fri, Aug 26, 2011 at 02:34:29PM +0200, Michael Matz wrote:
> Hi,
>
> On Fri, 26 Aug 2011, Richard Guenther wrote:
>
> > >> I am going to be sending the renaming patch later today or tomorrow.
> > >> In principle, the things I want to abstract are those that are
> > >> forcing me to include lto-streamer.h from
> > >> {tree,gimple,data}-streamer.*. I will know better when I merge this
> > >> into the pph branch, though.
> > >
> > > Yeah, I think we discussed this already and agreed on that this is a
> > > sensible plan.
> >
> > This patch caused http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50165 it
> > seems that LTO string hashing is seriously broken now.
>
> Once regstrap passes on x86_64-linux I'm checking this in as obvious.
While you are touching it, I think we should also optimize it as in the
patch below. I'm afraid no string length optimization would be able to
figure out that it doesn't have to call strlen twice, because the
htab_find_slot isn't pure.
2011-08-26 Jakub Jelinek
* lto-streamer-in.c (canon_file_name): Avoid calling strlen twice,
use memcpy instead of strcpy.
--- gcc/lto-streamer-in.c.jj2011-08-26 14:39:52.0 +0200
+++ gcc/lto-streamer-in.c 2011-08-26 14:40:59.543884012 +0200
@@ -98,21 +98,20 @@ canon_file_name (const char *string)
{
void **slot;
struct string_slot s_slot;
+ size_t len = strlen (string);
s_slot.s = string;
- s_slot.len = strlen (string);
+ s_slot.len = len;
slot = htab_find_slot (file_name_hash_table, &s_slot, INSERT);
if (*slot == NULL)
{
- size_t len;
char *saved_string;
struct string_slot *new_slot;
- len = strlen (string);
saved_string = (char *) xmalloc (len + 1);
new_slot = XCNEW (struct string_slot);
new_slot->len = len;
- strcpy (saved_string, string);
+ memcpy (saved_string, string, len + 1);
new_slot->s = saved_string;
*slot = new_slot;
return saved_string;
Jakub
Re: [lto] Refactor streamer (1/N) (issue4809083)
On 11-08-26 04:24 , Richard Guenther wrote: This patch caused http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50165 it seems that LTO string hashing is seriously broken now. Sorry about this. Bad timing as I will be away until 7/Sep. Would it make things easier if the commit that introduced this was reverted? Diego.
Re: [lto] Refactor streamer (1/N) (issue4809083)
Hi,
On Fri, 26 Aug 2011, Richard Guenther wrote:
> >> I am going to be sending the renaming patch later today or tomorrow.
> >> In principle, the things I want to abstract are those that are
> >> forcing me to include lto-streamer.h from
> >> {tree,gimple,data}-streamer.*. I will know better when I merge this
> >> into the pph branch, though.
> >
> > Yeah, I think we discussed this already and agreed on that this is a
> > sensible plan.
>
> This patch caused http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50165 it
> seems that LTO string hashing is seriously broken now.
Once regstrap passes on x86_64-linux I'm checking this in as obvious.
Ciao,
Michael.
--
PR lto/50165
* lto-streamer-in.c (canon_file_name): Initialize new_slot->len.
Index: lto-streamer-in.c
===
--- lto-streamer-in.c (revision 178040)
+++ lto-streamer-in.c (working copy)
@@ -113,6 +113,7 @@ canon_file_name (const char *string)
new_slot = XCNEW (struct string_slot);
strcpy (saved_string, string);
new_slot->s = saved_string;
+ new_slot->len = len;
*slot = new_slot;
return saved_string;
}
Re: [lto] Refactor streamer (1/N) (issue4809083)
On Mon, Aug 8, 2011 at 5:17 PM, Richard Guenther wrote:
> On Mon, 8 Aug 2011, Diego Novillo wrote:
>
>> On Mon, Aug 8, 2011 at 10:52, Michael Matz wrote:
>>
>> > Sound. ;) Looking forward to some bikeshedding about naming in (2) and
>> > overabstraction in (3) :)
>>
>> Heh, yeah.
>>
>> I am going to be sending the renaming patch later today or tomorrow.
>> In principle, the things I want to abstract are those that are forcing
>> me to include lto-streamer.h from {tree,gimple,data}-streamer.*.
>> I will know better when I merge this into the pph branch, though.
>
> Yeah, I think we discussed this already and agreed on that this is
> a sensible plan.
This patch caused http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50165
it seems that LTO string hashing is seriously broken now.
Richard.
> Richard.
Re: [lto] Refactor streamer (1/N) (issue4809083)
On Mon, 8 Aug 2011, Diego Novillo wrote:
> On Mon, Aug 8, 2011 at 10:52, Michael Matz wrote:
>
> > Sound. ;) Looking forward to some bikeshedding about naming in (2) and
> > overabstraction in (3) :)
>
> Heh, yeah.
>
> I am going to be sending the renaming patch later today or tomorrow.
> In principle, the things I want to abstract are those that are forcing
> me to include lto-streamer.h from {tree,gimple,data}-streamer.*.
> I will know better when I merge this into the pph branch, though.
Yeah, I think we discussed this already and agreed on that this is
a sensible plan.
Richard.
Re: [lto] Refactor streamer (1/N) (issue4809083)
On Mon, Aug 8, 2011 at 10:52, Michael Matz wrote:
> Sound. ;) Looking forward to some bikeshedding about naming in (2) and
> overabstraction in (3) :)
Heh, yeah.
I am going to be sending the renaming patch later today or tomorrow.
In principle, the things I want to abstract are those that are forcing
me to include lto-streamer.h from {tree,gimple,data}-streamer.*.
I will know better when I merge this into the pph branch, though.
Diego.
Re: [lto] Refactor streamer (1/N) (issue4809083)
On Mon, Aug 8, 2011 at 10:35, Jack Howarth wrote: > On Mon, Aug 08, 2011 at 10:23:34AM -0400, Diego Novillo wrote: >> >> This is the first patch in a series of refactoring patches to cleanup >> the API for the LTO streamer. I need this cleanup so I can change the >> way things like caching and external references work in PPH. With the >> current code, I would need to introduce even more streamer hooks. But >> if we simplify the API and create smaller low-level streaming >> functions, then we can have the LTO and PPH streamer use these >> functions without having to resort to a lot of callbacks. > > Diego, > While you are cleaning up the streamer can you also take a look at.. > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49992 > > to see if these duplicate symbols also exist on linux in a non-fatal manner? > The change in r177358 broke the ability to do a lto-bootstrap on darwin. > While the lto-bootstrap appears to still work on linux (from the > gcc-testresults > postings), it is unclear if any of these is using the lto-streamer instead > of the lto linker plugin. I think this should be fixed ASP since darwin is > probably one of the last targets to still use the lto-streamer instead of > the linker plugin. I think we are talking about different things. The streamer is the set of low-level functions that pickle and unpickle GCC data structures. It has nothing to do with the linker plugin. All I'm doing is moving functions around and renaming things. None of these patches will change any existing functionality (least of all anything related to higher-level modules). Diego.
Re: [lto] Refactor streamer (1/N) (issue4809083)
Hi, On Mon, 8 Aug 2011, Diego Novillo wrote: > With these patches I'd like to: > > 1 Create independent facilities to stream trees, gimple and other > data structures independently of LTO. This first patch creates > the files data-streamer*.[ch], tree-streamer*.[ch] and > gimple-streamer*.[ch]. There are no other changes yet. This simply > moves functions out of lto-streamer*[ch], but no functions or data > structures have been renamed. This comes in a separate patch. > > 2 Make the naming scheme consistent. We use a mix of names to mean > the same thing (_out_, _output_, _write_, etc). We have lto_ > prefixes for functions that are actually generic. And we still have > references to dwarf names (uleb, sleb, etc). > > 3 Abstract the LTO data structures. The buffering and > descriptor data structures used for LTO are somewhat confusing. The > generic streaming routines should simply work on bytecode buffers > that are independent of LTO, PPH or other streamers we may want to > create in the future. > > 4 Reduce the number of callbacks. Ideally, I'd like to eliminate them > completely. The API we need to use from PPH is more settled now, so > it should be easier to decide what needs a callback and what can be > exposed. > > Richard, Michael, Jan, how does this plan sound? Sound. ;) Looking forward to some bikeshedding about naming in (2) and overabstraction in (3) :) Ciao, Michael.
