Re: [PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]

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

On Thu, Nov 3, 2022 at 9:07 PM Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> On Fri, Oct 28, 2022 at 10:28:21AM +0200, Richard Biener wrote:
> > Yes, the idea was also to free up memory but then that part never
> > really materialized - the idea was to always run free-lang-data, not
> > just when later outputting LTO bytecode.  The reason is probably
> > mainly the diagnostic regressions you observe.
> >
> > Maybe a better strathegy than your patch would be to work towards
> > that goal but reduce the number of "freeings", instead adjusting the
> > LTO streamer to properly ignore frontend specific bits where clearing
> > conflicts with the intent to preserve accurate diagnostics throughout
> > the compilation.
> >
> > If you see bits that when not freed would fix some of the observed
> > issues we can see to replicate the freeing in the LTO output machinery.
> >
> > Richard.
>
> Thanks again for the suggestions. I took a look and it seems pretty doable to
> just stop resetting all the diagnostics hooks in free-lang-data. Once that's
> done, the only problematic part that I have been able to identify is here in
> ipa-free-lang-data.c around line 674:
>
> ====
>   /* We need to keep field decls associated with their trees. Otherwise tree
>      merging may merge some fields and keep others disjoint which in turn will
>      not do well with TREE_CHAIN pointers linking them.
>
>      Also do not drop containing types for virtual methods and tables because
>      these are needed by devirtualization.
>      C++ destructors are special because C++ frontends sometimes produces
>      virtual destructor as an alias of non-virtual destructor.  In
>      devirutalization code we always walk through aliases and we need
>      context to be preserved too.  See PR89335  */
>   if (TREE_CODE (decl) != FIELD_DECL
>       && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
>           || (!DECL_VIRTUAL_P (decl)
>               && (TREE_CODE (decl) != FUNCTION_DECL
>                   || !DECL_CXX_DESTRUCTOR_P (decl)))))
>     DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl));
> ====
>
> The C++ implementations of the decl_printable_name langhook and the diagnostic
> starter callback do not work as-is when the DECL_CONTEXT for class member
> functions disappears.  So I did have a patch that changes the C++
> implementations to work in this case, but attached here is a new one along the
> lines of what you suggested, rather changing the above part of free-lang-data
> so it doesn't activate as often. The patch is pretty complete (other than
> missing a commit message) and bootstrap + regtest all languages looks good
> with no regressions. I tried the same with BUILD_CONFIG=bootstrap-lto as well,
> and that also looked good when it eventually finished. I added testcases for
> several frontends to verify that the diagnostics still work with -flto. I am
> not sure what are the implications for LTO itself, of changing this part of
> the pass, so I would have to ask you to weigh in on that aspect please. Thanks!

First of all sorry for the delay and thanks for trying.  The effect on LTO is an
increase in the amount of streamed IL since we follow the DECL_CONTEXT
edge when streaming the tree graph.  So my solution for this would be to
reflect the case you remove in free-lang-data in both
lto-streamer-out.cc:DFS::DFS_write_tree_body where we do

      if (TREE_CODE (expr) != TRANSLATION_UNIT_DECL
          && ! DECL_CONTEXT (expr))
        DFS_follow_tree_edge ((*all_translation_units)[0]);
      else
        DFS_follow_tree_edge (DECL_CONTEXT (expr));

and in tree-streamer-out.cc:write_ts_decl_minimal_tree_pointers which
does

  if (TREE_CODE (expr) != TRANSLATION_UNIT_DECL
      && ! DECL_CONTEXT (expr))
    stream_write_tree_ref (ob, (*all_translation_units)[0]);
  else
    stream_write_tree_ref (ob, DECL_CONTEXT (expr));

that possibly boils down to "just" doing

   tree ctx = DECL_CONTEXT (..);
   if (TREE_CODE (..) == VAR_DECL || TREE_CODE (..) == FUNCTION_DECL)
     ctx = fld_decl_context (ctx);

and using 'ctx' for DECL_CONTEXT in those two places (and exporting the
fld_decl_context function).

As said the idea for this is that we want to avoid streaming type trees when
not necessary.  When doing an LTO bootstrap with your patch you should
see (slightly) larger object files.

Richard.

>
> -Lewis