[DebugInfo] Redundant `.loc` due to Clang issues in codegen of `for` statement

[pogo59]

I'm filing this issue as a follow-up to PR108300. Ordinarily I'd track this as an internal issue within Sony, but as I'm retiring from Sony effective today, I won't have access to my test case or explanation. I do intend to look at this in the next couple of weeks.

Here's my example. It's the simplest form of the for statement; more complicated forms might have additional issues, but let's start small.

int i = 2;
void test_for() {
#line 300
  for (;          i < 10; ++i)
    i += 2;
}

Compile with -g -gno-column-info -S -emit-llvm and we see this IR and location metadata:

for.cond:                                         ; preds = %for.inc, %entry
  %0 = load i32, ptr @i, align 4, !dbg !18
  %cmp = icmp slt i32 %0, 10, !dbg !18
  br i1 %cmp, label %for.body, label %for.end, !dbg !21

!14 = distinct !DISubprogram(....)
!17 = !DILocation(line: 300, scope: !14)
!18 = !DILocation(line: 300, scope: !19)
!19 = distinct !DILexicalBlock(scope: !20, file: !3, line: 300)
!20 = distinct !DILexicalBlock(scope: !14, file: !3, line: 300)
!21 = !DILocation(line: 300, scope: !20)

Observe that the evaluation of the condition uses !18 as its DILocation, while the conditional branch uses !21. These have the same source location, but different scopes. When it comes time to emit the .loc directives for the machine instructions (in llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp), the fact that the branch has a different DILocation causes us to generate a new .loc directive. The fact that the only difference is the scope (which is irrelevant to .loc) is not considered.

Look at the assembly code for the same block:

.Ltmp0:
    .loc    0 300 0 prologue_end            # tc13837-for.cpp:300:0
    jmp .LBB0_1
.LBB0_1:                                # %for.cond
                                        # =>This Inner Loop Header: Depth=1
.Ltmp1:
    .loc    0 300 0 is_stmt 0               # tc13837-for.cpp:300:0
    cmpl    $10, i(%rip)
.Ltmp2:
    .loc    0 300 0                         # tc13837-for.cpp:300:0
    jge .LBB0_4

The .loc at .Ltmp1 is emitted because it is at the start of a new basic block. But the .loc at .Ltmp2 is 100% redundant; it's emitted only because of the scope confusion described above.

Arguably we could filter out the redundant .loc directives in LLVM, but I claim the root cause is in Clang codegen; see the PR cited above, which fixed the equivalent problem with if statements simply by correcting how Clang sets up its notion of DebugLoc.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Paul T Robinson (pogo59)

I'm filing this issue as a follow-up to PR108300. Ordinarily I'd track this as an internal issue within Sony, but as I'm retiring from Sony effective today, I won't have access to my test case or explanation. I do intend to look at this in the next couple of weeks. Here's my example. It's the simplest form of the `for` statement; more complicated forms might have additional issues, but let's start small. ``` int i = 2; void test_for() { #line 300 for (; i < 10; ++i) i += 2; } ``` Compile with `-g -gno-column-info -S -emit-llvm` and we see this IR and location metadata: ``` for.cond: ; preds = %for.inc, %entry %0 = load i32, ptr @i, align 4, !dbg !18 %cmp = icmp slt i32 %0, 10, !dbg !18 br i1 %cmp, label %for.body, label %for.end, !dbg !21 !14 = distinct !DISubprogram(....) !17 = !DILocation(line: 300, scope: !14) !18 = !DILocation(line: 300, scope: !19) !19 = distinct !DILexicalBlock(scope: !20, file: !3, line: 300) !20 = distinct !DILexicalBlock(scope: !14, file: !3, line: 300) !21 = !DILocation(line: 300, scope: !20) ``` Observe that the evaluation of the condition uses `!18` as its `DILocation`, while the conditional branch uses `!21`. These have the same source location, but different scopes. When it comes time to emit the `.loc` directives for the machine instructions (in `llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp`), the fact that the branch has a different `DILocation` causes us to generate a new `.loc` directive. The fact that the only difference is the _scope_ (which is irrelevant to `.loc`) is not considered. Look at the assembly code for the same block: ``` .Ltmp0: .loc 0 300 0 prologue_end # tc13837-for.cpp:300:0 jmp .LBB0_1 .LBB0_1: # %for.cond # =>This Inner Loop Header: Depth=1 .Ltmp1: .loc 0 300 0 is_stmt 0 # tc13837-for.cpp:300:0 cmpl $10, i(%rip) .Ltmp2: .loc 0 300 0 # tc13837-for.cpp:300:0 jge .LBB0_4 ``` The `.loc` at `.Ltmp1` is emitted because it is at the start of a new basic block. But the `.loc` at `.Ltmp2` is 100% redundant; it's emitted only because of the scope confusion described above. Arguably we could filter out the redundant `.loc` directives in LLVM, but I claim the root cause is in Clang codegen; see the PR cited above, which fixed the equivalent problem with `if` statements simply by correcting how Clang sets up its notion of `DebugLoc`.

