> >
> > + lto_apply_location_cache ();
> > +
>
> Please add a comment here. It might be possible to write a
Will do.
> lto_location_cache_decl_source_location (TYPE_NAME (t1)) that
> looks if the location is in the cache and otherwise falls back to
> DECL_SOURCE_LOCATION. That would be kind-of slow, of course.
>
> Seems to me the above is at least unnecessary if -Wno-odr is in
> effect? (ah, that's enabled by default...)
Yep, also we output just few warning per unit so flushing is OK.
We do not really need to cache everything, comitting in resonably large
chunks works similarly well.
> > @@ -1953,7 +1955,10 @@ lto_read_decls (struct lto_file_decl_dat
> > /* Register TYPE_DECLs with the debuginfo machinery. */
> > if (!flag_wpa
> > && TREE_CODE (t) == TYPE_DECL)
> > - debug_hooks->type_decl (t, !DECL_FILE_SCOPE_P (t));
> > + {
> > + lto_apply_location_cache ();
>
> Did you check the effect of this? That is, if you remove this call
> (you get bogus debug, yes...), what does it do to the memory usage
> figures? We might want to simply queue those TYPE_DECLs in a
> vector and call type_decl () on them after the lto_apply_location_cache
> call below.
Yep, this one is bit more worrysome. It happens only during ltrans that has
a lot fewer locations to deal with at first place though.
I will check the usage - one thing I was looking into is to simply delay calling
the hook. I.e. collect these into an array or move them to later pass.
Lets try to do this incrementally.
> > -location_t
> > -lto_input_location (struct bitpack_d *bp, struct data_in *data_in)
> > +static const char *current_file;
> > +static int current_line;
> > +static int current_col;
> > +static location_t current_loc;
>
> Heh, these days we'd encapsulate all this into a nice C++ class and go
> with a single global 'location_cache' class instance ... ;) (ok, I don't
> mind too much)
Hmm, that makes sense indeed ;)
> > - return UNKNOWN_LOCATION;
> > + {
> > + *loc = UNKNOWN_LOCATION;
> > + return;
> > + }
> > + *loc = BUILTINS_LOCATION + 1;
> >
>
> Any special reason to use BUILTINS_LOCATION + 1 here instead of
> unconditionally doing
>
> *loc = UNKNOWN_LOCATION;
> if (bp_unpack_value (bp, 1))
> return;
>
> ?
Yes, I used UNKNOWN_LOCATION prevoiusly. It ICEs because we consider then
all decls bulitin
#define DECL_IS_BUILTIN(DECL) \
(LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION)
And we have sanity check about not streaming bulitins (that can not fire,
because we do not know how to stream builtin location)
> > + *loc = current_loc;
> > + return;
>
> Does that really happen in practice? ;)
It does, when streaming gimple statements and using the uncached path.
Thanks,
Honza
>
> The rest looks ok to me.
>
> Thanks,
> Richard.
>
> > }
> > - else if (line_change)
> > - linemap_line_start (line_table, current_line, current_col);
> >
> > - return linemap_position_for_column (line_table, current_col);
> > + struct cached_location entry = {stream_file, loc, stream_line,
> > stream_col};
> > + loc_cache.safe_push (entry);
> > }
> >
> >
> > @@ -390,7 +514,7 @@ input_eh_region (struct lto_input_block
> > r->u.must_not_throw.failure_decl = stream_read_tree (ib, data_in);
> > bitpack_d bp = streamer_read_bitpack (ib);
> > r->u.must_not_throw.failure_loc
> > - = stream_input_location (&bp, data_in);
> > + = stream_input_location_now (&bp, data_in);
> > }
> > break;
> >
> > @@ -927,8 +1051,8 @@ input_struct_function_base (struct funct
> > fn->last_clique = bp_unpack_value (&bp, sizeof (short) * 8);
> >
> > /* Input the function start and end loci. */
> > - fn->function_start_locus = stream_input_location (&bp, data_in);
> > - fn->function_end_locus = stream_input_location (&bp, data_in);
> > + fn->function_start_locus = stream_input_location_now (&bp, data_in);
> > + fn->function_end_locus = stream_input_location_now (&bp, data_in);
> > }
> >
> >
> > @@ -1126,6 +1250,7 @@ lto_read_body_or_constructor (struct lto
> > }
> > else
> > input_constructor (fn_decl, data_in, &ib_main);
> > + lto_apply_location_cache ();
> > /* And fixup types we streamed locally. */
> > {
> > struct streamer_tree_cache_d *cache = data_in->reader_cache;
> > Index: tree-streamer-in.c
> > ===================================================================
> > --- tree-streamer-in.c (revision 221582)
> > +++ tree-streamer-in.c (working copy)
> > @@ -411,7 +411,7 @@ unpack_ts_block_value_fields (struct dat
> > {
> > BLOCK_ABSTRACT (expr) = (unsigned) bp_unpack_value (bp, 1);
> > /* BLOCK_NUMBER is recomputed. */
> > - BLOCK_SOURCE_LOCATION (expr) = stream_input_location (bp, data_in);
> > + stream_input_location (&BLOCK_SOURCE_LOCATION (expr), bp, data_in);
> > }
> >
> > /* Unpack all the non-pointer fields of the TS_TRANSLATION_UNIT_DECL
> > @@ -433,7 +433,7 @@ static void
> > unpack_ts_omp_clause_value_fields (struct data_in *data_in,
> > struct bitpack_d *bp, tree expr)
> > {
> > - OMP_CLAUSE_LOCATION (expr) = stream_input_location (bp, data_in);
> > + stream_input_location (&OMP_CLAUSE_LOCATION (expr), bp, data_in);
> > switch (OMP_CLAUSE_CODE (expr))
> > {
> > case OMP_CLAUSE_DEFAULT:
> > @@ -503,7 +503,7 @@ streamer_read_tree_bitfields (struct lto
> > unpack_ts_fixed_cst_value_fields (&bp, expr);
> >
> > if (CODE_CONTAINS_STRUCT (code, TS_DECL_MINIMAL))
> > - DECL_SOURCE_LOCATION (expr) = stream_input_location (&bp, data_in);
> > + stream_input_location (&DECL_SOURCE_LOCATION (expr), &bp, data_in);
> >
> > if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> > unpack_ts_decl_common_value_fields (&bp, expr);
> > @@ -522,7 +522,7 @@ streamer_read_tree_bitfields (struct lto
> >
> > if (CODE_CONTAINS_STRUCT (code, TS_EXP))
> > {
> > - SET_EXPR_LOCATION (expr, stream_input_location (&bp, data_in));
> > + stream_input_location (&EXPR_CHECK (expr)->exp.locus, &bp, data_in);
> > if (code == MEM_REF
> > || code == TARGET_MEM_REF)
> > {
> > @@ -905,11 +905,20 @@ lto_input_ts_exp_tree_pointers (struct l
> > struct data_in *data_in, tree expr)
> > {
> > int i;
> > + tree block;
> >
> > for (i = 0; i < TREE_OPERAND_LENGTH (expr); i++)
> > TREE_OPERAND (expr, i) = stream_read_tree (ib, data_in);
> >
> > - TREE_SET_BLOCK (expr, stream_read_tree (ib, data_in));
> > + block = stream_read_tree (ib, data_in);
> > +
> > + /* TODO: Block is stored in the locus information. It may make more
> > sense to
> > + to make it go via the location cache. */
> > + if (block)
> > + {
> > + lto_apply_location_cache ();
> > + TREE_SET_BLOCK (expr, block);
> > + }
> > }
> >
> >
> >
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
> Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)
