[llvm] r235989 - Reapply r235977 "[DebugInfo] Add debug locations to constant SD nodes"

[Sergey Dmitrouk]

Keith,

That line is basically the only real change in whole commit, all other
lines just move debug location so it can be used in that constructor (by
the way, I recently noticed that I failed to update ConstantFPSDNode
constructor to accept DebugLoc).

All,

After trying it out I'm not sure one can disable CSE of constant nodes,
everything falls apart after that...  Some functions are reentrant and
the only way they stop recursive calls is when cached node returns
(hence crashes due to stack overflow), others iterate until something
similar occurs (these are turned into infinite loops).

Good solution seems to require separating DebugLoc from ConstantSDNode,
so that same constant could have multiple locations, but I'm not sure
how to do this without breaking all casts.

How about somewhat hackish solution that uses correct debug location,
but forgets it should same constant be used at several locations?
The hackish part: node is created with debug location, which can be
erased later.  I understand this is not ideal and won't be accurate
for constant with multiple uses, but it might be a reasonable compromise.

I tried this solution on example from https://llvm.org/bugs/show_bug.cgi?id=13269
and some other snippets I have including the one provided by Keith, all
seem to output expected results.

In effect we would have correct locations for some cases and not-that-bad
(like before this change) results for the rest.  This could certainly be
improved later if anyone knows a way... (Looks more like a design issue
to me.)

This is the best idea I could come up with so far, too many parts of
LLVM make assumptions on constant nodes, that's why it's hard to make any
related changes.

-- 
Sergey

On Thu, May 07, 2015 at 03:57:58AM -0700, Keith Walker wrote:
> Sergey,
> 
> You have my vote (for what it's worth) on making the change in the
> constructor in order to restore the previous behaviour without necessarily
> reverting all the API changes.
> 
> However other more experienced people who know the code in this area might
> have a different view.
> 
> Keith
> 
> > -----Original Message-----
> > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > bounces at cs.uiuc.edu] On Behalf Of Sergey Dmitrouk
> > Sent: 07 May 2015 11:07
> > To: David Blaikie
> > Cc: llvm-commits at cs.uiuc.edu
> > Subject: Re: [llvm] r235989 - Reapply r235977 "[DebugInfo] Add debug
> > locations to constant SD nodes"
> > 
> > On Wed, May 06, 2015 at 04:05:54PM -0700, David Blaikie wrote:
> > >    This seems to produce some regressions, for example:
> > >
> > >    https://www.diffchecker.com/p2ladh2w
> > >
> > >    The constant used in both the inlined function and outside the
> > inlined
> > >    function is attributed to the first use (the inlined function), but
> > then
> > >    the codegen for the inlined function's constant ends up morphed
> > into a
> > >    setg and inc, so the constant used outside the inlined function
> > ends up
> > >    after some of the outer code, but attributed to it...
> > >
> > >    GDB doesn't seem to trip over this (but in theory, stepping through
> > this
> > >    code should cause the debugger to go in/out of the inlined function
> > more
> > >    than once) but does seem incorrect.
> > 
> > It's basically different manifestation of the issue noted by Keith.
> > First ConstantSDNode for some value gets cached and used in all
> > appearances
> > of the same constant, as debug location is bound to ConstantSDNode all
> > such
> > constants get location of the first one.
> > 
> > Strictly saying this is not a regression, it just happened to be more or
> > less correct before this change, but it was more of a luck than
> > intention.
> > 
> > Do you want me to disable the change until I figure out how to solve
> > this issue with constants?  Not reverting the commit, just dropping
> > using
> > location argument in ConstantSDNode constructor, so that API changes
> > will still be there.
> > 
> > >    Also, this causes substantial increases to debug info size,
> > especially for
> > >    -gmlt, due to many of these discontiguous ranges... that seems
> > unfortunate
> > >    (though some of them may be legitimate).
> > 
> > Not sure this will go away completely, looks like it's either this or
> > wrong
> > locations, but if constants will get correct locations, ranges should
> > become more contiguous.
> > 
> > --
> > Sergey
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits