On Thu, 14 Apr 2011, Diego Novillo wrote:
> On Thu, Apr 14, 2011 at 15:28, Jakub Jelinek <ja...@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. */
