llvm dev - Apr 2018 - [RFC] Generate Debug Information for Labels in Function

On Fri, Mar 30, 2018 at 9:39 AM Adrian Prantl <aprantl at apple.com>
wrote:
> I'm really sorry for not realizing this yesterday, but the problems
> pertaining to inlining made me realize that your original design with the
> dbg.label intrinsic might actually be a better approach especially when
> considering optimized code. We will get inlining support for free because
> it is just another instruction and it can deal with more than one label at
> the same address. It looks a bit more complicated in unoptimized code, but
> that seems like a small price to pay.
>
> We just need to make sure that the backend doesn't get confused when
loop
> unrolling duplicates a dbg.label but that should be doable.
>
+10, it should be an intrinsic.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180330/b9251ab4/attachment.html>

> -----Original Message-----
> From: Hsiangkai Wang [mailto:hsiangkai at gmail.com]
> Sent: Sunday, April 01, 2018 12:12 PM
> To: Reid Kleckner
> Cc: Adrian Prantl; llvm-dev; Robinson, Paul; David Blaikie; Eric
> Christopher
> Subject: Re: [llvm-dev] [RFC] Generate Debug Information for Labels in
> Function
> 
> Hi all,
> 
> I am sorry that I didn’t carefully think about how to handle labels in
> inlined function.
> 
> Today, I did some simple experiments and found that some basic block
> may be removed in CFGSimplifyPass and the attached metadata will be
> eliminated. So, if the optimization is turned on, label metadata will
> disappear. However, intrinsic will keep existing in the function if we
> handle it correctly. So, if we take care about optimization and
> inlining, to keep label metadata through intrinsic could be right for
> free.
It makes sense that basic blocks might be manipulated (split, merged, etc)
in ways such that it would require a lot of attention to preserve metadata 
correctly.  An intrinsic is harder to lose. :-)   I suppose if the entire 
basic block is optimized away, the label is really dead, and in that case 
it is okay to remove the intrinsic.
> 
> I learned a lot from discussions. Thanks for your comments and
> suggestions. If there is no other concern, I will keep my original
> implementation and send patches to Phabricator again. Sorry have
> bothered you.
No trouble.  This is what discussions are for; they have value, even if
the conclusion is "it was right the first time."
> 
> Any comments?
The previous patch set looked reasonable, however please post patches
in testable units, with their tests in the same review.  This helps
reviewers to understand what you are doing, and makes it easier to see
whether there are missing pieces.
Thanks!
--paulr
> 
> On Sat, Mar 31, 2018 at 5:38 AM, Reid Kleckner <rnk at google.com>
wrote:
> > On Fri, Mar 30, 2018 at 9:39 AM Adrian Prantl <aprantl at
apple.com> wrote:
> >>
> >> I'm really sorry for not realizing this yesterday, but the
problems
> >> pertaining to inlining made me realize that your original design
with
> the
> >> dbg.label intrinsic might actually be a better approach especially
when
> >> considering optimized code. We will get inlining support for free
> because it
> >> is just another instruction and it can deal with more than one
label at
> the
> >> same address. It looks a bit more complicated in unoptimized code,
but
> that
> >> seems like a small price to pay.
> >>
> >> We just need to make sure that the backend doesn't get
confused when
> loop
> >> unrolling duplicates a dbg.label but that should be doable.
> >
> >
> > +10, it should be an intrinsic.

> On Apr 1, 2018, at 9:12 AM, Hsiangkai Wang <hsiangkai at gmail.com>
wrote:
> 
> Hi all,
> 
> I am sorry that I didn’t carefully think about how to handle labels in
> inlined function.
There is no need to apologize! Thank you very much for engaging in the
discussion and for contributing you patches.
> 
> Today, I did some simple experiments and found that some basic block
> may be removed in CFGSimplifyPass and the attached metadata will be
> eliminated. So, if the optimization is turned on, label metadata will
> disappear. However, intrinsic will keep existing in the function if we
> handle it correctly. So, if we take care about optimization and
> inlining, to keep label metadata through intrinsic could be right for
> free.
> 
> I learned a lot from discussions. Thanks for your comments and
> suggestions. If there is no other concern, I will keep my original
> implementation and send patches to Phabricator again.
That sounds like a good plan.

-- adrian