[GlobalISel][DebugInfo] Remove debug info with zero line from constants inserted at entry block

[dzhidzhoev]

Summary

Emission of constants having DebugLoc with line 0 causes significant increase of debug_line section size for some source files.

To illustrate, we can compare section sizes of several files from llvm test-suite, built with SelectionDAG vs GlobalISel, on Aarch64 (macOS), using -O0 optimization level:

Source path	SDAG text sz	GISel text sz	SDAG debug_line sz	GISel debug_line sz
SingleSource/Regression/C/gcc-c-torture/execute/strlen-2.c	15320	660	14872	6340
SingleSource/Regression/C/gcc-c-torture/execute/20040629-1.c	33640	26300	2812	6693
SingleSource/Benchmarks/Misc/flops-4.c	1428	1196	594	1008
MultiSource/Benchmarks/MiBench/consumer-typeset/z31.c	2716	964	809	903
MultiSource/Benchmarks/Prolangs-C/gnugo/showinst.c	2534	2502	189	573

For instance, here is a fragment of flops-4.c.o debug line section dump

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000    174      0      1   0             0  is_stmt
0x0000000000000010      0      0      1   0             0
0x0000000000000018    185      4      1   0             0  is_stmt prologue_end
0x000000000000001c      0      0      1   0             0
0x0000000000000024    186      4      1   0             0  is_stmt
0x000000000000002c    189     10      1   0             0  is_stmt
0x0000000000000030      0      0      1   0             0
0x0000000000000038    207     11      1   0             0  is_stmt
0x0000000000000044    208     11      1   0             0  is_stmt
0x0000000000000048      0      0      1   0             0
0x0000000000000058    210     10      1   0             0  is_stmt
0x000000000000005c      0      0      1   0             0
0x0000000000000060    211     10      1   0             0  is_stmt
0x0000000000000064      0      0      1   0             0
0x000000000000006c    212     10      1   0             0  is_stmt
0x0000000000000070      0      0      1   0             0
0x000000000000007c    213     10      1   0             0  is_stmt
0x0000000000000080      0      0      1   0             0
0x0000000000000088    214     10      1   0             0  is_stmt
0x000000000000008c      0      0      1   0             0
0x0000000000000094    215     10      1   0             0  is_stmt

Lot of zero lines are produced by constants (global values) having DebugLoc with line 0.
It seems that they're not significant for debugging experience.

With the commit applied, total size of debug_line sections of llvm shared libraries has reduced by 2.5%.
Change of debug line section size of files listed above:

Source path	GISel debug_line sz	Patch debug_line sz
SingleSource/Regression/C/gcc-c-torture/execute/strlen-2.c	6340	1465
SingleSource/Regression/C/gcc-c-torture/execute/20040629-1.c	6693	3782
SingleSource/Benchmarks/Misc/flops-4.c	1008	609
MultiSource/Benchmarks/MiBench/consumer-typeset/z31.c	903	841
MultiSource/Benchmarks/Prolangs-C/gnugo/showinst.c	573	190

[arsenm]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

158

Should also specifically check for constants?

[dzhidzhoev]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

158

It would be nice, but various of machine instructions can be emitted from llvm::ConstantExpr.

To split instructions originated from constants and non-constant ones in this check, DILocationVerifier should be extended with state variable indicating that we're emitting instructions within translate(Constant, Register) call.
Since IRTranslator::translate(Constant, Register) call could be recursive, we need to add nesting counter + member functions to increase and decrease it.
Initially I considered this as an excessive complication of DILocationVerifier, but I can add it if it is needed.

[dblaikie]

I'm on board with the general idea, FWIW.

[dzhidzhoev]

Added const check to DILocationVerifier

[aemerson]

+ @aprantl

[probinson]

I'd have thought AsmPrinter would deal with this, I remember adding code to look at the sequence of emitted .loc directives and handle line 0 specially. But if this is helping, it seems reasonable. It would be nice to experiment with actually debugging the generated code and see if the quality of the debugging experience is better/worse/unchanged. Maybe @jmorse your team can take a quick look?

[dzhidzhoev]

Ping

[dzhidzhoev]

Ping

[probinson]

See also https://github.com/llvm/llvm-project/issues/56370 which wants to introduce more line-0 entries. There is clearly some tension going one between two goals, and I'd like to understand that better.

