* [lto/pph] Do not pack more bits than requested (issue4415044) @ 2011-04-14 19:01 Diego Novillo 2011-04-14 19:40 ` Jakub Jelinek 0 siblings, 1 reply; 7+ messages in thread From: Diego Novillo @ 2011-04-14 19:01 UTC (permalink / raw) To: reply, rguenther, gcc-patches This is a corner case that happens not to affect LTO because the "bad" value is the last one packed. In pack_ts_type_value_fields, the last thing we pack is TYPE_ALIAS_SET, which in many cases ends up being -1. However, we request bp_pack_value to pack HOST_BITS_PER_INT (32), whereas -1 is 64 bits long on x86_64. bp_pack_value stores all 64 bits in the word, but only counts 32. Any further bit packed will simply be ignored (since they're OR'd in). I found this while packing more bits from C++ types, no matter what I packed, I would get 1s on the reading side. I fixed it by making bp_pack_value chop VAL so that only the first NBITS are stored. Alternately, we could just assert that the caller didn't send a value that overflows NBITS. Richi, do you prefer one over the other? Diego. * lto-streamer.h (bitpack_create): Only use the lower NBITS from VAL. diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index 5f56fc6..0d49430 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -1190,8 +1190,19 @@ bitpack_create (struct lto_output_stream *s) static inline void bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits) { - bitpack_word_t word = bp->word; + bitpack_word_t mask, word; int pos = bp->pos; + + word = bp->word; + + gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD); + + /* Make sure that VAL only has the lower NBITS set. Generate a + mask with the lower NBITS set and use it to filter the upper + bits from VAL. */ + mask = ((bitpack_word_t) 1 << nbits) - 1; + val = val & mask; + /* If val does not fit into the current bitpack word switch to the next one. */ if (pos + nbits > BITS_PER_BITPACK_WORD) -- This patch is available for review at http://codereview.appspot.com/4415044 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lto/pph] Do not pack more bits than requested (issue4415044) 2011-04-14 19:01 [lto/pph] Do not pack more bits than requested (issue4415044) Diego Novillo @ 2011-04-14 19:40 ` Jakub Jelinek 2011-04-14 21:04 ` Diego Novillo 0 siblings, 1 reply; 7+ messages in thread From: Jakub Jelinek @ 2011-04-14 19:40 UTC (permalink / raw) To: Diego Novillo; +Cc: reply, rguenther, gcc-patches On Thu, Apr 14, 2011 at 02:50:53PM -0400, Diego Novillo wrote: > @@ -1190,8 +1190,19 @@ bitpack_create (struct lto_output_stream *s) > static inline void > bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits) > { > - bitpack_word_t word = bp->word; > + bitpack_word_t mask, word; > int pos = bp->pos; > + > + word = bp->word; > + > + gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD); > + > + /* Make sure that VAL only has the lower NBITS set. Generate a > + mask with the lower NBITS set and use it to filter the upper > + bits from VAL. */ > + mask = ((bitpack_word_t) 1 << nbits) - 1; If bitpack_word_t has BITS_PER_BITPACK_WORD bits, then for nbits = BITS_PER_BITPACK_WORD this will be undefined. Use say mask = ((bitpack_word_t) 2 << (nbits - 1)) - 1; or something similar (assertion ensures that nbits isn't 0). Jakub ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lto/pph] Do not pack more bits than requested (issue4415044) 2011-04-14 19:40 ` Jakub Jelinek @ 2011-04-14 21:04 ` Diego Novillo 2011-04-15 9:13 ` Richard Guenther 0 siblings, 1 reply; 7+ messages in thread From: Diego Novillo @ 2011-04-14 21:04 UTC (permalink / raw) To: Jakub Jelinek; +Cc: reply, rguenther, gcc-patches On Thu, Apr 14, 2011 at 15:28, Jakub Jelinek <jakub@redhat.com> wrote: > If bitpack_word_t has BITS_PER_BITPACK_WORD bits, then for > nbits = BITS_PER_BITPACK_WORD this will be undefined. > Use say > mask = ((bitpack_word_t) 2 << (nbits - 1)) - 1; > or something similar (assertion ensures that nbits isn't 0). Quite right, thanks. In the meantime, I've changed my mind with this. I think it's safer if we just assert that the value we are about to pack fit in the number of bits the caller specified. The only problematic user is pack_ts_type_value_fields when it tries to pack a -1 for the type's alias set. I think we should just stream that as an integer and not go through the bitpacking overhead. For now, I'm applying this to the pph branch. Tested on x86_64. No LTO failures. Diego. * lto-streamer-out.c (pack_ts_type_value_fields): Pack all bits of -1 value. * lto-streamer.h (bitpack_create): Assert that the value to pack does not overflow NBITS. * lto-streamer-in.c (unpack_ts_type_value_fields): Unpack BITS_PER_BITPACK_WORD bits for TYPE_ALIAS_SET. diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c index 97b86ce..f04e031 100644 --- a/gcc/lto-streamer-in.c +++ b/gcc/lto-streamer-in.c @@ -1734,7 +1734,7 @@ unpack_ts_type_value_fields (struct bitpack_d *bp, tree expr) TYPE_USER_ALIGN (expr) = (unsigned) bp_unpack_value (bp, 1); TYPE_READONLY (expr) = (unsigned) bp_unpack_value (bp, 1); TYPE_ALIGN (expr) = (unsigned) bp_unpack_value (bp, HOST_BITS_PER_INT); - TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, HOST_BITS_PER_INT); + TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, BITS_PER_BITPACK_WORD); } diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c index 3ccad8b..89ad9c5 100644 --- a/gcc/lto-streamer-out.c +++ b/gcc/lto-streamer-out.c @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr) bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1); bp_pack_value (bp, TYPE_READONLY (expr), 1); bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT); - bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT); + bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, + BITS_PER_BITPACK_WORD); } diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index 0d49430..73afd46 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -1190,18 +1190,15 @@ bitpack_create (struct lto_output_stream *s) static inline void bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits) { - bitpack_word_t mask, word; + bitpack_word_t word = bp->word; int pos = bp->pos; - word = bp->word; - + /* We shouldn't try to pack more bits than can fit in a bitpack word. */ gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD); - /* Make sure that VAL only has the lower NBITS set. Generate a - mask with the lower NBITS set and use it to filter the upper - bits from VAL. */ - mask = ((bitpack_word_t) 1 << nbits) - 1; - val = val & mask; + /* The value to pack should not overflow NBITS. */ + gcc_assert (nbits == BITS_PER_BITPACK_WORD + || val <= ((bitpack_word_t) 1 << nbits)); /* If val does not fit into the current bitpack word switch to the next one. */ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lto/pph] Do not pack more bits than requested (issue4415044) 2011-04-14 21:04 ` Diego Novillo @ 2011-04-15 9:13 ` Richard Guenther 2011-04-15 12:50 ` Diego Novillo 0 siblings, 1 reply; 7+ messages in thread From: Richard Guenther @ 2011-04-15 9:13 UTC (permalink / raw) To: Diego Novillo; +Cc: Jakub Jelinek, reply, gcc-patches On Thu, 14 Apr 2011, Diego Novillo wrote: > On Thu, Apr 14, 2011 at 15:28, Jakub Jelinek <jakub@redhat.com> wrote: > > > If bitpack_word_t has BITS_PER_BITPACK_WORD bits, then for > > nbits = BITS_PER_BITPACK_WORD this will be undefined. > > Use say > > mask = ((bitpack_word_t) 2 << (nbits - 1)) - 1; > > or something similar (assertion ensures that nbits isn't 0). > > Quite right, thanks. In the meantime, I've changed my mind with this. > I think it's safer if we just assert that the value we are about to > pack fit in the number of bits the caller specified. > > The only problematic user is pack_ts_type_value_fields when it tries > to pack a -1 for the type's alias set. I think we should just stream > that as an integer and not go through the bitpacking overhead. > > For now, I'm applying this to the pph branch. Tested on x86_64. No > LTO failures. See below > > Diego. > > * lto-streamer-out.c (pack_ts_type_value_fields): Pack all bits > of -1 value. > * lto-streamer.h (bitpack_create): Assert that the value to > pack does not overflow NBITS. > * lto-streamer-in.c (unpack_ts_type_value_fields): Unpack > BITS_PER_BITPACK_WORD bits for TYPE_ALIAS_SET. > > diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c > index 97b86ce..f04e031 100644 > --- a/gcc/lto-streamer-in.c > +++ b/gcc/lto-streamer-in.c > @@ -1734,7 +1734,7 @@ unpack_ts_type_value_fields (struct bitpack_d > *bp, tree expr) > TYPE_USER_ALIGN (expr) = (unsigned) bp_unpack_value (bp, 1); > TYPE_READONLY (expr) = (unsigned) bp_unpack_value (bp, 1); > TYPE_ALIGN (expr) = (unsigned) bp_unpack_value (bp, HOST_BITS_PER_INT); > - TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, HOST_BITS_PER_INT); > + TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, BITS_PER_BITPACK_WORD); > } > > > diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c > index 3ccad8b..89ad9c5 100644 > --- a/gcc/lto-streamer-out.c > +++ b/gcc/lto-streamer-out.c > @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr) > bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1); > bp_pack_value (bp, TYPE_READONLY (expr), 1); > bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT); > - bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT); > + bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, > + BITS_PER_BITPACK_WORD); As we only want to stream alias-set zeros just change it to a single bit, like bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1); and on the reader side restore either a zero or -1. > } > > > diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h > index 0d49430..73afd46 100644 > --- a/gcc/lto-streamer.h > +++ b/gcc/lto-streamer.h > @@ -1190,18 +1190,15 @@ bitpack_create (struct lto_output_stream *s) > static inline void > bp_pack_value (struct bitpack_d *bp, bitpack_word_t val, unsigned nbits) > { > - bitpack_word_t mask, word; > + bitpack_word_t word = bp->word; > int pos = bp->pos; > > - word = bp->word; > - > + /* We shouldn't try to pack more bits than can fit in a bitpack word. */ > gcc_assert (nbits > 0 && nbits <= BITS_PER_BITPACK_WORD); Asserts will break merging the operations and make them slow again. Please no asserts in these core routines. Look at the .optimized dump of a series of bp_pack_value, they should be basically optimized to a series of ORs. As for the -1 case, it's simply broken use of the interface. Richard. > - /* Make sure that VAL only has the lower NBITS set. Generate a > - mask with the lower NBITS set and use it to filter the upper > - bits from VAL. */ > - mask = ((bitpack_word_t) 1 << nbits) - 1; > - val = val & mask; > + /* The value to pack should not overflow NBITS. */ > + gcc_assert (nbits == BITS_PER_BITPACK_WORD > + || val <= ((bitpack_word_t) 1 << nbits)); > > /* If val does not fit into the current bitpack word switch to the > next one. */ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lto/pph] Do not pack more bits than requested (issue4415044) 2011-04-15 9:13 ` Richard Guenther @ 2011-04-15 12:50 ` Diego Novillo 2011-04-15 13:00 ` Richard Guenther 0 siblings, 1 reply; 7+ messages in thread From: Diego Novillo @ 2011-04-15 12:50 UTC (permalink / raw) To: Richard Guenther; +Cc: Jakub Jelinek, reply, gcc-patches On Fri, Apr 15, 2011 at 04:56, Richard Guenther <rguenther@suse.de> wrote: >> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr) >> bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1); >> bp_pack_value (bp, TYPE_READONLY (expr), 1); >> bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT); >> - bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT); >> + bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, >> + BITS_PER_BITPACK_WORD); > > As we only want to stream alias-set zeros just change it to a single bit, > like > > bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1); > > and on the reader side restore either a zero or -1. Ah, yes. Much better. > As for the -1 case, it's simply broken use of the interface. Which would've been caught by the assertion. How about this, we keep the asserts with #ifdef ENABLE_CHECKING. This would've saved me some ugly debugging. Diego. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lto/pph] Do not pack more bits than requested (issue4415044) 2011-04-15 12:50 ` Diego Novillo @ 2011-04-15 13:00 ` Richard Guenther 2011-04-15 13:19 ` Diego Novillo 0 siblings, 1 reply; 7+ messages in thread From: Richard Guenther @ 2011-04-15 13:00 UTC (permalink / raw) To: Diego Novillo; +Cc: Jakub Jelinek, reply, gcc-patches [-- Attachment #1: Type: TEXT/PLAIN, Size: 1292 bytes --] On Fri, 15 Apr 2011, Diego Novillo wrote: > On Fri, Apr 15, 2011 at 04:56, Richard Guenther <rguenther@suse.de> wrote: > > >> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr) > >> Â Â bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1); > >> Â Â bp_pack_value (bp, TYPE_READONLY (expr), 1); > >> Â Â bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT); > >> - Â bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT); > >> + Â bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, > >> + Â Â Â Â Â Â Â Â BITS_PER_BITPACK_WORD); > > > > As we only want to stream alias-set zeros just change it to a single bit, > > like > > > > Â Â bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1); > > > > and on the reader side restore either a zero or -1. > > Ah, yes. Much better. > > > As for the -1 case, it's simply broken use of the interface. > > Which would've been caught by the assertion. How about this, we keep > the asserts with #ifdef ENABLE_CHECKING. This would've saved me some > ugly debugging. I think we should rather add the masking. The assert would only trigger for particular values, not for bogus use of the interface (you get sign-extension for signed arguments). Richard. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lto/pph] Do not pack more bits than requested (issue4415044) 2011-04-15 13:00 ` Richard Guenther @ 2011-04-15 13:19 ` Diego Novillo 0 siblings, 0 replies; 7+ messages in thread From: Diego Novillo @ 2011-04-15 13:19 UTC (permalink / raw) To: Richard Guenther; +Cc: Jakub Jelinek, reply, gcc-patches On Fri, Apr 15, 2011 at 08:49, Richard Guenther <rguenther@suse.de> wrote: > On Fri, 15 Apr 2011, Diego Novillo wrote: > >> On Fri, Apr 15, 2011 at 04:56, Richard Guenther <rguenther@suse.de> wrote: >> >> >> @@ -518,7 +518,8 @@ pack_ts_type_value_fields (struct bitpack_d *bp, tree expr) >> >> bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1); >> >> bp_pack_value (bp, TYPE_READONLY (expr), 1); >> >> bp_pack_value (bp, TYPE_ALIGN (expr), HOST_BITS_PER_INT); >> >> - bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, HOST_BITS_PER_INT); >> >> + bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0 ? 0 : -1, >> >> + BITS_PER_BITPACK_WORD); >> > >> > As we only want to stream alias-set zeros just change it to a single bit, >> > like >> > >> > bp_pack_value (bp, TYPE_ALIAS_SET (expr) == 0, 1); >> > >> > and on the reader side restore either a zero or -1. >> >> Ah, yes. Much better. >> >> > As for the -1 case, it's simply broken use of the interface. >> >> Which would've been caught by the assertion. How about this, we keep >> the asserts with #ifdef ENABLE_CHECKING. This would've saved me some >> ugly debugging. > > I think we should rather add the masking. The assert would only > trigger for particular values, not for bogus use of the interface > (you get sign-extension for signed arguments). OK, that works too. I'll prepare a patch. Diego. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-04-15 13:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-14 19:01 [lto/pph] Do not pack more bits than requested (issue4415044) Diego Novillo 2011-04-14 19:40 ` Jakub Jelinek 2011-04-14 21:04 ` Diego Novillo 2011-04-15 9:13 ` Richard Guenther 2011-04-15 12:50 ` Diego Novillo 2011-04-15 13:00 ` Richard Guenther 2011-04-15 13:19 ` Diego Novillo
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).