[llvmbot]

@llvm/issue-subscribers-debuginfo

Author: Paul T Robinson (pogo59)

I'm filing this issue as a follow-up to PR108300. Ordinarily I'd track this as an internal issue within Sony, but as I'm retiring from Sony effective today, I won't have access to my test case or explanation. I do intend to look at this in the next couple of weeks. Here's my example. It's the simplest form of the `for` statement; more complicated forms might have additional issues, but let's start small. ``` int i = 2; void test_for() { #line 300 for (; i < 10; ++i) i += 2; } ``` Compile with `-g -gno-column-info -S -emit-llvm` and we see this IR and location metadata: ``` for.cond: ; preds = %for.inc, %entry %0 = load i32, ptr @i, align 4, !dbg !18 %cmp = icmp slt i32 %0, 10, !dbg !18 br i1 %cmp, label %for.body, label %for.end, !dbg !21 !14 = distinct !DISubprogram(....) !17 = !DILocation(line: 300, scope: !14) !18 = !DILocation(line: 300, scope: !19) !19 = distinct !DILexicalBlock(scope: !20, file: !3, line: 300) !20 = distinct !DILexicalBlock(scope: !14, file: !3, line: 300) !21 = !DILocation(line: 300, scope: !20) ``` Observe that the evaluation of the condition uses `!18` as its `DILocation`, while the conditional branch uses `!21`. These have the same source location, but different scopes. When it comes time to emit the `.loc` directives for the machine instructions (in `llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp`), the fact that the branch has a different `DILocation` causes us to generate a new `.loc` directive. The fact that the only difference is the _scope_ (which is irrelevant to `.loc`) is not considered. Look at the assembly code for the same block: ``` .Ltmp0: .loc 0 300 0 prologue_end # tc13837-for.cpp:300:0 jmp .LBB0_1 .LBB0_1: # %for.cond # =>This Inner Loop Header: Depth=1 .Ltmp1: .loc 0 300 0 is_stmt 0 # tc13837-for.cpp:300:0 cmpl $10, i(%rip) .Ltmp2: .loc 0 300 0 # tc13837-for.cpp:300:0 jge .LBB0_4 ``` The `.loc` at `.Ltmp1` is emitted because it is at the start of a new basic block. But the `.loc` at `.Ltmp2` is 100% redundant; it's emitted only because of the scope confusion described above. Arguably we could filter out the redundant `.loc` directives in LLVM, but I claim the root cause is in Clang codegen; see the PR cited above, which fixed the equivalent problem with `if` statements simply by correcting how Clang sets up its notion of `DebugLoc`.

[pogo59]

Note that fixing the if issue resulted in a noticeable reduction in object size, as reported here. I'd expect the for statement fix to cause some reduction as well.

I did not find any similar issues with the while statement. I think that if/for/while are the only places that implicitly introduce lexical scopes, although C++ has become so huge it's hard to be certain.