On Thu, 4 Apr 2019, Alexandre Oliva wrote:
> On Apr 3, 2019, Richard Biener <rguent...@suse.de> wrote:
>
> > My solution is to instead of dropping the debug binds still
> > move them to the destination but then reset them. This solves
> > the wrong-debug issue.
>
> *nod*, yeah, that's probably the best we can do without major surgery.
>
> > But it of course possibly degrades debug information for the
> > other paths into the destination block.
>
> If we could find a condition that took control flow through the
> forwarding path vs other paths, we could try to preserve them with a
> ternary expression that uses an unbound debug temp as the reset value,
> the closest to a conditional debug bind we could have right now. But I
> guess it would make no difference once we attempted to resolve the
> expression and found a subexpression to be reset.
>
> > I'm less sure what would be the correct action for DEBUG_BEGIN
> > stmts (the patch contines to drop them on the floor, leaving
> > reset debug-binds possibly surrounded by wrong stmt context?).
>
> Dropping them is the best we can do now.
>
> > Not sure what else debug stmt kinds we have and what the proper
> > action for those would be.
>
> Inline entry point markers, also to be dropped for now.
>
> > Somewhere I've written that debug-stmts living on edges would
> > also solve the issue on the GIMPLE (and RTL) side, not sure
> > if we can make sense of those in the end though.
>
> Once you go down that path, I guess you'll soon find a need for control
> flow graphs within edges, at which point it gets really ugly.
>
> I'm slightly more hopeful about identifying guarding conditions to
> introduce conditional debug stmts.
>
> > Having stmts permanently on edges is also sth new on GIMPLE so I'm
> > staying away from that at this moment...
>
> *firm nod* :-)
>
>
> > I've noted that for the specific case where there is
> > (before the next DEBUG_BEGIN_STMT? before the next real stmt?)
> > (debug-) definitions of the debug vars we reset in the destination
> > block then dropping the debug stmt might be better and avoids
> > the debug-info quality degadation.
>
> If there's any instruction or view that would be reached by the earlier
> bind (the one that precedes the one we'd drop or reset), it would get
> wrong debug information if we were to drop the bind rather than reset
> it. If there isn't, whatever happens to the bind will have no effect.
> This suggests to me that resetting it is the right thing to do.
Hmm. I was thinking of
void foo(int n)
{
if (n == 0)
return;
int i = 0;
do
{
++i;
}
while (i < n);
}
where before CCP we have
<bb 2> :
# DEBUG BEGIN_STMT
if (n_2(D) == 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
<bb 4> :
# DEBUG BEGIN_STMT
i_3 = 0;
# DEBUG i => i_3
<bb 5> :
# i_1 = PHI <i_3(4), i_4(5)>
# DEBUG i => i_1
# DEBUG BEGIN_STMT
# DEBUG BEGIN_STMT
i_4 = i_1 + 1;
# DEBUG i => i_4
if (i_4 < n_2(D))
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
<bb 3> :
# DEBUG BEGIN_STMT
// predicted unlikely by early return (on trees) predictor.
goto <bb 6>; [INV]
<bb 6> :
return;
and then CCP propagates out i_3 = 0 resulting in
# DEBUG i => 0
and bb 4 degrading into a forwarder block CFG cleanup removes. Now
before the patch we'd drop the above but afterwards we end up with
<bb 4> :
# i_1 = PHI <0(2), i_4(4)>
# DEBUG i => NULL
# DEBUG i => i_1
# DEBUG BEGIN_STMT
# DEBUG BEGIN_STMT
i_4 = i_1 + 1;
# DEBUG i => i_4
if (i_4 < n_2(D))
goto <bb 4>; [INV]
else
goto <bb 5>; [INV]
that might be OK(?) and in gdb I can't find it doing any harm. What
I suggested was to, for example, notice that there's a PHI defining
'i' in the destination and thus it would be safe to drop the debug-bind
(it's now associated with a "wrong" stmt/view since we dropped the
DEBUG BEGIN_STMT as well?). Similarly we immediately arrive at
# DEBUG i => i_1 and thus dopping would be OK even if the actual
IV were replaced by another one?
Since I've now done the above experiment in gdb I feel safer with
the idea btw - at least this kind of simple cases do not regress
visibly ;)
> Now, if we were to try to duplicate the logic that decides whether the
> bind might possibly be observable, in order to drop it on the floor
> instead of resetting it, we should look for another bind of the same
> variable before any real stmt or debug marker. If we find one, it would
> be ok to drop the bind instead of resetting it. I suppose we might even
> make this part of the debug bind resetting logic, or introduce separate
> but reusable functionality for this sort of cleanup.
Hmm, but with the BEGIN_STMT markers still in place the views would
make the different values observable, no?
> But didn't we have
> something to that effect already? I vaguely recall Jakub implemented
> something that would drop binds that could not be observed, and that it
> became a lot less effective after adjusting it to deal with view
> markers.
>
> > I guess that at least looking at all PHI nodes of the destination
> > might be a good idea because those defs happen before the "new" debug
> > reset.
>
> /me mumbles something about PHI debug binds ;-)
Eh. But yes, sth like
# DEBUG PHI i => <NULL(2), i(5)>
meaning to reset on the edge into the loop and keep it "as is"
on the other edge would be equivalent to having the debug reset
on the edge. That would of course just delay the issue to RTL
expansion where PHIs get replaced by copies (and that has to be
compare-debug safe). OTOH in (non-CFG-layout-mode?) RTL we can
have insns between basic blocks (aka on edges).
>
> > 2019-04-03 Richard Biener <rguent...@suse.de>
>
> > PR debug/89892
> > PR debug/89905
> > * tree-cfgcleanup.c (remove_forwarder_block): Always move
> > debug bind stmts but reset them if they are not valid at the
> > destination.
>
> > * gcc.dg/guality/pr89892.c: New testcase.
> > * gcc.dg/guality/pr89905.c: Likewise.
>
> LGTM, thanks.
>
> Any reason to mention PR 88882 in the Subject: but not in the ChangeLog?
I've not added its testcase (and meanwhile closed all of them as dups).
I'll see to add a guality testcase for my simple loop example above
(and try to trick it into going wrong with the patch some more) and
then apply the patch tomorrow.
Thanks,
Richard.
