On Mon, May 10, 2021 at 8:43 PM Martin Jambor <mjam...@suse.cz> wrote: > > On Mon, May 10 2021, Richard Biener wrote: > > On Tue, Apr 27, 2021 at 5:26 PM Martin Jambor <mjam...@suse.cz> wrote: > >> > >> Hi, > >> > >> Whereas the previous patch fixed issues with code left behind after > >> IPA-SRA removed a parameter but 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. > >> > >> 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 and have > >> them. > >> > >> 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. > > > > Isn't this what insert_debug_temp_for_var_def already does when you > > remove a stmt and if you take care to do that back-to-front? So with > > IPA SRA removing a parameter you'd "only" need to make sure to > > set up a debug stmt for the parameter itself and that be picked up > > for the (uninitialized) default-def you map to? > > > > But there is no removal, the dead statements creating dead SSAs are not > even copied when tree-inline.c does its thing, such SSAs are actually > mapped to error_mark_node.
OK. Still its a bit odd what you do - I'd have expected that we can somehow create the debug stmt from the original stmt and then "copy"/remap that instead of the original stmt (which we DCE). It looks like you didn't yet install the DCE patch so I'd have to dig it up to remember how the DCE is wired in, but basically I'd have expected remap_gimple_stmt for a DCEd stmt to go the same way if it were a debug-bind but of course instead of copying a debug bind, create one from scratch, pushing it to id->debug_stmts so it gets re-mapped later. Removed SSA defs have to be set up as mapping to a new debug temp, so we do have to scan the original DCEd stmt for defs. > The code is heavily inspired by what removal does but (IIRC I hope) it > is also much simpler because IPA-SRA can only remove limited classes of > scalars. Maybe some code can be split out and shared - I realize that insert_debug_temp_for_var_def does stuff that's not appropriate in the inlining context. Or just add another parameter so we can fend off that code ... Sorry for the delay, Richard. > > Martin > > > > >> 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 the > >> handling of debug information for removed parameters is LTO unfriendly > >> in general and so needs a bit more work. > >> > >> Bootstrapped and tested on x86_64-linux, i686-linux and aarch64-linux. > >> Also LTO-bootstrapped and LTO-profiledbootstrapped on x86_64-linux. > >> > >> OK for trunk? > >> > >> Thanks, > >> > >> Martin > >> > >> > >> gcc/ChangeLog: > >> > >> 2021-03-29 Martin Jambor <mjam...@suse.cz> > >> > >> 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 <mjam...@suse.cz> > >> > >> PR ipa/93385 > >> * gcc.dg/guality/ipa-sra-1.c: New test.
