[PATCH] D29833: Improve the API of DILocation::getMergedLocation()

[David Blaikie via llvm-commits]

Ah, sure - if it comes from two different calls (even to the same function)
there's no inlined scope that would be correct, so it should be dropped.

On Tue, Feb 14, 2017 at 11:03 AM Taewook Oh <twoh at fb.com> wrote:

> I think I was unclear. What I meant was that it would be better if the
> merged and hoisted call instruction has a scope of “@test, line: 0, column:
> 0”, not “@bar, line: 0, column: 0”.
>
>
>
> Thanks,
>
> Taewook
>
>
>
> *From: *David Blaikie <dblaikie at gmail.com>
> *Date: *Tuesday, February 14, 2017 at 10:49 AM
> *To: *"reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org" <
> reviews+D29833+public+d0b46dd83e56e03f at reviews.llvm.org>, "
> aprantl at apple.com" <aprantl at apple.com>, "paul.robinson at sony.com" <
> paul.robinson at sony.com>, "danielcdh at gmail.com" <danielcdh at gmail.com>, "
> echristo at gmail.com" <echristo at gmail.com>, "rnk at google.com" <rnk at google.com>,
> "rob.lougher at gmail.com" <rob.lougher at gmail.com>
> *Cc: *Taewook Oh <twoh at fb.com>, "andrea.dibiagio at gmail.com" <
> andrea.dibiagio at gmail.com>, "llvm-commits at lists.llvm.org" <
> llvm-commits at lists.llvm.org>, Mehdi AMINI <mehdi.amini at apple.com>
> *Subject: *Re: [PATCH] D29833: Improve the API of
> DILocation::getMergedLocation()
>
>
>
> Ah, sure - we can't/shouldn't remove any inline scope information (if it's
> unambiguous - if we hoisted from two different function calls we should
> probably drop the inlining scope information in the same way that we drop
> the ambiguous locations, unfortunately), but block scopes may or may not be
> helpful to preserve. I'm not sure what the right answer is there.
>
>
>
> On Tue, Feb 14, 2017 at 10:44 AM Taewook Oh via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> twoh added inline comments.
>
>
> ================
> Comment at: lib/IR/Instruction.cpp:664-668
> +  if (Loc && isa<CallInst>(this)) {
> +    DbgLoc = DILocation::get(Loc->getContext(), 0, 0, Loc->getScope(),
> +                             Loc->getInlinedAt());
> +    return;
> +  }
> ----------------
> dblaikie wrote:
> > I'm not sure we can get the scope right, though - since we're
> potentially moving this location across scopes. It's going to be
> jumpy/create a difficult range in the scope no matter what, really...
> >
> > Any ideas? Take the scope from a nearby location at the destination, if
> possible? (doesn't really reflect reality, but nothing will - this would at
> least mean a chance of not punching holes in/making islands in scopes that
> would necessitate a DW_AT_ranges/more verbose description/etc)
> It would be hard to get the precise scope anyway, but I think it might be
> worth to consider the inlining context. Please take a look at the following
> test case:
>
> ```
> define i32 @bar() !dbg !6 {
> entry:
>   %call = tail call i32 @foo(), !dbg !8
>   ret i32 %call, !dbg !9
> }
>
> define i32 @test(i32 %a, i32 %b) !dbg !10 {
> entry:
>   %tobool = icmp ne i32 %a, 0, !dbg !11
>   br i1 %tobool, label %if.then, label %if.else, !dbg !11
>
> if.then:                                          ; preds = %entry
>   %call.i = call i32 @foo(), !dbg !12
>   %sub = sub nsw i32 %b, %call.i, !dbg !14
>   br label %if.end, !dbg !15
>
> if.else:                                          ; preds = %entry
>   %call1 = call i32 @foo(), !dbg !16
>   %sub2 = sub nsw i32 %b, %call1, !dbg !17
>   br label %if.end
>
> if.end:                                           ; preds = %if.else,
> %if.then
>   %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
>   ret i32 %b.addr.0, !dbg !18
> }
> ```
>
> where
>
> ```
> !6 = distinct !DISubprogram(name: "bar", ...)
> !10 = distinct !DISubprogram(name: "test", ...)
> !12 = !DILocation(line: 4, column: 10, scope: !6, inlinedAt: !13)
> !16 = !DILocation(line: 11, column: 10, scope: !10)
> ```
>
> Here the call instruction in %if.then block is actually inlined from @bar,
> and if we pickup the scope from that instruction, the final code would be
> look something like below:
>
> ```
> define i32 @test(i32 %a, i32 %b) !dbg !10 {
> entry:
>   %call.i = call i32 @foo(), !dbg !11
>   %sub = sub nsw i32 %b, %call.i, !dbg !13
>   ret i32 %sub, !dbg !14
> }
>
> !6 = distinct !DISubprogram(name: "bar", ...)
> !11 = !DILocation(line: 0, column: 0, scope: !6, inlinedAt: !12)
> ```
>
> As the call instructions are merged and hoisted inside test not bar, it
> would be awkward if the merged call instruction has a scope of bar, though
> the debug location is still just (line: 0, column: 0).
>
> I missed the discussion in llvm-dev and implemented a patch for the same
> issue considering inline scope (D29927). Maybe worth take a look.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D29833
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D29833&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=9Leqrn73ABLnKnbnINVXAvQ8dotwlC-Yd0CdOm037Mo&s=_IcWlv6Hg_yo5fDdIiU0jfSUExVmbDNqlChJLK--7Y4&e=>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170214/b0bce693/attachment.html>