[PATCH] D53390: [DebugInfo][Dexter] Unreachable line stepped onto after SimplifyCFG.
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 6 07:28:03 PST 2018
vsk added inline comments.
================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1379
+ Instruction *InsertPt = BIParent->size() > 1
+ ? BIParent->getTerminator()->getPrevNode()
+ : BIParent->getTerminator();
----------------
CarlosAlbertoEnciso wrote:
> CarlosAlbertoEnciso wrote:
> > vsk wrote:
> > > CarlosAlbertoEnciso wrote:
> > > > CarlosAlbertoEnciso wrote:
> > > > > vsk wrote:
> > > > > > A bit nitty-gritty, but: Can the instruction before the terminator be a debug info intrinsic?
> > > > > >
> > > > > > If so, I think the line location you pick be different if you switch from -gline-tables-only versus -g.
> > > > > Thanks for your review.
> > > > >
> > > > > Changing the test case to:
> > > > >
> > > > > ```
> > > > > int main() {
> > > > > volatile int foo = 0;
> > > > >
> > > > > int beards = 0;
> > > > > bool cond = foo == 4;
> > > > > if (cond)
> > > > > beards = 8;
> > > > > else
> > > > > beards = 4;
> > > > >
> > > > > volatile bool tmp;
> > > > > volatile bool *face = &tmp;
> > > > > *face = cond;
> > > > >
> > > > > return beards;
> > > > > }
> > > > > ```
> > > > >
> > > > > The IR will look like:
> > > > >
> > > > > ```
> > > > > ...
> > > > > %foo.0. = load volatile i32, i32* %foo, align 4, !dbg !28, !tbaa !23
> > > > > %cmp = icmp eq i32 %foo.0., 4, !dbg !28
> > > > > %frombool = zext i1 %cmp to i8, !dbg !28
> > > > > call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !28
> > > > > br i1 %cmp, label %if.then, label %if.else, !dbg !29
> > > > >
> > > > > if.then: ; preds = %entry
> > > > > call void @llvm.dbg.value(metadata i32 8, metadata !15, metadata !DIExpression()), !dbg !27
> > > > > br label %if.end, !dbg !30
> > > > >
> > > > > if.else: ; preds = %entry
> > > > > call void @llvm.dbg.value(metadata i32 4, metadata !15, metadata !DIExpression()), !dbg !27
> > > > > br label %if.end
> > > > >
> > > > > if.end: ; preds = %if.else, %if.then
> > > > > ...
> > > > > ```
> > > > >
> > > > > The instruction before the terminator is a debug info intrinsic.
> > > > >
> > > > >
> > > > > If so, I think the line location you pick be different if you switch from -gline-tables-only versus -g.
> > > >
> > > > IR for '-g'
> > > >
> > > > ```
> > > > ...
> > > > %cmp = icmp eq i32 %foo.0., 4, !dbg !26
> > > > %frombool = zext i1 %cmp to i8, !dbg !26
> > > > call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !26
> > > > br i1 %cmp, label %if.then, label %if.else, !dbg !27
> > > > ...
> > > > !26 = !DILocation(line: 5, scope: !8)
> > > > ```
> > > >
> > > > IR for '-gline-tables-only'
> > > >
> > > > ```
> > > > ...
> > > > %cmp = icmp eq i32 %foo.0., 4, !dbg !15
> > > > %frombool = zext i1 %cmp to i8, !dbg !15
> > > > br i1 %cmp, label %if.then, label %if.else, !dbg !16
> > > > ...
> > > > !15 = !DILocation(line: 5, scope: !8)
> > > > ...
> > > > ```
> > > > InsertPts
> > > > ```
> > > > call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !26
> > > >
> > > > &&
> > > >
> > > > %frombool = zext i1 %cmp to i8, !dbg !15
> > > > ```
> > > >
> > > > For both options, the line location of the insertion point is the same, debug location `!DILocation(line: 5, scope: !8)`.
> > > >
> > > >
> > > I think it's still possible to construct cases where -g vs. -gline-tables-only produces different locations. What if you add `int bar = 0` on the line after `bool cond = foo == 4`? I see this:
> > >
> > > ```
> > > %6 = zext i1 %5 to i8, !dbg !31
> > > call void @llvm.dbg.value(metadata i8 %6, metadata !15, metadata !DIExpression()), !dbg !31
> > > call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !32
> > > br i1 %5, label %7, label %8, !dbg !33
> > > ```
> > >
> > > Where !32 and !31 are on different lines. Link in godbolt: https://godbolt.org/z/VswzLx
> > You are correct. By adding that line, the insertion points have different debug locations:
> >
> > ```
> > call void @llvm.dbg.value(metadata i32 0, metadata !18, metadata !DIExpression()), !dbg !28
> > !28 = !DILocation(line: 6, scope: !8)
> > ```
> > vs
> >
> > ```
> > %frombool = zext i1 %cmp to i8, !dbg !15
> > !15 = !DILocation(line: 5, scope: !8)
> > ```
> >
> >
> These are the IRs for both options:
>
> ```
> ...
> %cmp = icmp eq i32 %foo.0., 4, !dbg !27
> %frombool = zext i1 %cmp to i8, !dbg !27
> call void @llvm.dbg.value(metadata i8 %frombool, metadata !16, metadata !DIExpression()), !dbg !27
> call void @llvm.dbg.value(metadata i32 0, metadata !18, metadata !DIExpression()), !dbg !28
> br i1 %cmp, label %if.then, label %if.else, !dbg !29
> ...
> !27 = !DILocation(line: 5, scope: !8)
> !28 = !DILocation(line: 6, scope: !8)
> ```
> &
> ```
> ...
> %cmp = icmp eq i32 %foo.0., 4, !dbg !15
> %frombool = zext i1 %cmp to i8, !dbg !15
> br i1 %cmp, label %if.then, label %if.else, !dbg !16
> ...
> !15 = !DILocation(line: 5, scope: !8)
> ```
>
> What I can propose to have the same behavior for both options (-g vs -gline-tables-only), is for the case when the insertion point is a debug intrinsic, to find the previous instruction that is not a debug intrinsic (within the same BB), to be used as insertion point.
>
> Create a function `Instruction::getPrevNonDebugInstruction()` to get that instruction. We have already `Instruction::getNextNonDebugInstruction()`.
>
> Doing that, then for the above IRs, for both cases, the insertion point will be:
>
> ```
> %frombool = zext i1 %cmp to i8, !dbg !27
> ```
> or
>
> ```
> %frombool = zext i1 %cmp to i8, !dbg !15
> ```
>
> that have the same debug location.
>
> What to do if the BB contains only debug instructions?
>
Sgtm. Maybe use an empty location when no previous one can be found.
https://reviews.llvm.org/D53390
More information about the llvm-commits
mailing list