[dblaikie]

2.5% in .debug_line, which isn't a large part of total debug info size seems kind of small (it'd be good to have % of total .debug_* section sizes, and of the objects/binary too, to put this in perspective).

Presumably the zero line on these was added for a reason to begin with (though maybe with hindsight we decide that reason is insufficient/doesn't apply here/etc) - could you check version history/see if there's some back story to this? (perhaps it was added incidentally while addressing some other issue, perhaps not)

One thing that is important is that line zero is used at the start of basic blocks if the instructions don't otherwise have a location - because otherwise sample profiles can be mislead (because the line location from the previous basic block will cover the unassigned locations and then a sample could incorrectly conclude that a source line was executed when it was not). I don't know if that's related to this case - doesn't /look/ like it, but figured I'd mention it jus tin case.

[aemerson]

I originally made constants use line 0 because on D63286 Adrian said that if we don't emit 0, the constants end up being interpreted as being part of the prologue, which they're technically not.

[dblaikie]

In D127488#3636496, @aemerson wrote:

I originally made constants use line 0 because on D63286 Adrian said that if we don't emit 0, the constants end up being interpreted as being part of the prologue, which they're technically not.

Though it doesn't look like @aprantl thought this would be bad - just that it would happen. Do you/we/anyone have any particular reason to believe it would be bad/problematic if thees instructions became part of the prologue?

[aemerson]

In D127488#3636630, @dblaikie wrote:

In D127488#3636496, @aemerson wrote:

I originally made constants use line 0 because on D63286 Adrian said that if we don't emit 0, the constants end up being interpreted as being part of the prologue, which they're technically not.

Though it doesn't look like @aprantl thought this would be bad - just that it would happen. Do you/we/anyone have any particular reason to believe it would be bad/problematic if thees instructions became part of the prologue?

I don't know of any reason why it would be bad, but admittedly I'm not an expert in this area. The original intention of using zero was to avoid unnecessary unintended effects. If we do have an issue here with the debug info sizes then I'm ok with removing them.

[dzhidzhoev]

In D127488#3633395, @dblaikie wrote:

Presumably the zero line on these was added for a reason to begin with (though maybe with hindsight we decide that reason is insufficient/doesn't apply here/etc) - could you check version history/see if there's some back story to this? (perhaps it was added incidentally while addressing some other issue, perhaps not)

Previously constants generated at entry block had taken the same debug location as instruction being translated (https://reviews.llvm.org/D63286). It might cause jumpy debugger behavior.

In D127488#3633395, @dblaikie wrote:

One thing that is important is that line zero is used at the start of basic blocks if the instructions don't otherwise have a location - because otherwise sample profiles can be mislead (because the line location from the previous basic block will cover the unassigned locations and then a sample could incorrectly conclude that a source line was executed when it was not). I don't know if that's related to this case - doesn't /look/ like it, but figured I'd mention it jus tin case.

If an instruction at the start of basic block has no debug location, AsmPrinter emits zero line location for it.

[dzhidzhoev]

In D127488#3631137, @probinson wrote:

See also https://github.com/llvm/llvm-project/issues/56370 which wants to introduce more line-0 entries. There is clearly some tension going one between two goals, and I'd like to understand that better.

Since this patch affects constant generation, such problem could appear after Localizer pass (for example, if constant gets placed after call instruction). Here is the commit intended to fix that https://reviews.llvm.org/D128192.

[dzhidzhoev]

In D127488#3633395, @dblaikie wrote:

2.5% in .debug_line, which isn't a large part of total debug info size seems kind of small (it'd be good to have % of total .debug_* section sizes, and of the objects/binary too, to put this in perspective).

Total size of all debug sections of llvm shared libraries has reduced by 0.3%. Total size of debug_line sections over llvm object files has reduced by 2%.
Total size of all debug sections of llvm object files has reduced by 0,1%.
Total size of all debug sections of test-suite object files has reduced by 1.5%. Total size of debug_line sections over test-suite object files has reduced by 7.5%.

[dblaikie]

Does seem like a relatively small win - is there a particular situation where you came across this/where it was important for your usage? (it'd be interesting to know of novel use cases that might motivate changes like this)

Why are the other changes needed here - the previous change set the location to 0, the new change sets it to nothing, so why are the other changes (the scope devices, etc) needed? Are they representative of bugs/issues in the previous (line 0) solution?

[dzhidzhoev]

In D127488#3639858, @dblaikie wrote:

Does seem like a relatively small win - is there a particular situation where you came across this/where it was important for your usage? (it'd be interesting to know of novel use cases that might motivate changes like this)

It was noticed that almost each instruction accessing global value is marked with zero line in debug_line (even in simple case like that https://godbolt.org/z/W6shEoh95 at lines 23, 27).

Then it was discovered that for some source files we had significant increase of debug_* section size compared to SelectionDAG because of that, even considering difference of output code size of different backends. For these files we can reduce debug_* sections size by reducing debug_line size - like for files listed in review message:

Source path	SDag debug_* sz	GISel debug_* sz	GISel+patch debug_* sz
SingleSource/Regression/C/gcc-c-torture/execute/strlen-2.c	3116	8796	3926
SingleSource/Regression/C/gcc-c-torture/execute/20040629-1.c	16273	20154	17248
SingleSource/Benchmarks/Misc/flops-4.c	4574	4989	4595
MultiSource/Benchmarks/MiBench/consumer-typeset/z31.c	3198	5161	4647
MultiSource/Benchmarks/Prolangs-C/gnugo/showinst.c	1472	1856	1478

[dzhidzhoev]

In D127488#3639858, @dblaikie wrote:

Why are the other changes needed here - the previous change set the location to 0, the new change sets it to nothing, so why are the other changes (the scope devices, etc) needed? Are they representative of bugs/issues in the previous (line 0) solution?

DILocationVerifier basically doesn't allow to build instruction without debug location, if the instruction being translated has the location. We change DILocationVerifier, allowing to translate instructions with debug location into instructions without it, if instructions being emitted are inserted into "constant block". In IRTranslator::translate(Constant*, ...), DILocationVerifier is notified that instructions being translated into entry block belong to "constant block". Since IRTranslator::translate can be called recursively, DILocationVerifier has to track nested constant blocks by using a counter. Scope device is used to make it implicitly.

[dblaikie]

In D127488#3646698, @dzhidzhoev wrote:

In D127488#3639858, @dblaikie wrote:

Why are the other changes needed here - the previous change set the location to 0, the new change sets it to nothing, so why are the other changes (the scope devices, etc) needed? Are they representative of bugs/issues in the previous (line 0) solution?

DILocationVerifier basically doesn't allow to build instruction without debug location, if the instruction being translated has the location. We change DILocationVerifier, allowing to translate instructions with debug location into instructions without it, if instructions being emitted are inserted into "constant block". In IRTranslator::translate(Constant*, ...), DILocationVerifier is notified that instructions being translated into entry block belong to "constant block". Since IRTranslator::translate can be called recursively, DILocationVerifier has to track nested constant blocks by using a counter. Scope device is used to make it implicitly.

Hmm, not sure I'm following why the solution to this is more involved than the solution to allowing line 0? (could the check for line 0 be changed to allow no-line & be done at that point?)

[dzhidzhoev]

In D127488#3653616, @dblaikie wrote:

In D127488#3646698, @dzhidzhoev wrote:

In D127488#3639858, @dblaikie wrote:

Why are the other changes needed here - the previous change set the location to 0, the new change sets it to nothing, so why are the other changes (the scope devices, etc) needed? Are they representative of bugs/issues in the previous (line 0) solution?

DILocationVerifier basically doesn't allow to build instruction without debug location, if the instruction being translated has the location. We change DILocationVerifier, allowing to translate instructions with debug location into instructions without it, if instructions being emitted are inserted into "constant block". In IRTranslator::translate(Constant*, ...), DILocationVerifier is notified that instructions being translated into entry block belong to "constant block". Since IRTranslator::translate can be called recursively, DILocationVerifier has to track nested constant blocks by using a counter. Scope device is used to make it implicitly.

Hmm, not sure I'm following why the solution to this is more involved than the solution to allowing line 0? (could the check for line 0 be changed to allow no-line & be done at that point?)

Initially it was like that. Addressing this comment https://reviews.llvm.org/D127488#3573315, I have added more sophisticated check. Should I revert it to previous version https://reviews.llvm.org/D127488?id=435872 ?

[dblaikie]

In D127488#3654501, @dzhidzhoev wrote:

In D127488#3653616, @dblaikie wrote:

In D127488#3646698, @dzhidzhoev wrote:

In D127488#3639858, @dblaikie wrote:

Why are the other changes needed here - the previous change set the location to 0, the new change sets it to nothing, so why are the other changes (the scope devices, etc) needed? Are they representative of bugs/issues in the previous (line 0) solution?

DILocationVerifier basically doesn't allow to build instruction without debug location, if the instruction being translated has the location. We change DILocationVerifier, allowing to translate instructions with debug location into instructions without it, if instructions being emitted are inserted into "constant block". In IRTranslator::translate(Constant*, ...), DILocationVerifier is notified that instructions being translated into entry block belong to "constant block". Since IRTranslator::translate can be called recursively, DILocationVerifier has to track nested constant blocks by using a counter. Scope device is used to make it implicitly.

Hmm, not sure I'm following why the solution to this is more involved than the solution to allowing line 0? (could the check for line 0 be changed to allow no-line & be done at that point?)

Initially it was like that. Addressing this comment https://reviews.llvm.org/D127488#3573315, I have added more sophisticated check. Should I revert it to previous version https://reviews.llvm.org/D127488?id=435872 ?

@arsenm what're your thoughts here? I don't feel very strongly either way, but I didn't understand the added complexity.

[dzhidzhoev]

Ping

[arsenm]

In D127488#3656388, @dblaikie wrote:

Initially it was like that. Addressing this comment https://reviews.llvm.org/D127488#3573315, I have added more sophisticated check. Should I revert it to previous version https://reviews.llvm.org/D127488?id=435872 ?

@arsenm what're your thoughts here? I don't feel very strongly either way, but I didn't understand the added complexity.

I don't have a particularly strong opinion. I don't think it's worth adding a lot of extra complexity just for the benefit of an assert

[dzhidzhoev]

Reverted to previous diff, addressing @dblaikie and @arsenm comments

[dzhidzhoev]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

158

Could you advise how to check for constants in less complicated way? Currently I tend towards that we could accept this assert to be less exact for entry block instructions.

[dblaikie]

In D127488#3670446, @arsenm wrote:

In D127488#3656388, @dblaikie wrote:

Initially it was like that. Addressing this comment https://reviews.llvm.org/D127488#3573315, I have added more sophisticated check. Should I revert it to previous version https://reviews.llvm.org/D127488?id=435872 ?

@arsenm what're your thoughts here? I don't feel very strongly either way, but I didn't understand the added complexity.

I don't have a particularly strong opinion. I don't think it's worth adding a lot of extra complexity just for the benefit of an assert

OK, sounds good then.

[dzhidzhoev]

Rebased

[dzhidzhoev]

Rebased (on accepted version)

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

public:
   void createdInstr(MachineInstr &MI) override {
     assert(getCurrentInst() && "Inserted instruction without a current MI");
 
     // Only print the check message if we're actually checking it.
 #ifndef NDEBUG
     LLVM_DEBUG(dbgs() << "Checking DILocation from " << *CurrInst
                       << " was copied to " << MI);
 #endif
-    // We allow insts in the entry block to have a debug loc line of 0 because
+    // We allow insts in the entry block to have no debug loc because
     // they could have originated from constants, and we don't want a jumpy
     // debug experience.
     assert((CurrInst->getDebugLoc() == MI.getDebugLoc() ||
-            MI.getDebugLoc().getLine() == 0) &&
+            (MI.getParent()->isEntryBlock() && !MI.getDebugLoc())) &&
            "Line info was not transferred to all instructions");
   }
 };
 } // namespace
 #endif // ifndef NDEBUG
 
 
 void IRTranslator::getAnalysisUsage(AnalysisUsage &AU) const {

 #include "llvm/IR/Instruction.def"
   default:
     return false;
   }
 }
 
 bool IRTranslator::translate(const Constant &C, Register Reg) {
   // We only emit constants into the entry block from here. To prevent jumpy
-  // debug behaviour set the line to 0.
+  // debug behaviour remove debug line.
   if (auto CurrInstDL = CurBuilder->getDL())
-    EntryBuilder->setDebugLoc(DILocation::get(C.getContext(), 0, 0,
-                                              CurrInstDL.getScope(),
-                                              CurrInstDL.getInlinedAt()));
+    EntryBuilder->setDebugLoc(DebugLoc());
 
   if (auto CI = dyn_cast<ConstantInt>(&C))
     EntryBuilder->buildConstant(Reg, *CI);
   else if (auto CF = dyn_cast<ConstantFP>(&C))
     EntryBuilder->buildFConstant(Reg, *CF);
   else if (isa<UndefValue>(C))
     EntryBuilder->buildUndef(Reg);
   else if (isa<ConstantPointerNull>(C))

llvm/test/CodeGen/AArch64/GlobalISel/combine-shift-of-shifted-dbg-value-fallback.ll

 ; CHECK:       .Lfunc_begin0:
 ; CHECK-NEXT:    .file 1 "/" "tmp.ll"
 ; CHECK-NEXT:    .loc 1 1 0 // tmp.ll:1:0
 ; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  // %bb.0: // %bb
 ; CHECK-NEXT:    //DEBUG_VALUE: baz:3 <- undef
 ; CHECK-NEXT:    .loc 1 4 1 prologue_end // tmp.ll:4:1
 ; CHECK-NEXT:    lsl x8, x0, #4
-; CHECK-NEXT:    .loc 1 0 0 is_stmt 0 // tmp.ll:0:0
 ; CHECK-NEXT:    adrp x9, global+202752
 ; CHECK-NEXT:    add x9, x9, :lo12:global+202752
-; CHECK-NEXT:    .loc 1 4 1 // tmp.ll:4:1
 ; CHECK-NEXT:    and x8, x8, #0x1ff0
-; CHECK-NEXT:    .loc 1 5 1 is_stmt 1 // tmp.ll:5:1
+; CHECK-NEXT:    .loc 1 5 1 // tmp.ll:5:1
 ; CHECK-NEXT:    str xzr, [x9, x8]
 ; CHECK-NEXT:    .loc 1 6 1 // tmp.ll:6:1
 ; CHECK-NEXT:    ret
 ; CHECK-NEXT:  .Ltmp0:
 bb:
   %tmp = ptrtoint i8* %arg to i64, !dbg !14
   %tmp1 = shl i64 %tmp, 1, !dbg !15
   %tmp2 = and i64 %tmp1, 1022, !dbg !16

llvm/test/CodeGen/AArch64/GlobalISel/constant-dbg-loc.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc -mtriple aarch64 -O0 -stop-after=irtranslator -global-isel -verify-machineinstrs %s -o - 2>&1 | FileCheck --match-full-lines %s
 target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
 target triple = "arm64-apple-ios5.0.0"
 
 @var1 = common global i32 0, align 4, !dbg !0
 @var2 = common global i32 0, align 4, !dbg !6
 
-; We check here that the G_GLOBAL_VALUE has a debug loc with line 0.
+; We check here that the G_GLOBAL_VALUE has no debug loc.
 define i32 @main() #0 !dbg !14 {
   ; CHECK-LABEL: name: main
   ; CHECK: bb.1.entry:
   ; CHECK:   successors: %bb.2(0x40000000), %bb.3(0x40000000)
   ; CHECK:   [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0
-  ; CHECK:   [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var1, debug-location !DILocation(line: 0, scope: !18)
+  ; CHECK:   [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var1
   ; CHECK:   [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 1
   ; CHECK:   [[C2:%[0-9]+]]:_(s32) = G_CONSTANT i32 2
-  ; CHECK:   [[GV1:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var2, debug-location !DILocation(line: 0, scope: !22)
+  ; CHECK:   [[GV1:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @var2
   ; CHECK:   [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0.retval
   ; CHECK:   G_STORE [[C]](s32), [[FRAME_INDEX]](p0) :: (store (s32) into %ir.retval)
   ; CHECK:   [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[GV]](p0), debug-location !17 :: (dereferenceable load (s32) from @var1)
   ; CHECK:   [[ICMP:%[0-9]+]]:_(s1) = G_ICMP intpred(eq), [[LOAD]](s32), [[C1]], debug-location !19
   ; CHECK:   G_BRCOND [[ICMP]](s1), %bb.2, debug-location !20
   ; CHECK:   G_BR %bb.3, debug-location !20
   ; CHECK: bb.2.if.then:
   ; CHECK:   successors: %bb.3(0x80000000)