Re: Faster streaming of enums

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, May 25, 2011 at 11:45 AM, Jan Hubicka <hubicka@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 ?*/
>