* Fix LTO streaming of BUILTINS_LOCATION @ 2015-06-08 4:52 Jan Hubicka 2015-06-08 7:35 ` Richard Biener 0 siblings, 1 reply; 5+ messages in thread From: Jan Hubicka @ 2015-06-08 4:52 UTC (permalink / raw) To: rguenther, gcc-patches Hi, currently we stream BUILTINS_LOCATION by expanding it and streaming resulting filename/line/col tripplet. That is a nonsense and breaks some logic that special case it. This patch fixes it by special casing it same way as we do UNKNOWN_LOCATION (we have precisely 2 special location codes, so doing compound bitpack is not needed) Bootstrapped/regtested ppc64le-linux, OK? Honza * lto-streamer-out.c (lto_output_location): Correctly stream BUILTINS_LOCATION * lto-streamer-in (lto_input_location): Likewise. Index: lto-streamer-out.c =================================================================== --- lto-streamer-out.c (revision 224201) +++ lto-streamer-out.c (working copy) @@ -205,6 +205,9 @@ bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1); if (loc == UNKNOWN_LOCATION) return; + bp_pack_value (bp, loc == BUILTINS_LOCATION, 1); + if (loc == BUILTINS_LOCATION) + return; xloc = expand_location (loc); Index: lto-streamer-in.c =================================================================== --- lto-streamer-in.c (revision 224201) +++ lto-streamer-in.c (working copy) @@ -283,6 +283,11 @@ *loc = UNKNOWN_LOCATION; return; } + if (bp_unpack_value (bp, 1)) + { + *loc = BUILTINS_LOCATION; + return; + } *loc = BUILTINS_LOCATION + 1; file_change = bp_unpack_value (bp, 1); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Fix LTO streaming of BUILTINS_LOCATION 2015-06-08 4:52 Fix LTO streaming of BUILTINS_LOCATION Jan Hubicka @ 2015-06-08 7:35 ` Richard Biener 2015-06-08 7:39 ` Jan Hubicka 0 siblings, 1 reply; 5+ messages in thread From: Richard Biener @ 2015-06-08 7:35 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Mon, 8 Jun 2015, Jan Hubicka wrote: > Hi, > currently we stream BUILTINS_LOCATION by expanding it and streaming resulting > filename/line/col tripplet. That is a nonsense and breaks some logic > that special case it. > > This patch fixes it by special casing it same way as we do UNKNOWN_LOCATION > (we have precisely 2 special location codes, so doing compound bitpack is > not needed) > > Bootstrapped/regtested ppc64le-linux, OK? > > Honza > > * lto-streamer-out.c (lto_output_location): Correctly stream > BUILTINS_LOCATION > * lto-streamer-in (lto_input_location): Likewise. > Index: lto-streamer-out.c > =================================================================== > --- lto-streamer-out.c (revision 224201) > +++ lto-streamer-out.c (working copy) > @@ -205,6 +205,9 @@ > bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1); > if (loc == UNKNOWN_LOCATION) > return; > + bp_pack_value (bp, loc == BUILTINS_LOCATION, 1); > + if (loc == BUILTINS_LOCATION) > + return; Hmm, with this and #define DECL_IS_BUILTIN(DECL) \ (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION) shouldn't we rather stream all locations <= BUILTINS_LOCATION literally? That is, instead of two bits stream a [0, BUILTINS_LOCATION+1] 'enum' here? Btw, line-map.h has RESERVED_LOCATION_COUNT for the locations that are "special" (currently two, so your patch will work in practice). > xloc = expand_location (loc); > > Index: lto-streamer-in.c > =================================================================== > --- lto-streamer-in.c (revision 224201) > +++ lto-streamer-in.c (working copy) > @@ -283,6 +283,11 @@ > *loc = UNKNOWN_LOCATION; > return; > } > + if (bp_unpack_value (bp, 1)) > + { > + *loc = BUILTINS_LOCATION; > + return; > + } > *loc = BUILTINS_LOCATION + 1; Btw, this assignment to *loc looks odd (I suppose it's to make location caching work). Richard. > file_change = bp_unpack_value (bp, 1); > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Fix LTO streaming of BUILTINS_LOCATION 2015-06-08 7:35 ` Richard Biener @ 2015-06-08 7:39 ` Jan Hubicka 2015-06-08 7:47 ` Richard Biener 0 siblings, 1 reply; 5+ messages in thread From: Jan Hubicka @ 2015-06-08 7:39 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, gcc-patches > > + bp_pack_value (bp, loc == BUILTINS_LOCATION, 1); > > + if (loc == BUILTINS_LOCATION) > > + return; > > Hmm, with this and > > #define DECL_IS_BUILTIN(DECL) \ > (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION) > > shouldn't we rather stream all locations <= BUILTINS_LOCATION literally? > That is, instead of two bits stream a [0, BUILTINS_LOCATION+1] 'enum' > here? Btw, line-map.h has RESERVED_LOCATION_COUNT for the locations > that are "special" (currently two, so your patch will work in practice). Yep, i considered that. Because we have precisely two special locations (ATM) and UNKNOWN_LOCATION is quite common, that would waste one extra bit on that case. Probably not a big deal, so if you think streaming all locations up to BUILTINS_LOCATION literarly is more robust, i will update the patch. > > > xloc = expand_location (loc); > > > > Index: lto-streamer-in.c > > =================================================================== > > --- lto-streamer-in.c (revision 224201) > > +++ lto-streamer-in.c (working copy) > > @@ -283,6 +283,11 @@ > > *loc = UNKNOWN_LOCATION; > > return; > > } > > + if (bp_unpack_value (bp, 1)) > > + { > > + *loc = BUILTINS_LOCATION; > > + return; > > + } > > *loc = BUILTINS_LOCATION + 1; > > Btw, this assignment to *loc looks odd (I suppose it's to make > location caching work). *loc is set to UNKNOWN_LOCATION/BUILTINS_LOCATION for those locations that are not cached and all others get BUILTINS_LOCATION + 1 which quite safely triggers ICE in line_map lookup though I do not recall why. I originally used UNKNOWN_LOCATION for cached values but that did not work as it confused DECL_IS_BUILTIN. We could extend API by adding INVALID_LOCATION and set it to INT_MAX or something that would also ICE. Honza > > Richard. > > > file_change = bp_unpack_value (bp, 1); > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Fix LTO streaming of BUILTINS_LOCATION 2015-06-08 7:39 ` Jan Hubicka @ 2015-06-08 7:47 ` Richard Biener 2015-06-08 22:33 ` Jan Hubicka 0 siblings, 1 reply; 5+ messages in thread From: Richard Biener @ 2015-06-08 7:47 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Mon, 8 Jun 2015, Jan Hubicka wrote: > > > + bp_pack_value (bp, loc == BUILTINS_LOCATION, 1); > > > + if (loc == BUILTINS_LOCATION) > > > + return; > > > > Hmm, with this and > > > > #define DECL_IS_BUILTIN(DECL) \ > > (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION) > > > > shouldn't we rather stream all locations <= BUILTINS_LOCATION literally? > > That is, instead of two bits stream a [0, BUILTINS_LOCATION+1] 'enum' > > here? Btw, line-map.h has RESERVED_LOCATION_COUNT for the locations > > that are "special" (currently two, so your patch will work in practice). > > Yep, i considered that. Because we have precisely two special locations (ATM) > and UNKNOWN_LOCATION is quite common, that would waste one extra bit on that > case. > > Probably not a big deal, so if you think streaming all locations up to > BUILTINS_LOCATION literarly is more robust, i will update the patch. Yeah, I think streaming all locations up to RESERVED_LOCATION_COUNT literally is more robust. Thus do bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT, loc < RESERVED_LOCATION_COUNT ? loc : RESERVED_LOCATION_COUNT); if (loc < RESERVED_LOCATION_COUNT) return; and on unpacking use the special RESERVED_LOCATION_COUNT value to fall thru to unpacked location handling. Richard. > > > > > xloc = expand_location (loc); > > > > > > Index: lto-streamer-in.c > > > =================================================================== > > > --- lto-streamer-in.c (revision 224201) > > > +++ lto-streamer-in.c (working copy) > > > @@ -283,6 +283,11 @@ > > > *loc = UNKNOWN_LOCATION; > > > return; > > > } > > > + if (bp_unpack_value (bp, 1)) > > > + { > > > + *loc = BUILTINS_LOCATION; > > > + return; > > > + } > > > *loc = BUILTINS_LOCATION + 1; > > > > Btw, this assignment to *loc looks odd (I suppose it's to make > > location caching work). > > *loc is set to UNKNOWN_LOCATION/BUILTINS_LOCATION for those locations that are > not cached and all others get BUILTINS_LOCATION + 1 which quite safely triggers > ICE in line_map lookup though I do not recall why. I originally used > UNKNOWN_LOCATION for cached values but that did not work as it confused > DECL_IS_BUILTIN. > > We could extend API by adding INVALID_LOCATION and set it to INT_MAX or something > that would also ICE. > > Honza > > > > Richard. > > > > > file_change = bp_unpack_value (bp, 1); > > > > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Fix LTO streaming of BUILTINS_LOCATION 2015-06-08 7:47 ` Richard Biener @ 2015-06-08 22:33 ` Jan Hubicka 0 siblings, 0 replies; 5+ messages in thread From: Jan Hubicka @ 2015-06-08 22:33 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, gcc-patches > > Yeah, I think streaming all locations up to RESERVED_LOCATION_COUNT > literally is more robust. Thus do > > bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT, > loc < RESERVED_LOCATION_COUNT ? loc : > RESERVED_LOCATION_COUNT); > if (loc < RESERVED_LOCATION_COUNT) > return; Yep, I did that in meantime (did not notice we have RESERVED_LOCATION_COUNT) This is what I commited. 2015-06-08 Jan Hubicka <hubicka@ucw.cz> * lto-streamer-out.c (lto_output_location): Stream reserved locations correctly. * lto-streamer-in.c (lto_output_location): Likewise. Index: lto-streamer-out.c =================================================================== --- lto-streamer-out.c (revision 224248) +++ lto-streamer-out.c (working copy) @@ -202,8 +202,10 @@ expanded_location xloc; loc = LOCATION_LOCUS (loc); - bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1); - if (loc == UNKNOWN_LOCATION) + bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT, + loc < RESERVED_LOCATION_COUNT + ? loc : RESERVED_LOCATION_COUNT); + if (loc < RESERVED_LOCATION_COUNT) return; xloc = expand_location (loc); Index: lto-streamer-in.c =================================================================== --- lto-streamer-in.c (revision 224248) +++ lto-streamer-in.c (working copy) @@ -278,13 +278,14 @@ gcc_assert (current_cache == this); - if (bp_unpack_value (bp, 1)) - { - *loc = UNKNOWN_LOCATION; - return; - } - *loc = BUILTINS_LOCATION + 1; + *loc = bp_unpack_int_in_range (bp, "location", 0, RESERVED_LOCATION_COUNT); + if (*loc < RESERVED_LOCATION_COUNT) + return; + + /* Keep value RESERVED_LOCATION_COUNT in *loc as linemap lookups will + ICE on it. */ + file_change = bp_unpack_value (bp, 1); line_change = bp_unpack_value (bp, 1); column_change = bp_unpack_value (bp, 1); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-08 22:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-08 4:52 Fix LTO streaming of BUILTINS_LOCATION Jan Hubicka 2015-06-08 7:35 ` Richard Biener 2015-06-08 7:39 ` Jan Hubicka 2015-06-08 7:47 ` Richard Biener 2015-06-08 22:33 ` Jan Hubicka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).