Re: Faster streaming of enums

[Richard Guenther <richard.guenther@gmail.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  */
>