Re: [lto/pph] Do not pack more bits than requested (issue4415044)
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. */
Re: [lto/pph] Do not pack more bits than requested (issue4415044)
On Fri, Apr 15, 2011 at 04:56, Richard Guenther rguent...@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.
Re: [lto/pph] Do not pack more bits than requested (issue4415044)
On Fri, 15 Apr 2011, Diego Novillo wrote: On Fri, Apr 15, 2011 at 04:56, Richard Guenther rguent...@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.
Re: [lto/pph] Do not pack more bits than requested (issue4415044)
On Fri, Apr 15, 2011 at 08:49, Richard Guenther rguent...@suse.de wrote: On Fri, 15 Apr 2011, Diego Novillo wrote: On Fri, Apr 15, 2011 at 04:56, Richard Guenther rguent...@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.
Re: [lto/pph] Do not pack more bits than requested (issue4415044)
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
