On Wed, 9 Dec 2020, Bernd Edlinger wrote:
> On 12/8/20 7:57 PM, Bernd Edlinger wrote:
> > On 12/8/20 11:35 AM, Richard Biener wrote:
> >>
> >> + {
> >> + /* Remove a nonbind marker when the outer scope of the
> >> + inline function is completely removed. */
> >> + if (gimple_debug_nonbind_marker_p (stmt)
> >> + && BLOCK_ABSTRACT_ORIGIN (b))
> >> + {
> >> + while (TREE_CODE (b) == BLOCK
> >> + && !inlined_function_outer_scope_p (b))
> >> + b = BLOCK_SUPERCONTEXT (b);
> >>
> >> So given we never remove a inlined_function_outer_scope_p BLOCK from
> >> the block tree can we assert that we find such a BLOCK? If we never
> >> elide those BLOCKs how can it happen that we elide it in the end?
>
> We can remove inlined function outer scope when they have no subblocks
> any more, or only unused subblocks, and there is an exception from the
> rule when no debug info is generated, that is due to this:
>
> > else if (!flag_auto_profile && debug_info_level == DINFO_LEVEL_NONE
> > && !optinfo_wants_inlining_info_p ())
> > {
> > /* Even for -g0 don't prune outer scopes from artificial
> > functions, otherwise diagnostics using tree_nonartificial_location
> > will not be emitted properly. */
> > if (inlined_function_outer_scope_p (scope))
> > {
> > tree ao = BLOCK_ORIGIN (scope);
> > if (ao
> > && TREE_CODE (ao) == FUNCTION_DECL
> > && DECL_DECLARED_INLINE_P (ao)
> > && lookup_attribute ("artificial", DECL_ATTRIBUTES (ao)))
> > unused = false;
> > }
> > }
> >
>
> I instrumented the remove_unused_scope_block_p now as follows,
> to better understand what happens here:
>
> diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c
> index 9ea24a1..3dd859c 100644
> --- a/gcc/tree-ssa-live.c
> +++ b/gcc/tree-ssa-live.c
> @@ -525,9 +525,15 @@ remove_unused_scope_block_p (tree scope, bool
> in_ctor_dtor_block)
> *t = BLOCK_SUBBLOCKS (*t);
> while (BLOCK_CHAIN (*t))
> {
> + gcc_assert (TREE_USED (*t));
> + if (debug_info_level != DINFO_LEVEL_NONE)
> + gcc_assert (!inlined_function_outer_scope_p
> (BLOCK_SUPERCONTEXT (*t)));
> BLOCK_SUPERCONTEXT (*t) = supercontext;
> t = &BLOCK_CHAIN (*t);
> }
> + gcc_assert (TREE_USED (*t));
> + if (debug_info_level != DINFO_LEVEL_NONE)
> + gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT
> (*t)));
> BLOCK_CHAIN (*t) = next;
> BLOCK_SUPERCONTEXT (*t) = supercontext;
> t = &BLOCK_CHAIN (*t);
>
> This survives a bootstrap, but I consider that just as an experiment...
>
> This means that the BLOCK_SUPERCONTEXT pointers never skip
> an inlined_function_outer_scope_p, *except* when no debug info is
> generated, but then it is fine, as there are either no debug_nonbind_marker_p,
> or it would not matter, if an outer scope is missed.
>
> After the above loop runs, the BLOCK_SUBBLOCKS->BLOCK_CHAIN have only
> Blocks with TREE_USED
> Blocks with !TREE_USED are removed from the SUBBLOCKS->CHAIN list, but
> have still a valid BLOCK_SUPERCONTEXT. However BLOCK_CHAIN and BLOCK_SUBBLOCKS
> are not used any more, and could theoretically misused for something, but
> fortunately that is not necessary.
>
> I think that result suggests that the proposed patch does the right thing,
> already as-is.
>
>
> Do you agree?
Yes, I think that in the end with all the constraints under which we
elide blocks the patch does the correct thing. What makes me uneasy
is how un-obvious that is ;) That said, the comment should probably
say
Elide debug marker stmts that have an associated BLOCK from an
inline instance removed with also the outermost scope BLOCK of
said inline instance removed. If the outermost scope BLOCK of
said inline instance is preserved use that in place of the removed
BLOCK. That keeps the marker associated to the correct inline
instance (or no inline instance in case it was not from an inline
instance).
That at least makes the intent clear while the implementation
might still not be 100% obviously correct. That's why I suggested
to track "inline instance outermost scope BLOCK of BLOCK" in the
function that messes with the BLOCK tree.
But I guess the comment is enough to reverse-engineer that.
So OK with a comment along this line (if I got it correct, that is ;))
Thanks,
Richard.
