On Wed, May 25, 2011 at 11:45 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> Hi,
> after fixing 1 byte i/o function call and most of hash table overhead,
> functions to handle ulebs and slebs shows top in profile. We use them in
> many cases where we know value range of the operand will fit in 1 byte. In
> particular to handle enums.
> This is also dangerous since we generally assume enums to be in their value
> range.
>
> This patch adds i/o bits for enums and integers in range that should inline
> well and add some sanity checking.
>
> I converted only tree streamer tags, but if accepted, I will convert more.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> * lto-streamer-out.c (output_record_start): Use lto_output_enum
> (lto_output_tree): Use output_record_start.
> * lto-streamer-in.c (input_record_start): Use lto_input_enum
> (lto_get_pickled_tree): Use input_record_start.
> * lto-section-in.c (lto_section_overrun): Turn into fatal error.
> (lto_value_range_error): New function.
> * lto-streamer.h (lto_value_range_error): Declare.
> (lto_output_int_in_range, lto_input_int_in_range): New functions.
> (lto_output_enum, lto_input_enum): New macros.
> Index: lto-streamer-out.c
> ===================================================================
> *** lto-streamer-out.c (revision 174175)
> --- lto-streamer-out.c (working copy)
> *************** output_sleb128 (struct output_block *ob,
> *** 270,281 ****
>
> /* Output the start of a record with TAG to output block OB. */
>
> ! static void
> output_record_start (struct output_block *ob, enum LTO_tags tag)
> {
> ! /* Make sure TAG fits inside an unsigned int. */
> ! gcc_assert (tag == (enum LTO_tags) (unsigned) tag);
> ! output_uleb128 (ob, tag);
> }
>
>
> --- 270,279 ----
>
> /* Output the start of a record with TAG to output block OB. */
>
> ! static inline void
> output_record_start (struct output_block *ob, enum LTO_tags tag)
> {
> ! lto_output_enum (ob->main_stream, LTO_tags, LTO_NUM_TAGS, tag);
> }
>
>
> *************** lto_output_tree (struct output_block *ob
> *** 1401,1407 ****
> will instantiate two different nodes for the same object. */
> output_record_start (ob, LTO_tree_pickle_reference);
> output_uleb128 (ob, ix);
> ! output_uleb128 (ob, lto_tree_code_to_tag (TREE_CODE (expr)));
> }
> else if (lto_stream_as_builtin_p (expr))
> {
> --- 1399,1405 ----
> will instantiate two different nodes for the same object. */
> output_record_start (ob, LTO_tree_pickle_reference);
> output_uleb128 (ob, ix);
> ! output_record_start (ob, lto_tree_code_to_tag (TREE_CODE (expr)));
I'd prefer lto_output_enum here as we don't really start a new output
record but just emit something for a sanity check.
> }
> else if (lto_stream_as_builtin_p (expr))
> {
> Index: lto-streamer-in.c
> ===================================================================
> *** lto-streamer-in.c (revision 174175)
> --- lto-streamer-in.c (working copy)
> *************** lto_input_string (struct data_in *data_i
> *** 231,241 ****
>
> /* Return the next tag in the input block IB. */
>
> ! static enum LTO_tags
> input_record_start (struct lto_input_block *ib)
> {
> ! enum LTO_tags tag = (enum LTO_tags) lto_input_uleb128 (ib);
> ! return tag;
> }
>
>
> --- 231,240 ----
>
> /* Return the next tag in the input block IB. */
>
> ! static inline enum LTO_tags
> input_record_start (struct lto_input_block *ib)
> {
> ! return lto_input_enum (ib, LTO_tags, LTO_NUM_TAGS);
> }
>
>
> *************** lto_get_pickled_tree (struct lto_input_b
> *** 2558,2564 ****
> enum LTO_tags expected_tag;
>
> ix = lto_input_uleb128 (ib);
> ! expected_tag = (enum LTO_tags) lto_input_uleb128 (ib);
>
> result = lto_streamer_cache_get (data_in->reader_cache, ix);
> gcc_assert (result
> --- 2557,2563 ----
> enum LTO_tags expected_tag;
>
> ix = lto_input_uleb128 (ib);
> ! expected_tag = input_record_start (ib);
Likewise use input_enum.
> result = lto_streamer_cache_get (data_in->reader_cache, ix);
> gcc_assert (result
> Index: lto-section-in.c
> ===================================================================
> *** lto-section-in.c (revision 174175)
> --- lto-section-in.c (working copy)
> *************** lto_get_function_in_decl_state (struct l
> *** 483,488 ****
> void
> lto_section_overrun (struct lto_input_block *ib)
> {
> ! internal_error ("bytecode stream: trying to read %d bytes "
> ! "after the end of the input buffer", ib->p - ib->len);
> }
> --- 483,498 ----
> void
> lto_section_overrun (struct lto_input_block *ib)
> {
> ! fatal_error ("bytecode stream: trying to read %d bytes "
> ! "after the end of the input buffer", ib->p - ib->len);
> ! }
> !
> ! /* Report out of range value. */
> !
> ! void
> ! lto_value_range_error (const char *purpose, HOST_WIDE_INT val,
> ! HOST_WIDE_INT min, HOST_WIDE_INT max)
> ! {
> ! fatal_error ("%s out of range: Range is %i to %i, value is %i",
> ! purpose, (int)min, (int)max, (int)val);
> }
> Index: lto-streamer.h
> ===================================================================
> *** lto-streamer.h (revision 174175)
> --- lto-streamer.h (working copy)
> *************** extern int lto_eq_in_decl_state (const v
> *** 771,776 ****
> --- 771,779 ----
> extern struct lto_in_decl_state *lto_get_function_in_decl_state (
> struct lto_file_decl_data *, tree);
> extern void lto_section_overrun (struct lto_input_block *)
> ATTRIBUTE_NORETURN;
> + extern void lto_value_range_error (const char *,
> + HOST_WIDE_INT, HOST_WIDE_INT,
> + HOST_WIDE_INT) ATTRIBUTE_NORETURN;
>
> /* In lto-section-out.c */
> extern hashval_t lto_hash_decl_slot_node (const void *);
> *************** lto_input_1_unsigned (struct lto_input_b
> *** 1199,1202 ****
> --- 1202,1267 ----
> return (ib->data[ib->p++]);
> }
>
> + /* Output VAL into OBS and verify it is in range MIN...MAX that is supposed
> + to be compile time constant.
> + Be host independent, limit range to 31bits. */
> +
> + static inline void
> + lto_output_int_in_range (struct lto_output_stream *obs,
> + HOST_WIDE_INT min,
> + HOST_WIDE_INT max,
> + HOST_WIDE_INT val)
> + {
> + HOST_WIDE_INT range = max - min;
> +
> + gcc_checking_assert (val >= min && val <= max && range > 0
> + && range < 0x7fffffff);
> +
> + val -= min;
> + lto_output_1_stream (obs, val & 255);
> + if (range >= 0xff)
> + lto_output_1_stream (obs, (val << 8) & 255);
> + if (range >= 0xffff)
> + lto_output_1_stream (obs, (val << 16) & 255);
> + if (range >= 0xffffff)
> + lto_output_1_stream (obs, (val << 24) & 255);
so you didn't want to create a bitpack_pack_int_in_range and use
bitpacks for enums? I suppose for some of the remaining cases
packing them into existing bitpacks would be preferable?
> + }
> +
> + /* Input VAL into OBS and verify it is in range MIN...MAX that is supposed
> + to be compile time constant. PURPOSE is used for error reporting. */
> +
> + static inline HOST_WIDE_INT
> + lto_input_int_in_range (struct lto_input_block *ib,
> + const char *purpose,
> + HOST_WIDE_INT min,
> + HOST_WIDE_INT max)
> + {
> + HOST_WIDE_INT range = max - min;
> + HOST_WIDE_INT val = lto_input_1_unsigned (ib);
> +
> + gcc_checking_assert (range > 0);
The assert doesn't match the one during output.
> +
> + if (range >= 0xff)
> + val |= ((HOST_WIDE_INT)lto_input_1_unsigned (ib)) << 8;
> + if (range >= 0xffff)
> + val |= ((HOST_WIDE_INT)lto_input_1_unsigned (ib)) << 16;
> + if (range >= 0xffffff)
> + val |= ((HOST_WIDE_INT)lto_input_1_unsigned (ib)) << 24;
> + val += min;
> + if (val < min || val > max)
> + lto_value_range_error (purpose, val, min, max);
> + return val;
> + }
> +
> + /* Output VAL of type "enum enum_name" into OBS.
> + Assume range 0...ENUM_LAST - 1. */
> + #define lto_output_enum(obs,enum_name,enum_last,val) \
> + lto_output_int_in_range ((obs), 0, (int)(enum_last) - 1, (int)(val))
> +
> + /* Input enum of type "enum enum_name" from IB.
> + Assume range 0...ENUM_LAST - 1. */
> + #define lto_input_enum(ib,enum_name,enum_last) \
> + (enum enum_name)lto_input_int_in_range ((ib), #enum_name, 0, \
> + (int)(enum_last) - 1)
> +
> #endif /* GCC_LTO_STREAMER_H */
>
