Re: Better location streaming

[Richard Guenther <richard.guenther@gmail.com>]

On Thu, May 26, 2011 at 11:34 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> we spend a lot of effort (and disk space) into streaming file/line/column and
> sys_p fields of locations. Since often the trees come from same statement, it
> is common for those to not change.  We even already have current location info
> in output_block and data_in, but don't use it.
>
> This patch update streaming to stream first bitpack of what changed and then
> streaming only the changed locations.  Because I did not find any use of
> insys_p field in the backend, I removed streaming it (though it would be easy
> doable to stream it along the file info).
>
> Because we need only 4 bits, it would make a lot of sense to merge location
> bitpack with the other bitpack and for this reason I split the i/o intput part
> doing bitpack and part streaming the actual data.  At the moment we always do
> that at once, since doing anything saner needs a bit of reorganization of how
> we stream trees.
>
> Regtested x86_64-linux, full bootstrap&regtest running, OK?

This looks all very hackish with no immediate benefit mostly because
of the use of lto_output_string.  I think what you should do instead
is split up lto_output_string_with_length into the piece that streams
the string itself to the string-stream and returns an index into it
and the piece streaming the index to the specified stream.  Then you
can simply bitpack that index and the two int line / column fields.

Richard.

> Honza
>
>        * lto-streamer-out.c (clear_line_info): Set location_output_done.
>        (lto_output_location_bitpack): New function.
>        (lto_output_location_data): New function.
>        (lto_output_location): Reorg.
>        * lto-streamer-in.c (clear_line_info): Set location_input_done.
>        (lto_input_location_bitpack): New function.
>        (lto_input_location_data): New function.
>        (lto_input_location): Reorg.
>        * lto-streamer.h (struct output_block): Add output_file, output_line,
>        output_col and location_output_done flags.
>        (struct data_in): Add input_file, input_line, input_col and
>        location_input_done flags.
> Index: lto-streamer-out.c
> ===================================================================
> --- lto-streamer-out.c  (revision 174264)
> +++ lto-streamer-out.c  (working copy)
> @@ -95,6 +95,7 @@ clear_line_info (struct output_block *ob
>   ob->current_file = NULL;
>   ob->current_line = 0;
>   ob->current_col = 0;
> +  ob->location_output_done = 1;
>  }
>
>
> @@ -587,29 +588,72 @@ pack_value_fields (struct bitpack_d *bp,
>  }
>
>
> -/* Emit location LOC to output block OB.  */
> +/* Output info about new location into bitpack BP.
> +   After outputting bitpack, lto_output_location_data has
> +   to be done to output actual data.  */
> +
> +static inline void
> +lto_output_location_bitpack (struct bitpack_d *bp,
> +                            struct output_block *ob,
> +                            location_t loc)
> +{
> +  gcc_checking_assert (ob->location_output_done);
> +  bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1);
> +  if (loc == UNKNOWN_LOCATION)
> +    ob->output_file = ob->output_line = ob->output_col = 0;
> +  else
> +    {
> +      expanded_location xloc;
> +
> +      xloc = expand_location (loc);
> +
> +      ob->output_file = xloc.file != ob->current_file;
> +      ob->output_line = xloc.line != ob->current_line;
> +      ob->output_col = xloc.column != ob->current_col;
> +    }
> +  bp_pack_value (bp, ob->output_file, 1);
> +  bp_pack_value (bp, ob->output_line, 1);
> +  bp_pack_value (bp, ob->output_col, 1);
> +  ob->location_output_done = false;
> +}
> +
> +
> +/* Output location info we prepared to output in
> +   lto_output_location_bitpack  */
>
>  static void
> -lto_output_location (struct output_block *ob, location_t loc)
> +lto_output_location_data (struct output_block *ob)
>  {
> -  expanded_location xloc;
> +  gcc_checking_assert (!ob->location_output_done);
>
> -  if (loc == UNKNOWN_LOCATION)
> +  if (ob->output_file)
> +    lto_output_string (ob, ob->main_stream, ob->current_file);
> +  if (ob->output_line)
>     {
> -      lto_output_string (ob, ob->main_stream, NULL);
> -      return;
> +      gcc_checking_assert (ob->current_line >= 0);
> +      output_uleb128 (ob, ob->current_line);
>     }
> +  if (ob->output_col)
> +    {
> +      gcc_checking_assert (ob->current_col >= 0);
> +      output_uleb128 (ob, ob->current_col);
> +    }
> +  ob->location_output_done = true;
> +}
>
> -  xloc = expand_location (loc);
>
> -  lto_output_string (ob, ob->main_stream, xloc.file);
> -  output_sleb128 (ob, xloc.line);
> -  output_sleb128 (ob, xloc.column);
> -  output_sleb128 (ob, xloc.sysp);
> -
> -  ob->current_file = xloc.file;
> -  ob->current_line = xloc.line;
> -  ob->current_col = xloc.column;
> +/* Emit location LOC to output block OB.
> +   When bitpack is handy, it is more space effecient to call
> +   lto_output_location_bitpack and lto_output_location_data directly
> +   adding info into the existing bitpack.  */
> +
> +static void
> +lto_output_location (struct output_block *ob, location_t loc)
> +{
> +  struct bitpack_d bp = bitpack_create (ob->main_stream);
> +  lto_output_location_bitpack (&bp, ob, loc);
> +  lto_output_bitpack (&bp);
> +  lto_output_location_data (ob);
>  }
>
>
> @@ -642,7 +686,7 @@ lto_output_tree_ref (struct output_block
>
>   if (expr == NULL_TREE)
>     {
> -      output_zero (ob);
> +      output_record_start (ob, LTO_null);

?

>       return;
>     }
>
> Index: lto-streamer-in.c
> ===================================================================
> --- lto-streamer-in.c   (revision 174264)
> +++ lto-streamer-in.c   (working copy)
> @@ -281,40 +281,69 @@ clear_line_info (struct data_in *data_in
>   data_in->current_file = NULL;
>   data_in->current_line = 0;
>   data_in->current_col = 0;
> +  data_in->location_input_done = true;
>  }
>
>
> -/* Read a location from input block IB.  */
> +/* Read a location bitpack from input block IB.  */
> +
> +static inline void
> +lto_input_location_bitpack (struct data_in *data_in, struct bitpack_d *bp)
> +{
> +  gcc_checking_assert (data_in->location_input_done);
> +  data_in->location_input_done = false;
> +  data_in->input_unknown_location = bp_unpack_value (bp, 1);
> +  data_in->input_file_p = bp_unpack_value (bp, 1);
> +  data_in->input_line_p = bp_unpack_value (bp, 1);
> +  data_in->input_col_p = bp_unpack_value (bp, 1);
> +}
> +
> +
> +/* Read locaiton data we determined to change and update info.  */
>
>  static location_t
> -lto_input_location (struct lto_input_block *ib, struct data_in *data_in)
> +lto_input_location_data (struct lto_input_block *ib, struct data_in *data_in)
>  {
> -  expanded_location xloc;
> +  bool prev_file = data_in->current_file != NULL;
>
> -  xloc.file = lto_input_string (data_in, ib);
> -  if (xloc.file == NULL)
> +  gcc_checking_assert (!data_in->location_input_done);
> +  data_in->location_input_done = true;
> +  if (data_in->input_unknown_location)
>     return UNKNOWN_LOCATION;
> +  if (data_in->input_file_p)
> +    {
> +      data_in->current_file = lto_input_string (data_in, ib);
> +      data_in->current_file = canon_file_name (data_in->current_file);
> +    }
> +  if (data_in->input_line_p)
> +    data_in->current_line = lto_input_uleb128 (ib);
> +  if (data_in->input_col_p)
> +    data_in->current_col = lto_input_uleb128 (ib);
>
> -  xloc.file = canon_file_name (xloc.file);
> -  xloc.line = lto_input_sleb128 (ib);
> -  xloc.column = lto_input_sleb128 (ib);
> -  xloc.sysp = lto_input_sleb128 (ib);
> -
> -  if (data_in->current_file != xloc.file)
> +  if (data_in->input_file_p)
>     {
> -      if (data_in->current_file)
> +      if (prev_file)
>        linemap_add (line_table, LC_LEAVE, false, NULL, 0);
>
> -      linemap_add (line_table, LC_ENTER, xloc.sysp, xloc.file, xloc.line);
> +      linemap_add (line_table, LC_ENTER, false, data_in->current_file, data_in->current_line);
>     }
> -  else if (data_in->current_line != xloc.line)
> -    linemap_line_start (line_table, xloc.line, xloc.column);
> +  else if (data_in->input_line_p)
> +    linemap_line_start (line_table, data_in->current_line, data_in->current_col);
>
> -  data_in->current_file = xloc.file;
> -  data_in->current_line = xloc.line;
> -  data_in->current_col = xloc.column;
> +  return linemap_position_for_column (line_table, data_in->current_col);
> +}
> +
> +
> +/* Read a location from input block IB.  */
> +
> +static location_t
> +lto_input_location (struct lto_input_block *ib, struct data_in *data_in)
> +{
> +  struct bitpack_d bp;
>
> -  return linemap_position_for_column (line_table, xloc.column);
> +  bp = lto_input_bitpack (ib);
> +  lto_input_location_bitpack (data_in, &bp);
> +  return lto_input_location_data (ib, data_in);
>  }
>
>
> Index: lto-streamer.h
> ===================================================================
> --- lto-streamer.h      (revision 174264)
> +++ lto-streamer.h      (working copy)
> @@ -694,6 +694,10 @@ struct output_block
>   const char *current_file;
>   int current_line;
>   int current_col;
> +  unsigned output_file : 1;
> +  unsigned output_line : 1;
> +  unsigned output_col : 1;
> +  unsigned location_output_done : 1;
>
>   /* True if writing globals and types.  */
>   bool global;
> @@ -728,6 +732,11 @@ struct data_in
>   const char *current_file;
>   int current_line;
>   int current_col;
> +  unsigned input_unknown_location : 1;
> +  unsigned input_file_p : 1;
> +  unsigned input_line_p : 1;
> +  unsigned input_col_p : 1;
> +  unsigned location_input_done : 1;
>
>   /* Maps each reference number to the resolution done by the linker. */
>   VEC(ld_plugin_symbol_resolution_t,heap) *globals_resolution;
>