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

[Sergey Dmitrouk]

Keith,

I tried adding debug location pointer to profile of constant nodes in
lib/CodeGen/SelectionDAG/SelectionDAG.cpp:AddNodeIDCustom() like this:

    ID.AddPointer(C->getDebugLoc().get());

And it did fix output for your sample file, but there are bad consequences
for existing tests.  Some fail because of additional instructions (as
some constants are not eliminated), some cause llc crashes (too deep
recursion), some hang with 100% CPU load...

In other words it's not very promising solution, but in any case correct
debug locations will require this bloat of constants or there will be
cases where locations are wrong.

If you wish to effectively revert the change, it will require only single
line change in the constructor of ConstantSDNode
(include/llvm/CodeGen/SelectionDAGNodes.h), like this:

 class ConstantSDNode : public SDNode {
   const ConstantInt *Value;
   friend class SelectionDAG;
   ConstantSDNode(bool isTarget, bool isOpaque, const ConstantInt *val,
                  DebugLoc DL, EVT VT)
     : SDNode(isTarget ? ISD::TargetConstant : ISD::Constant,
-             0, DL, getSDVTList(VT)), Value(val) {
+             0, DebugLoc(), getSDVTList(VT)), Value(val) {
     SubclassData |= (uint16_t)isOpaque;
   }

+ plus disabling related tests.

-- 
Sergey

On Tue, May 05, 2015 at 09:40:37AM -0700, Keith Walker wrote:
> Sergey,
> 
> Thanks for looking at the cause of why the line information for the constant
> was being associated with the wrong line; at least I now understand why it
> is occurring ..... did your experimentations suggested whether there was any
> simple solution for this case?
> 
> Keith
> 
> > -----Original Message-----
> > From: Sergey Dmitrouk [mailto:sdmitrouk at accesssoftek.com]
> > Sent: 01 May 2015 19:05
> > To: Keith Walker; Renato Golin
> > Cc: LLVM Commits
> > Subject: Re: [llvm] r235989 - Reapply r235977 "[DebugInfo] Add debug
> > locations to constant SD nodes"
> > 
> > Thanks for spotting this Keith, the intension is to get better debug
> > locations on ARM.  My English can be quite misleading...
> > 
> > On Fri, May 01, 2015 at 08:26:33AM -0700, Renato Golin wrote:
> > > On 1 May 2015 at 15:53, Keith Walker <kwalker at arm.com> wrote:
> > > > But after this change there is a "spurious" line entry for the
> > instruction
> > > > at 0x18 loading the constant 0 which is incorrectly associated with
> > the line
> > > > 6.
> > >
> > > From the size of the change, this looks like a bad assumption in one
> > > of the unrelated SDLocs.
> > 
> > Likely in general, but not true in this case.  Here
> > SelectionDAG::getConstant() checks for constant with the same value and
> > returns previously used node for the line above (there is zero offset in
> > store instruction for the first assignment).
> > 
> > In other words, debug locations are not part of node profile used by
> > FoldingSet and I'm not sure whether it's safe to change this even just
> > for
> > constant nodes.  It will prohibit identical nodes elimination, although
> > debug locations should become correct.  I'll try this out.
> > 
> > > Why haven't you added "SDLoc DL," to the end of the list of arguments?
> > > It'd have made the patch a lot smaller and simpler.
> > 
> > I didn't want to add to existing inconsistency in order of arguments
> > among methods of SelectionDAG.  Most of them (not all) tend to have
> > DebugLoc
> > argument before argument of type EVT, so I did the same.
> > 
> > Another reason is default arguments, SDLoc is mandatory argument and
> > can't be placed after defaulted ones.  This left me with the choice
> > before/after EVT.
> > 
> > Best regards,
> > Sergey