[llvm] r310985 - Merge debug info when hoist then-else code to if.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 25 18:02:28 PDT 2017


On Fri, Aug 25, 2017 at 5:58 PM Robinson, Paul <paul.robinson at sony.com>
wrote:

> Beyond that we get to the zero location - but in what scope?
>
>
>
> The scope of where it's hoisted into; in this case, the scope of the
> branch instruction.  Which could be a parent of the common parent of the
> hoisted instruction, but so what.  The scope of a line-0 location is not
> especially relevant,
>

Seems pretty important to me in terms of whether or not it's part of an
inlined function (that's modeled with the same scope infrastructure), for
example. (& also in terms of splitting ranges - which is mostly just about
the size of the debug info - range fragments add more overhead, etc)

The branch instruction could be from a caller into two inlined calls, etc.


> and using the scope of where the instruction lands seems least disruptive.
>
> --paulr
>
>
>
> *From:* llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] *On
> Behalf Of *David Blaikie via llvm-commits
> *Sent:* Friday, August 25, 2017 5:10 PM
> *To:* Adrian Prantl
> *Cc:* llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r310985 - Merge debug info when hoist then-else
> code to if.
>
>
>
>
>
> On Fri, Aug 25, 2017 at 4:44 PM Adrian Prantl <aprantl at apple.com> wrote:
>
> On Aug 25, 2017, at 3:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> Adrian - what were we doing about cases where merged locations were on
> call instructions? I think this case is one of those, where if the
> instruction is a call, this may drop the location from the call, then
> inline through the call creating corrupted debug info.
>
>
>
> I guess that in this case the only sane thing we can do is create a new
> artificial line 0 location in one of the two calls' scope. It's not pretty,
> but that;s the best I can come up with.
>
>
> A few options:
>
> Further up in the same file, the code avoids merging if the instruction is
> a call (originally r380995) SimplifyCFG.cpp:1282.
>
> So that's one option - do nothing on calls. But it's probably not
> ideal/wrong (since it's not either location)
>
> Beyond that we get to the zero location - but in what scope?
> The root scope of the function? (what if these two calls are both within a
> single inlined call? That'll punch a hole in the range which isn't entirely
> accurate) The nearest common parent scope to the two instructions (assuming
> they both have debug locations? If one doesn't have a location, could we
> reliably use the other location? Probably not - so if one doesn't have a
> location we should use the zero location at the root of the current
> function for sure)
> Worth finding the common parent scope?
> Should this functionality be built into getMergedLocation?
>
>
>
>
> -- adrian
>
>
>
>
> Here's a test case that seems to exercise/assert on that (but it asserts
> relatively late - would be handy to have a way to run the verifier between
> each optimization - maybe there is such an option?):
>
> struct string {
>
>   ~string();
>
> };
>
> void f2();
>
> void f1(int) { f2(); }
>
> void run(int c) {
>
>   string body;
>
>   while (true) {
>
>     if (c)
>
>       f1(c);
>
>     else
>
>       f1(c);
>
>   }
>
> }
>
>
>
> On Tue, Aug 15, 2017 at 6:56 PM Dehao Chen via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: dehao
> Date: Tue Aug 15 18:55:26 2017
> New Revision: 310985
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310985&view=rev
> Log:
> Merge debug info when hoist then-else code to if.
>
> Summary: When we move then-else code to if, we need to merge its debug
> info, otherwise the hoisted instruction may have inaccurate debug info
> attached.
>
> Reviewers: aprantl, probinson, dblaikie, echristo, loladiro
>
> Reviewed By: aprantl
>
> Subscribers: sanjoy, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D36778
>
> Added:
>     llvm/trunk/test/Transforms/SimplifyCFG/min_dbginfo.ll
> Modified:
>     llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>
> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=310985&r1=310984&r2=310985&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Tue Aug 15 18:55:26
> 2017
> @@ -1336,6 +1336,8 @@ HoistTerminator:
>      I2->replaceAllUsesWith(NT);
>      NT->takeName(I1);
>    }
> +  NT->setDebugLoc(DILocation::getMergedLocation(
> +      I1->getDebugLoc(), I2->getDebugLoc()));
>
>    IRBuilder<NoFolder> Builder(NT);
>    // Hoisting one of the terminators from our successor is a great thing.
>
> Added: llvm/trunk/test/Transforms/SimplifyCFG/min_dbginfo.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/min_dbginfo.ll?rev=310985&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/SimplifyCFG/min_dbginfo.ll (added)
> +++ llvm/trunk/test/Transforms/SimplifyCFG/min_dbginfo.ll Tue Aug 15
> 18:55:26 2017
> @@ -0,0 +1,39 @@
> +; RUN: opt < %s -simplifycfg -S | FileCheck %s
> +
> +; Checks if the debug info is removed for the "select" instruction.
> +; CHECK: cmp {{.*}} !dbg
> +; CHECK-NOT: select {{.*}} !dbg
> +define i32 @min(i32 %a, i32 %b) {
> +entry:
> +  %cmp = icmp slt i32 %a, %b, !dbg !9
> +  br i1 %cmp, label %if.then, label %if.else, !dbg !10
> +
> +if.then:
> +  br label %return, !dbg !11
> +
> +if.else:
> +  br label %return, !dbg !12
> +
> +return:
> +  %retval.0 = phi i32 [ %a, %if.then ], [ %b, %if.else ]
> +  ret i32 %retval.0, !dbg !13
> +}
> +
> +!llvm.dbg.cu = !{!0}
> +!llvm.module.flags = !{!3, !4, !5}
> +!llvm.ident = !{!6}
> +
> +!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1,
> producer: "clang version 6.0.0 (trunk 310792)", isOptimized: true,
> runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
> +!1 = !DIFile(filename: "min.cc", directory: "/")
> +!2 = !{}
> +!3 = !{i32 2, !"Dwarf Version", i32 4}
> +!4 = !{i32 2, !"Debug Info Version", i32 3}
> +!5 = !{i32 1, !"wchar_size", i32 4}
> +!6 = !{!"clang version 6.0.0 (trunk 310792)"}
> +!7 = distinct !DISubprogram(name: "min", scope: !1, file: !1, line: 3,
> type: !8, isLocal: false, isDefinition: true, scopeLine: 3, flags:
> DIFlagPrototyped, isOptimized: true, unit: !0, variables: !2)
> +!8 = !DISubroutineType(types: !2)
> +!9 = !DILocation(line: 4, column: 8, scope: !7)
> +!10 = !DILocation(line: 4, column: 6, scope: !7)
> +!11 = !DILocation(line: 5, column: 3, scope: !7)
> +!12 = !DILocation(line: 7, column: 3, scope: !7)
> +!13 = !DILocation(line: 9, column: 1, scope: !7)
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170826/ace0d5a5/attachment.html>


More information about the llvm-commits mailing list