On Thu, 3 Sep 2020, Jakub Jelinek wrote:
> On Thu, Sep 03, 2020 at 12:07:36PM +0200, Richard Biener wrote:
> > LGTM.
>
> Thanks.
>
> > Can we now remove stream_input_location_now?
>
> There are a few remaining users, one is:
> case LTO_ert_must_not_throw:
> {
> r->type = ERT_MUST_NOT_THROW;
> 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_now (&bp, data_in);
> }
> and the other two:
> /* Input the function start and end loci. */
> fn->function_start_locus = stream_input_location_now (&bp, data_in);
> fn->function_end_locus = stream_input_location_now (&bp, data_in);
> I know nothing about those, so have kept them as is.
I think it's safe to share locations with the rest of the IL and the EH
tree so just replacing them should be fine.
> I don't know if
> failure_loc can or can't include blocks, what is the reason why it uses
> *_now (can r be reallocated?) and similarly about the function start/end.
> Will defer these to Honza or anybody else who knows LTO better.
>
> Ok, looking some more, seems r isn't really reallocated and failure_loc
> shouldn't really include blocks, judging from
> case ERT_MUST_NOT_THROW:
> new_r->u.must_not_throw.failure_loc =
> LOCATION_LOCUS (old_r->u.must_not_throw.failure_loc);
> and
> this_region->u.must_not_throw.failure_loc
> = LOCATION_LOCUS (gimple_location (tp));
> as the only setters of failure_loc outside of lto-streamer-in.c.
> Dunno if it would be sufficient to just use normal stream_input_location
> and let the apply cache after all EH regions and later all bbs are input
> apply it.
I think so.
> > > all these != checks to non-zero? Either by oring something into those
> > > tests, or perhaps:
> > > if (ob->current_reset)
> > > {
> > > if (xloc.file == NULL)
> > > ob->current_file = "";
> > > if (xloc.line == 0)
> > > ob->current_line = 1;
> > > if (xloc.column == 0)
> > > ob->current_column = 1;
> > > ob->current_reset = false;
> > > }
> > > before doing those bp_pack_value calls with a comment, effectively forcing
> > > all 6 != comparisons to be true?
> >
> > Yeah, guess that would be cleanest. How does UNKNOWN_LOCATION
> > expand btw? Using that as "reset" value also might work?
>
> On the lto-streamer-out.c, for UNKNOWN_LOCATION/BUILTINS_LOCATION we just
> do:
> bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT,
> loc < RESERVED_LOCATION_COUNT
> ? loc : RESERVED_LOCATION_COUNT);
> and that is it (well, for locs with blocks I do more now, but
> ob->current_{file,line,column,sysp} aren't touched).
>
> I'll test a patch.
Thanks,
Richard.
