Re: [PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]
On Thu, Nov 3, 2022 at 9:07 PM Lewis Hyatt 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
Re: [PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]
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! -Lewis [PATCH] middle-end: Preserve frontend diagnostics in free-lang-data [PR101551, PR106274] gcc/ChangeLog: PR lto/106274 PR middle-end/101551 * ipa-free-lang-data.cc (free_lang_data_in_decl): Preserve DECL_CONTEXT for class member functions. (free_lang_data): Do not reset frontend diagnostics customizations. gcc/testsuite/ChangeLog: PR lto/106274 PR middle-end/101551 * c-c++-common/diag-after-fld-1.c: New test. * g++.dg/diag-after-fld-1.C: New test. * g++.dg/diag-after-fld-2.C: New test. * gfortran.dg/allocatable_uninitialized_2.f90: New test. * objc.dg/diag-after-fld-1.m: New test. diff --git a/gcc/ipa-free-lang-data.cc b/gcc/ipa-free-lang-data.cc index ccdbf849c25..391b7689639 100644 --- a/gcc/ipa-free-lang-data.cc +++ b/gcc/ipa-free-lang-data.cc @@ -682,10 +682,8 @@ free_lang_data_in_decl (tree decl, class free_lang_data_d *fld) 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) + && TREE_CODE (decl) != VAR_DECL + && TREE_CODE (decl) != FUNCTION_DECL) DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); } @@ -1115,7 +1113,6 @@ free_lang_data (void) /* Reset some langhooks. Do not reset types_compatible_p, it may still be used indirectly via the get_alias_set langhook. */ lang_hooks.dwarf_name = lhd_dwarf_name; - lang_hooks.decl_printable_name = gimple_decl_printable_name; lang_hooks.gimplify_expr = lhd_gimplify_expr; lang_hooks.overwrite_decl_assembler_name = lhd_overwrite_decl_assembler_name; lang_hooks.print_xnode = lhd_print_tree_nothing; @@ -1141,9 +1138,6 @@ free_lang_data (void) make sure we never call decl_assembler_name on local symbols and devise a
Re: [PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]
On Tue, Oct 25, 2022 at 11:05 PM Lewis Hyatt wrote: > > On Tue, Oct 25, 2022 at 7:35 AM Richard Biener > wrote: > > > > On Thu, Oct 20, 2022 at 1:09 AM Lewis Hyatt via Gcc-patches > > wrote: > > > > > > Currently, the ipa-free-lang-data pass resets most of the frontend's > > > diagnostic customizations, such as the diagnostic_finalizer that prints > > > macro > > > expansion information, which is the subject of the two PRs. In most cases, > > > however, there is no need to reset these customizations; they still work > > > just > > > fine after the language-specific data has been freed. (Macro tracking > > > information, for instance, only depends on the line_maps instance and > > > does not > > > use the tree data structures at all.) > > > > > > Add an interface whereby frontends can convey which of their > > > customizations > > > should be preserved by ipa-free-lang-data. Only the macro tracking > > > behavior is > > > changed for now. Subsequent patches will add further configurations for > > > each > > > frontend. > > > > One point of the resetting of the hooks is to avoid crashes due to us > > zapping > > many of the lang specific data structures. If the hooks were more resilent > > that wouldn't be an issue. > > > > Right. The patch I have for C++ (not sent yet) makes the C++ versions > of decl_printable_name and and the diagnostic starter able to work > after free_lang_data runs. I just worry that future changes to the > C++ hooks would need to preserve this property, which could be error > prone since issues are not immediately apparent, and most of the > testsuite does not use -flto. > > > Now - as for macro tracking, how difficult is it to replicate that in the > > default hook implementation? Basically we have similar issues for > > late diagnostics of the LTO compile step where only the LTO (aka default) > > variant of the hooks are present - it would be nice to improve that as well. > > > > It is easy enough to make the default diagnostic finalizer print the > macro tracking information stored in the global line_table. (It just > needs to check if the global line_table is set, in which case call > virt_loc_aware_diagnostic_finalizer()). This would remove the need for > C-family frontends to override that callback. Fortran would still do > so, since it does other things in its finalizer. However, this would > not help with the LTO frontend because the line_table is not part of > what gets streamed out. Rather the line_table is rebuilt from scratch > when reading the data back in, but the macro tracking information is > not available at that time, just the basic location info (filename and > source location). I am not that familiar with the LTO streaming > process but I feel like streaming the entire line_table would not mesh > well with it (especially since multiple of them from different > translation units would need to be combined back together). > > > Note free_lang_data exists to "simplify" the LTO bytecode output - things > > freed do not need to be output. Of course the "freeing" logic could be > > wired into the LTO bytecode output machinery directly - simply do not > > output what we'd free. That way all info would prevail for the non-LTO > > compile and the hooks could continue to work as they do without any > > LTO streaming done. > > > > Naively (emphasis on the naive, as I don't have any experience with > this part of GCC), that is how I would have guessed it worked. But I > understood there are some benefits to freeing the lang data earlier > (e.g. reduced resource usage), and even a hope to start doing it in > non-LTO builds as well, so I thought some incremental changes as in > this patch to make diagnostics better after free_lang_data could > perhaps be useful. Thanks for taking a look at it! 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. > > -Lewis
Re: [PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]
On Tue, Oct 25, 2022 at 7:35 AM Richard Biener wrote: > > On Thu, Oct 20, 2022 at 1:09 AM Lewis Hyatt via Gcc-patches > wrote: > > > > Currently, the ipa-free-lang-data pass resets most of the frontend's > > diagnostic customizations, such as the diagnostic_finalizer that prints > > macro > > expansion information, which is the subject of the two PRs. In most cases, > > however, there is no need to reset these customizations; they still work > > just > > fine after the language-specific data has been freed. (Macro tracking > > information, for instance, only depends on the line_maps instance and does > > not > > use the tree data structures at all.) > > > > Add an interface whereby frontends can convey which of their customizations > > should be preserved by ipa-free-lang-data. Only the macro tracking behavior > > is > > changed for now. Subsequent patches will add further configurations for > > each > > frontend. > > One point of the resetting of the hooks is to avoid crashes due to us zapping > many of the lang specific data structures. If the hooks were more resilent > that wouldn't be an issue. > Right. The patch I have for C++ (not sent yet) makes the C++ versions of decl_printable_name and and the diagnostic starter able to work after free_lang_data runs. I just worry that future changes to the C++ hooks would need to preserve this property, which could be error prone since issues are not immediately apparent, and most of the testsuite does not use -flto. > Now - as for macro tracking, how difficult is it to replicate that in the > default hook implementation? Basically we have similar issues for > late diagnostics of the LTO compile step where only the LTO (aka default) > variant of the hooks are present - it would be nice to improve that as well. > It is easy enough to make the default diagnostic finalizer print the macro tracking information stored in the global line_table. (It just needs to check if the global line_table is set, in which case call virt_loc_aware_diagnostic_finalizer()). This would remove the need for C-family frontends to override that callback. Fortran would still do so, since it does other things in its finalizer. However, this would not help with the LTO frontend because the line_table is not part of what gets streamed out. Rather the line_table is rebuilt from scratch when reading the data back in, but the macro tracking information is not available at that time, just the basic location info (filename and source location). I am not that familiar with the LTO streaming process but I feel like streaming the entire line_table would not mesh well with it (especially since multiple of them from different translation units would need to be combined back together). > Note free_lang_data exists to "simplify" the LTO bytecode output - things > freed do not need to be output. Of course the "freeing" logic could be > wired into the LTO bytecode output machinery directly - simply do not > output what we'd free. That way all info would prevail for the non-LTO > compile and the hooks could continue to work as they do without any > LTO streaming done. > Naively (emphasis on the naive, as I don't have any experience with this part of GCC), that is how I would have guessed it worked. But I understood there are some benefits to freeing the lang data earlier (e.g. reduced resource usage), and even a hope to start doing it in non-LTO builds as well, so I thought some incremental changes as in this patch to make diagnostics better after free_lang_data could perhaps be useful. Thanks for taking a look at it! -Lewis
Re: [PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]
On Thu, Oct 20, 2022 at 1:09 AM Lewis Hyatt via Gcc-patches wrote: > > Currently, the ipa-free-lang-data pass resets most of the frontend's > diagnostic customizations, such as the diagnostic_finalizer that prints macro > expansion information, which is the subject of the two PRs. In most cases, > however, there is no need to reset these customizations; they still work just > fine after the language-specific data has been freed. (Macro tracking > information, for instance, only depends on the line_maps instance and does not > use the tree data structures at all.) > > Add an interface whereby frontends can convey which of their customizations > should be preserved by ipa-free-lang-data. Only the macro tracking behavior is > changed for now. Subsequent patches will add further configurations for each > frontend. One point of the resetting of the hooks is to avoid crashes due to us zapping many of the lang specific data structures. If the hooks were more resilent that wouldn't be an issue. Now - as for macro tracking, how difficult is it to replicate that in the default hook implementation? Basically we have similar issues for late diagnostics of the LTO compile step where only the LTO (aka default) variant of the hooks are present - it would be nice to improve that as well. Note free_lang_data exists to "simplify" the LTO bytecode output - things freed do not need to be output. Of course the "freeing" logic could be wired into the LTO bytecode output machinery directly - simply do not output what we'd free. That way all info would prevail for the non-LTO compile and the hooks could continue to work as they do without any LTO streaming done. Richard. > > gcc/ChangeLog: > > PR lto/106274 > PR middle-end/101551 > * diagnostic.h (struct diagnostic_context): Add new customization > point diagnostic_context::preserve_on_reset. > * diagnostic.cc (diagnostic_initialize): Initialize it. > * tree-diagnostic.h (tree_diagnostics_defaults): Add new optional > argument enable_preserve. > * tree-diagnostic.cc (tree_diagnostics_defaults): Implement it. > * ipa-free-lang-data.cc (free_lang_data): Use it. > > gcc/c-family/ChangeLog: > > PR lto/106274 > PR middle-end/101551 > * c-opts.cc (c_common_diagnostics_set_defaults): Preserve > diagnostic finalizer for the middle end. > > libgomp/ChangeLog: > > PR lto/106274 > PR middle-end/101551 > * testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Remove > now-unnecessary workaround. > * testsuite/libgomp.oacc-c-c++-common/vred2d-128.c: Likewise. > > gcc/testsuite/ChangeLog: > > PR lto/106274 > PR middle-end/101551 > * c-c++-common/diag-after-fld-1.c: New test. > --- > > Notes: > Hello- > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101551 > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106274 > > PR101551 complains that when compiling with offloading enabled, > diagnostics > output changes in several different ways. PR106274 notes the same thing > (which > has the same cause) when compiling with -flto, for the specific case of > macro > expansion tracking, which is no longer output for middle-end diagnostics > when > -flto is present. Restoring the macro tracking information, at least, can > be > done simply, which is the attached patch. This is straightforward because > the > printing of macro tracking information is not really reliant on the sort > of > language-specific data structures that are freed by ipa-free-lang-data... > it > just needs the line-maps API, which is common to all languages and which > is > not impacted by the work done by ipa-free-lang-data. > > To be clear, this is not about diagnostics issued *by* the lto frontend. > This > is just about diagnostics issued by the language frontends, in case they > were > asked to stream the IL for later use by the lto frontend. I think from the > user's perspective, compiling with or without -flto should not change the > quality of diagnostics, since on the face of it, -flto seems to be just a > request for the compiler to do something extra (write out the IL in > addition > to all the other stuff it does), and it's not clear why this should > change the > way diagnostics look. (I understand there must be some good reasons, why > it's > not the case.) So anyway I was focusing on how to keep the diagnostics as > close as possible to their normal form after ipa-free-lang-data is done. > > The approach I took was to add a new variable "preserve_on_reset" in the > diagnostic_context, allowing a frontend to specify whether its diagnostic > context customizations are safe to leave in place for the middle end. > There > are currently four customizations for which preservation can be enabled: > > * diagnostic starter >
[PATCH] diagnostics: Allow FEs to keep customizations for middle end [PR101551, PR106274]
Currently, the ipa-free-lang-data pass resets most of the frontend's
diagnostic customizations, such as the diagnostic_finalizer that prints macro
expansion information, which is the subject of the two PRs. In most cases,
however, there is no need to reset these customizations; they still work just
fine after the language-specific data has been freed. (Macro tracking
information, for instance, only depends on the line_maps instance and does not
use the tree data structures at all.)
Add an interface whereby frontends can convey which of their customizations
should be preserved by ipa-free-lang-data. Only the macro tracking behavior is
changed for now. Subsequent patches will add further configurations for each
frontend.
gcc/ChangeLog:
PR lto/106274
PR middle-end/101551
* diagnostic.h (struct diagnostic_context): Add new customization
point diagnostic_context::preserve_on_reset.
* diagnostic.cc (diagnostic_initialize): Initialize it.
* tree-diagnostic.h (tree_diagnostics_defaults): Add new optional
argument enable_preserve.
* tree-diagnostic.cc (tree_diagnostics_defaults): Implement it.
* ipa-free-lang-data.cc (free_lang_data): Use it.
gcc/c-family/ChangeLog:
PR lto/106274
PR middle-end/101551
* c-opts.cc (c_common_diagnostics_set_defaults): Preserve
diagnostic finalizer for the middle end.
libgomp/ChangeLog:
PR lto/106274
PR middle-end/101551
* testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Remove
now-unnecessary workaround.
* testsuite/libgomp.oacc-c-c++-common/vred2d-128.c: Likewise.
gcc/testsuite/ChangeLog:
PR lto/106274
PR middle-end/101551
* c-c++-common/diag-after-fld-1.c: New test.
---
Notes:
Hello-
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101551
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106274
PR101551 complains that when compiling with offloading enabled, diagnostics
output changes in several different ways. PR106274 notes the same thing
(which
has the same cause) when compiling with -flto, for the specific case of
macro
expansion tracking, which is no longer output for middle-end diagnostics
when
-flto is present. Restoring the macro tracking information, at least, can be
done simply, which is the attached patch. This is straightforward because
the
printing of macro tracking information is not really reliant on the sort of
language-specific data structures that are freed by ipa-free-lang-data... it
just needs the line-maps API, which is common to all languages and which is
not impacted by the work done by ipa-free-lang-data.
To be clear, this is not about diagnostics issued *by* the lto frontend.
This
is just about diagnostics issued by the language frontends, in case they
were
asked to stream the IL for later use by the lto frontend. I think from the
user's perspective, compiling with or without -flto should not change the
quality of diagnostics, since on the face of it, -flto seems to be just a
request for the compiler to do something extra (write out the IL in addition
to all the other stuff it does), and it's not clear why this should change
the
way diagnostics look. (I understand there must be some good reasons, why
it's
not the case.) So anyway I was focusing on how to keep the diagnostics as
close as possible to their normal form after ipa-free-lang-data is done.
The approach I took was to add a new variable "preserve_on_reset" in the
diagnostic_context, allowing a frontend to specify whether its diagnostic
context customizations are safe to leave in place for the middle end. There
are currently four customizations for which preservation can be enabled:
* diagnostic starter
* diagnostic finalizer
* format decoder
* decl_printable_name langhook
Preserving the diagnostic finalizer is sufficient to restore the output of
macro tracking information, and that's what I did in this patch. But it
seems
that it's possible to go a bit farther than this as well. Here are some
examples:
1. Fortran:
Consider the existing testcase
gfortran.dg/allocatable_uninitialized_1.f90. When compiled without
-flto,
it outputs:
===
allocatable_uninitialized_1.f90:6:47:
6 |a(1)=2*b(1) ! { dg-warning "uninitialized" }
| ^
Warning: 'b.offset' is used uninitialized [-Wuninitialized]
allocatable_uninitialized_1.f90:4:30:
4 | real,allocatable:: a(:),b(:)
| ^
note: 'b' declared here
allocatable_uninitialized_1.f90:6:47:
6 |a(1)=2*b(1) ! { dg-warning "uninitialized" }
