<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 7, 2015 at 6:38 AM, Sergey Dmitrouk <span dir="ltr"><<a href="mailto:sdmitrouk@accesssoftek.com" target="_blank">sdmitrouk@accesssoftek.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Keith,<br>
<br>
That line is basically the only real change in whole commit, all other<br>
lines just move debug location so it can be used in that constructor (by<br>
the way, I recently noticed that I failed to update ConstantFPSDNode<br>
constructor to accept DebugLoc).<br>
<br>
All,<br>
<br>
After trying it out I'm not sure one can disable CSE of constant nodes,<br>
everything falls apart after that...  Some functions are reentrant and<br>
the only way they stop recursive calls is when cached node returns<br>
(hence crashes due to stack overflow), others iterate until something<br>
similar occurs (these are turned into infinite loops).<br>
<br>
Good solution seems to require separating DebugLoc from ConstantSDNode,<br>
so that same constant could have multiple locations, but I'm not sure<br>
how to do this without breaking all casts.<br>
<br>
How about somewhat hackish solution that uses correct debug location,<br>
but forgets it should same constant be used at several locations?<br>
The hackish part: node is created with debug location, which can be<br>
erased later.  I understand this is not ideal and won't be accurate<br>
for constant with multiple uses, but it might be a reasonable compromise.<br></blockquote><div><br>Somewhat hackish, as you say, but not too bad - and seems like a reasonable compromise given the limitations we're dealing with (limitations like constant coalescing, multiple uses, etc)<br><br>If you could disable your change first, then work on this fix, that would be good.<br><br>(usually I'd be in favor of reverting the whole patch - but I realize this sort of API churn can bitrot quickly and the fix you've described doesn't sound like it'll take too long, etc)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I tried this solution on example from <a href="https://llvm.org/bugs/show_bug.cgi?id=13269" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=13269</a><br>
and some other snippets I have including the one provided by Keith, all<br>
seem to output expected results.<br>
<br>
In effect we would have correct locations for some cases and not-that-bad<br>
(like before this change) results for the rest.  This could certainly be<br>
improved later if anyone knows a way... (Looks more like a design issue<br>
to me.)<br>
<br>
This is the best idea I could come up with so far, too many parts of<br>
LLVM make assumptions on constant nodes, that's why it's hard to make any<br>
related changes.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Sergey<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Thu, May 07, 2015 at 03:57:58AM -0700, Keith Walker wrote:<br>
> Sergey,<br>
><br>
> You have my vote (for what it's worth) on making the change in the<br>
> constructor in order to restore the previous behaviour without necessarily<br>
> reverting all the API changes.<br>
><br>
> However other more experienced people who know the code in this area might<br>
> have a different view.<br>
><br>
> Keith<br>
><br>
> > -----Original Message-----<br>
> > From: <a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvm-commits-">llvm-commits-</a><br>
> > <a href="mailto:bounces@cs.uiuc.edu">bounces@cs.uiuc.edu</a>] On Behalf Of Sergey Dmitrouk<br>
> > Sent: 07 May 2015 11:07<br>
> > To: David Blaikie<br>
> > Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > Subject: Re: [llvm] r235989 - Reapply r235977 "[DebugInfo] Add debug<br>
> > locations to constant SD nodes"<br>
> ><br>
> > On Wed, May 06, 2015 at 04:05:54PM -0700, David Blaikie wrote:<br>
> > >    This seems to produce some regressions, for example:<br>
> > ><br>
> > >    <a href="https://www.diffchecker.com/p2ladh2w" target="_blank">https://www.diffchecker.com/p2ladh2w</a><br>
> > ><br>
> > >    The constant used in both the inlined function and outside the<br>
> > inlined<br>
> > >    function is attributed to the first use (the inlined function), but<br>
> > then<br>
> > >    the codegen for the inlined function's constant ends up morphed<br>
> > into a<br>
> > >    setg and inc, so the constant used outside the inlined function<br>
> > ends up<br>
> > >    after some of the outer code, but attributed to it...<br>
> > ><br>
> > >    GDB doesn't seem to trip over this (but in theory, stepping through<br>
> > this<br>
> > >    code should cause the debugger to go in/out of the inlined function<br>
> > more<br>
> > >    than once) but does seem incorrect.<br>
> ><br>
> > It's basically different manifestation of the issue noted by Keith.<br>
> > First ConstantSDNode for some value gets cached and used in all<br>
> > appearances<br>
> > of the same constant, as debug location is bound to ConstantSDNode all<br>
> > such<br>
> > constants get location of the first one.<br>
> ><br>
> > Strictly saying this is not a regression, it just happened to be more or<br>
> > less correct before this change, but it was more of a luck than<br>
> > intention.<br>
> ><br>
> > Do you want me to disable the change until I figure out how to solve<br>
> > this issue with constants?  Not reverting the commit, just dropping<br>
> > using<br>
> > location argument in ConstantSDNode constructor, so that API changes<br>
> > will still be there.<br>
> ><br>
> > >    Also, this causes substantial increases to debug info size,<br>
> > especially for<br>
> > >    -gmlt, due to many of these discontiguous ranges... that seems<br>
> > unfortunate<br>
> > >    (though some of them may be legitimate).<br>
> ><br>
> > Not sure this will go away completely, looks like it's either this or<br>
> > wrong<br>
> > locations, but if constants will get correct locations, ranges should<br>
> > become more contiguous.<br>
> ><br>
> > --<br>
> > Sergey<br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>