Re: [PATCH] ipa-sra: Improve debug info for removed parameters (PR 93385)
On Thu, 4 Nov 2021, Martin Jambor wrote: > Hi, > > On Wed, Nov 03 2021, Richard Biener wrote: > > On Mon, 1 Nov 2021, Martin Jambor wrote: > > > >> Hello, > >> > >> I'd like to ping this patch. > >> > >> Thanks, > >> > >> Martin > >> > >> > >> On Wed, Oct 13 2021, Martin Jambor wrote: > >> > Hi, > >> > > >> > in spring I added code eliminating any statements using parameters > >> > removed by IPA passes (to fix PR 93385). That patch fixed issues such > >> > as divisions by zero that such code could perform but it only reset > >> > all affected debug bind statements, this one updates them with > >> > expressions which can allow the debugger to print the removed value - > >> > see the added test-case for an example. > >> > > [...] > >> > > >> > Bootstrapped and tested on x86_64-linux, i686-linux and (long time > >> > ago) on aarch64-linux. Also LTO-bootstrapped and on x86_64-linux. > >> > > >> > Perhaps it is good to go to trunk? > > > > I think the patch is OK for trunk. It would be nice to common the > > Thank you very much, I will commit the patch shortly after a rebase and > a final test. > > > > > + tree vexpr = make_node (DEBUG_EXPR_DECL); > > + DECL_ARTIFICIAL (vexpr) = 1; > > + TREE_TYPE (vexpr) = TREE_TYPE (val); > > + SET_DECL_MODE (vexpr, TYPE_MODE (TREE_TYPE (val))); > > > > blob that exists all over the GCC code base with a new > > build_debug_expr_decl (tree type) (next to tree.c:build_decl). > > > > That makes sense, I'll prepare a patch as a follow-up. > > A minority of places in GCC which produce a DEBUG_DECL_EXPR however set > its mode like cfgexpand.c does: > > tree vexpr = make_node (DEBUG_EXPR_DECL); > DECL_ARTIFICIAL (vexpr) = 1; > TREE_TYPE (vexpr) = TREE_TYPE (value); > if (DECL_P (value)) > mode = DECL_MODE (value); > else > mode = TYPE_MODE (TREE_TYPE (value)); > SET_DECL_MODE (vexpr, mode); I'm not sure why the difference matters - I'd have to dig in history. The only place where this tends to differ is for FIELD_DECLs, but 'value' shouldn't be such one. And looking at the "value" for this looks odd anyhow. That said, I'd simply leave those cases in place (or simply re-set the mode as they do now). > Should build_debug_expr_decl's parameter be perhaps more general, > allowing both DECLs, TYPEs and use its TREE_TYPE if it is neither)? No, I don't like that option much. > Or should I leave such places as they are and only convert those that > set the mode to TYPE_MODE (type)? > > Thanks, > > Martin > > > >> > > >> > gcc/ChangeLog: > >> > > >> > 2021-03-29 Martin Jambor > >> > > >> > PR ipa/93385 > >> > * ipa-param-manipulation.h (class ipa_param_body_adjustments): New > >> > members remap_with_debug_expressions, m_dead_ssa_debug_equiv, > >> > m_dead_stmt_debug_equiv and prepare_debug_expressions. Added > >> > parameter to mark_dead_statements. > >> > * ipa-param-manipulation.c: Include tree-phinodes.h and cfgexpand.h. > >> > (ipa_param_body_adjustments::mark_dead_statements): New parameter > >> > debugstack, push into it all SSA names used in debug statements, > >> > produce m_dead_ssa_debug_equiv mapping for the removed param. > >> > (replace_with_mapped_expr): New function. > >> > (ipa_param_body_adjustments::remap_with_debug_expressions): Likewise. > >> > (ipa_param_body_adjustments::prepare_debug_expressions): Likewise. > >> > (ipa_param_body_adjustments::common_initialization): Gather and > >> > procecc SSA which will be removed but are in debug statements. Simplify. > >> > (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize > >> > new members. > >> > * tree-inline.c (remap_gimple_stmt): Create a debug bind when possible > >> > when avoiding a copy of an unnecessary statement. Remap removed SSA > >> > names in existing debug statements. > >> > (tree_function_versioning): Do not create DEBUG_EXPR_DECL for removed > >> > parameters if we have already done so. > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > 2021-03-29 Martin Jambor > >> > > >> > PR ipa/93385 > >> > * gcc.dg/guality/ipa-sra-1.c: New test. > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
Re: [PATCH] ipa-sra: Improve debug info for removed parameters (PR 93385)
Hi, On Wed, Nov 03 2021, Richard Biener wrote: > On Mon, 1 Nov 2021, Martin Jambor wrote: > >> Hello, >> >> I'd like to ping this patch. >> >> Thanks, >> >> Martin >> >> >> On Wed, Oct 13 2021, Martin Jambor wrote: >> > Hi, >> > >> > in spring I added code eliminating any statements using parameters >> > removed by IPA passes (to fix PR 93385). That patch fixed issues such >> > as divisions by zero that such code could perform but it only reset >> > all affected debug bind statements, this one updates them with >> > expressions which can allow the debugger to print the removed value - >> > see the added test-case for an example. >> > [...] >> > >> > Bootstrapped and tested on x86_64-linux, i686-linux and (long time >> > ago) on aarch64-linux. Also LTO-bootstrapped and on x86_64-linux. >> > >> > Perhaps it is good to go to trunk? > > I think the patch is OK for trunk. It would be nice to common the Thank you very much, I will commit the patch shortly after a rebase and a final test. > > + tree vexpr = make_node (DEBUG_EXPR_DECL); > + DECL_ARTIFICIAL (vexpr) = 1; > + TREE_TYPE (vexpr) = TREE_TYPE (val); > + SET_DECL_MODE (vexpr, TYPE_MODE (TREE_TYPE (val))); > > blob that exists all over the GCC code base with a new > build_debug_expr_decl (tree type) (next to tree.c:build_decl). > That makes sense, I'll prepare a patch as a follow-up. A minority of places in GCC which produce a DEBUG_DECL_EXPR however set its mode like cfgexpand.c does: tree vexpr = make_node (DEBUG_EXPR_DECL); DECL_ARTIFICIAL (vexpr) = 1; TREE_TYPE (vexpr) = TREE_TYPE (value); if (DECL_P (value)) mode = DECL_MODE (value); else mode = TYPE_MODE (TREE_TYPE (value)); SET_DECL_MODE (vexpr, mode); Should build_debug_expr_decl's parameter be perhaps more general, allowing both DECLs, TYPEs and use its TREE_TYPE if it is neither)? Or should I leave such places as they are and only convert those that set the mode to TYPE_MODE (type)? Thanks, Martin >> > >> > gcc/ChangeLog: >> > >> > 2021-03-29 Martin Jambor >> > >> >PR ipa/93385 >> >* ipa-param-manipulation.h (class ipa_param_body_adjustments): New >> >members remap_with_debug_expressions, m_dead_ssa_debug_equiv, >> >m_dead_stmt_debug_equiv and prepare_debug_expressions. Added >> >parameter to mark_dead_statements. >> >* ipa-param-manipulation.c: Include tree-phinodes.h and cfgexpand.h. >> >(ipa_param_body_adjustments::mark_dead_statements): New parameter >> >debugstack, push into it all SSA names used in debug statements, >> >produce m_dead_ssa_debug_equiv mapping for the removed param. >> >(replace_with_mapped_expr): New function. >> >(ipa_param_body_adjustments::remap_with_debug_expressions): Likewise. >> >(ipa_param_body_adjustments::prepare_debug_expressions): Likewise. >> >(ipa_param_body_adjustments::common_initialization): Gather and >> >procecc SSA which will be removed but are in debug statements. Simplify. >> >(ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize >> >new members. >> >* tree-inline.c (remap_gimple_stmt): Create a debug bind when possible >> >when avoiding a copy of an unnecessary statement. Remap removed SSA >> >names in existing debug statements. >> >(tree_function_versioning): Do not create DEBUG_EXPR_DECL for removed >> >parameters if we have already done so. >> > >> > gcc/testsuite/ChangeLog: >> > >> > 2021-03-29 Martin Jambor >> > >> >PR ipa/93385 >> >* gcc.dg/guality/ipa-sra-1.c: New test.
Re: [PATCH] ipa-sra: Improve debug info for removed parameters (PR 93385)
On Mon, 1 Nov 2021, Martin Jambor wrote: > Hello, > > I'd like to ping this patch. > > Thanks, > > Martin > > > On Wed, Oct 13 2021, Martin Jambor wrote: > > Hi, > > > > in spring I added code eliminating any statements using parameters > > removed by IPA passes (to fix PR 93385). That patch fixed issues such > > as divisions by zero that such code could perform but it only reset > > all affected debug bind statements, this one updates them with > > expressions which can allow the debugger to print the removed value - > > see the added test-case for an example. > > > > Even though I originally did not want to create DEBUG_EXPR_DECLs for > > intermediate values, I ended up doing so, because otherwise the code > > started creating statements like > > > ># DEBUG __aD.198693 => &MEM[(const struct _Alloc_nodeD.171110 > > *)D#195]._M_tD.184726->_M_implD.171154 > > > > which not only is a bit scary but also gimple-fold ICEs on > > it. Therefore I decided they are probably quite necessary. > > > > The patch simply notes each removed SSA name present in a debug > > statement and then works from it backwards, looking if it can > > reconstruct the expression it represents (which can fail if a > > non-degenerate PHI node is in the way). If it can, it populates two > > hash maps with those expressions so that 1) removed assignments are > > replaced with a debug bind defining a new intermediate debug_decl_expr > > and 2) existing debug binds that refer to SSA names that are bing > > removed now refer to corresponding debug_decl_exprs. > > > > If a removed parameter is passed to another function, the debugging > > information still cannot describe its value there - see the xfailed > > test in the testcase. I sort of know what needs to be done but that > > needs a little bit more of IPA infrastructure on top of this patch and > > so I would like to get this patch reviewed first. > > > > Bootstrapped and tested on x86_64-linux, i686-linux and (long time > > ago) on aarch64-linux. Also LTO-bootstrapped and on x86_64-linux. > > > > Perhaps it is good to go to trunk? I think the patch is OK for trunk. It would be nice to common the + tree vexpr = make_node (DEBUG_EXPR_DECL); + DECL_ARTIFICIAL (vexpr) = 1; + TREE_TYPE (vexpr) = TREE_TYPE (val); + SET_DECL_MODE (vexpr, TYPE_MODE (TREE_TYPE (val))); blob that exists all over the GCC code base with a new build_debug_expr_decl (tree type) (next to tree.c:build_decl). Thanks, Richard. > > Thanks, > > > > Martin > > > > gcc/ChangeLog: > > > > 2021-03-29 Martin Jambor > > > > PR ipa/93385 > > * ipa-param-manipulation.h (class ipa_param_body_adjustments): New > > members remap_with_debug_expressions, m_dead_ssa_debug_equiv, > > m_dead_stmt_debug_equiv and prepare_debug_expressions. Added > > parameter to mark_dead_statements. > > * ipa-param-manipulation.c: Include tree-phinodes.h and cfgexpand.h. > > (ipa_param_body_adjustments::mark_dead_statements): New parameter > > debugstack, push into it all SSA names used in debug statements, > > produce m_dead_ssa_debug_equiv mapping for the removed param. > > (replace_with_mapped_expr): New function. > > (ipa_param_body_adjustments::remap_with_debug_expressions): Likewise. > > (ipa_param_body_adjustments::prepare_debug_expressions): Likewise. > > (ipa_param_body_adjustments::common_initialization): Gather and > > procecc SSA which will be removed but are in debug statements. Simplify. > > (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize > > new members. > > * tree-inline.c (remap_gimple_stmt): Create a debug bind when possible > > when avoiding a copy of an unnecessary statement. Remap removed SSA > > names in existing debug statements. > > (tree_function_versioning): Do not create DEBUG_EXPR_DECL for removed > > parameters if we have already done so. > > > > gcc/testsuite/ChangeLog: > > > > 2021-03-29 Martin Jambor > > > > PR ipa/93385 > > * gcc.dg/guality/ipa-sra-1.c: New test. > > --- > > gcc/ipa-param-manipulation.c | 280 ++- > > gcc/ipa-param-manipulation.h | 12 +- > > gcc/testsuite/gcc.dg/guality/ipa-sra-1.c | 45 > > gcc/tree-inline.c| 45 ++-- > > 4 files changed, 305 insertions(+), 77 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c > > > > diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c > > index 26b02d7aa95..c84d669521c 100644 > > --- a/gcc/ipa-param-manipulation.c > > +++ b/gcc/ipa-param-manipulation.c > > @@ -43,6 +43,8 @@ along with GCC; see the file COPYING3. If not see > > #include "alloc-pool.h" > > #include "symbol-summary.h" > > #include "symtab-clones.h" > > +#include "tree-phinodes.h" > > +#include "cfgexpand.h" > > > > > > /* Actual prefixes of different newly synthetized parameters. Keep in sync > > @@ -972,10 +974,12 @@
Re: [PATCH] ipa-sra: Improve debug info for removed parameters (PR 93385)
Hello, I'd like to ping this patch. Thanks, Martin On Wed, Oct 13 2021, Martin Jambor wrote: > Hi, > > in spring I added code eliminating any statements using parameters > removed by IPA passes (to fix PR 93385). That patch fixed issues such > as divisions by zero that such code could perform but it only reset > all affected debug bind statements, this one updates them with > expressions which can allow the debugger to print the removed value - > see the added test-case for an example. > > Even though I originally did not want to create DEBUG_EXPR_DECLs for > intermediate values, I ended up doing so, because otherwise the code > started creating statements like > ># DEBUG __aD.198693 => &MEM[(const struct _Alloc_nodeD.171110 > *)D#195]._M_tD.184726->_M_implD.171154 > > which not only is a bit scary but also gimple-fold ICEs on > it. Therefore I decided they are probably quite necessary. > > The patch simply notes each removed SSA name present in a debug > statement and then works from it backwards, looking if it can > reconstruct the expression it represents (which can fail if a > non-degenerate PHI node is in the way). If it can, it populates two > hash maps with those expressions so that 1) removed assignments are > replaced with a debug bind defining a new intermediate debug_decl_expr > and 2) existing debug binds that refer to SSA names that are bing > removed now refer to corresponding debug_decl_exprs. > > If a removed parameter is passed to another function, the debugging > information still cannot describe its value there - see the xfailed > test in the testcase. I sort of know what needs to be done but that > needs a little bit more of IPA infrastructure on top of this patch and > so I would like to get this patch reviewed first. > > Bootstrapped and tested on x86_64-linux, i686-linux and (long time > ago) on aarch64-linux. Also LTO-bootstrapped and on x86_64-linux. > > Perhaps it is good to go to trunk? > > Thanks, > > Martin > > gcc/ChangeLog: > > 2021-03-29 Martin Jambor > > PR ipa/93385 > * ipa-param-manipulation.h (class ipa_param_body_adjustments): New > members remap_with_debug_expressions, m_dead_ssa_debug_equiv, > m_dead_stmt_debug_equiv and prepare_debug_expressions. Added > parameter to mark_dead_statements. > * ipa-param-manipulation.c: Include tree-phinodes.h and cfgexpand.h. > (ipa_param_body_adjustments::mark_dead_statements): New parameter > debugstack, push into it all SSA names used in debug statements, > produce m_dead_ssa_debug_equiv mapping for the removed param. > (replace_with_mapped_expr): New function. > (ipa_param_body_adjustments::remap_with_debug_expressions): Likewise. > (ipa_param_body_adjustments::prepare_debug_expressions): Likewise. > (ipa_param_body_adjustments::common_initialization): Gather and > procecc SSA which will be removed but are in debug statements. Simplify. > (ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize > new members. > * tree-inline.c (remap_gimple_stmt): Create a debug bind when possible > when avoiding a copy of an unnecessary statement. Remap removed SSA > names in existing debug statements. > (tree_function_versioning): Do not create DEBUG_EXPR_DECL for removed > parameters if we have already done so. > > gcc/testsuite/ChangeLog: > > 2021-03-29 Martin Jambor > > PR ipa/93385 > * gcc.dg/guality/ipa-sra-1.c: New test. > --- > gcc/ipa-param-manipulation.c | 280 ++- > gcc/ipa-param-manipulation.h | 12 +- > gcc/testsuite/gcc.dg/guality/ipa-sra-1.c | 45 > gcc/tree-inline.c| 45 ++-- > 4 files changed, 305 insertions(+), 77 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c > > diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c > index 26b02d7aa95..c84d669521c 100644 > --- a/gcc/ipa-param-manipulation.c > +++ b/gcc/ipa-param-manipulation.c > @@ -43,6 +43,8 @@ along with GCC; see the file COPYING3. If not see > #include "alloc-pool.h" > #include "symbol-summary.h" > #include "symtab-clones.h" > +#include "tree-phinodes.h" > +#include "cfgexpand.h" > > > /* Actual prefixes of different newly synthetized parameters. Keep in sync > @@ -972,10 +974,12 @@ ipa_param_body_adjustments::carry_over_param (tree t) > > /* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without > any replacement or splitting. REPL is the replacement VAR_SECL to base > any > - remaining uses of a removed parameter on. */ > + remaining uses of a removed parameter on. Push all removed SSA names that > + are used within debug statements to DEBUGSTACK. */ > > void > -ipa_param_body_adjustments::mark_dead_statements (tree dead_param) > +ipa_param_body_adjustments::mark_dead_statements (tree dead_param, > +
[PATCH] ipa-sra: Improve debug info for removed parameters (PR 93385)
Hi,
in spring I added code eliminating any statements using parameters
removed by IPA passes (to fix PR 93385). That patch fixed issues such
as divisions by zero that such code could perform but it only reset
all affected debug bind statements, this one updates them with
expressions which can allow the debugger to print the removed value -
see the added test-case for an example.
Even though I originally did not want to create DEBUG_EXPR_DECLs for
intermediate values, I ended up doing so, because otherwise the code
started creating statements like
# DEBUG __aD.198693 => &MEM[(const struct _Alloc_nodeD.171110
*)D#195]._M_tD.184726->_M_implD.171154
which not only is a bit scary but also gimple-fold ICEs on
it. Therefore I decided they are probably quite necessary.
The patch simply notes each removed SSA name present in a debug
statement and then works from it backwards, looking if it can
reconstruct the expression it represents (which can fail if a
non-degenerate PHI node is in the way). If it can, it populates two
hash maps with those expressions so that 1) removed assignments are
replaced with a debug bind defining a new intermediate debug_decl_expr
and 2) existing debug binds that refer to SSA names that are bing
removed now refer to corresponding debug_decl_exprs.
If a removed parameter is passed to another function, the debugging
information still cannot describe its value there - see the xfailed
test in the testcase. I sort of know what needs to be done but that
needs a little bit more of IPA infrastructure on top of this patch and
so I would like to get this patch reviewed first.
Bootstrapped and tested on x86_64-linux, i686-linux and (long time
ago) on aarch64-linux. Also LTO-bootstrapped and on x86_64-linux.
Perhaps it is good to go to trunk?
Thanks,
Martin
gcc/ChangeLog:
2021-03-29 Martin Jambor
PR ipa/93385
* ipa-param-manipulation.h (class ipa_param_body_adjustments): New
members remap_with_debug_expressions, m_dead_ssa_debug_equiv,
m_dead_stmt_debug_equiv and prepare_debug_expressions. Added
parameter to mark_dead_statements.
* ipa-param-manipulation.c: Include tree-phinodes.h and cfgexpand.h.
(ipa_param_body_adjustments::mark_dead_statements): New parameter
debugstack, push into it all SSA names used in debug statements,
produce m_dead_ssa_debug_equiv mapping for the removed param.
(replace_with_mapped_expr): New function.
(ipa_param_body_adjustments::remap_with_debug_expressions): Likewise.
(ipa_param_body_adjustments::prepare_debug_expressions): Likewise.
(ipa_param_body_adjustments::common_initialization): Gather and
procecc SSA which will be removed but are in debug statements. Simplify.
(ipa_param_body_adjustments::ipa_param_body_adjustments): Initialize
new members.
* tree-inline.c (remap_gimple_stmt): Create a debug bind when possible
when avoiding a copy of an unnecessary statement. Remap removed SSA
names in existing debug statements.
(tree_function_versioning): Do not create DEBUG_EXPR_DECL for removed
parameters if we have already done so.
gcc/testsuite/ChangeLog:
2021-03-29 Martin Jambor
PR ipa/93385
* gcc.dg/guality/ipa-sra-1.c: New test.
---
gcc/ipa-param-manipulation.c | 280 ++-
gcc/ipa-param-manipulation.h | 12 +-
gcc/testsuite/gcc.dg/guality/ipa-sra-1.c | 45
gcc/tree-inline.c| 45 ++--
4 files changed, 305 insertions(+), 77 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/guality/ipa-sra-1.c
diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 26b02d7aa95..c84d669521c 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -43,6 +43,8 @@ along with GCC; see the file COPYING3. If not see
#include "alloc-pool.h"
#include "symbol-summary.h"
#include "symtab-clones.h"
+#include "tree-phinodes.h"
+#include "cfgexpand.h"
/* Actual prefixes of different newly synthetized parameters. Keep in sync
@@ -972,10 +974,12 @@ ipa_param_body_adjustments::carry_over_param (tree t)
/* Populate m_dead_stmts given that DEAD_PARAM is going to be removed without
any replacement or splitting. REPL is the replacement VAR_SECL to base any
- remaining uses of a removed parameter on. */
+ remaining uses of a removed parameter on. Push all removed SSA names that
+ are used within debug statements to DEBUGSTACK. */
void
-ipa_param_body_adjustments::mark_dead_statements (tree dead_param)
+ipa_param_body_adjustments::mark_dead_statements (tree dead_param,
+ vec *debugstack)
{
/* Current IPA analyses which remove unused parameters never remove a
non-gimple register ones which have any use except as parameters in other
@@ -987,6 +991,7 @@ ipa_param_body_adjustments::mark_dead_stat
