[PATCH] D62650: [DebugInfo] DBG_VALUE instruction generated for loop counter is placed incorrectly.

Sun Jun 2 23:52:42 PDT 2019

avl marked 2 inline comments as done.
avl added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/wrong_debug_loc_after_regalloc.ll:45
+; CHECK-NOT: MOV
+; CHECK: DBG_VALUE $[[REG:[xw][0-9]+]], $noreg, !"i"
+; CHECK: MOV
----------------
aprantl wrote:
> what is the debug location attached to the MOV, the DBG_VALUE, and the instructions in between?
> If I understand the original code correctly, the intention was to not move a DBG_VALUE before the lexical scope of its variable starts.
log(before this patch) : 

512B    bb.2.for.body:

520B      renamable $x8 = MOVaddr target-flags(aarch64-page) @array, target-flags(aarch64-pageoff, aarch64-nc) @array, debug-location !54; test_debug_val.cpp:16:17
524B      renamable $w9 = MOVi32imm 56, debug-location !54; test_debug_val.cpp:16:17
          DBG_VALUE $x19, $noreg, !"i", !DIExpression(), debug-location !51; test_debug_val.cpp:0 line no:14
544B      STRWroX killed renamable $w9, killed renamable $x8, renamable $x19, 0, 1, debug-location !54 :: (store 4 into %ir.scevgep, !tbaa !38); test_debug_val.cpp:16:17

!51 = !DILocation(line: 0, scope: !35)
!52 = !DILocation(line: 14, column: 4, scope: !35)
!53 = !DILocation(line: 21, column: 4, scope: !14)
!54 = !DILocation(line: 16, column: 8, scope: !55)

short log : 

512B    bb.2.for.body:

520B      $x8 = MOVaddr scope: !55
524B      $w9 = MOVi32imm scope: !55
          DBG_VALUE $x19, $noreg, !"i" scope: !35 
544B      STRWroX killed renamable $w9 scope: !55

i.e. DBG_VALUE was inserted into enclosed scope.

The reason for this is that scope identified by MachineInstr : 

LiveDebugVariables.cpp:902

const InsnRange &Range : Scope->getRanges()  

using InsnRange = std::pair<const MachineInstr *, const MachineInstr *>;

At the moment when scope was created the very first instruction was STRWroX.
So it become start of range. Start slot index(with type of Slot_Block) for 
DBG_VALUE was trimmed to this instruction range. But after Register Allocator 
there were inserted new MOV instructions before STRWroX with the same scope as STRWroX. 
But Slot indices for DBG_VALUE was not updated and scope boundaries was not updated.

Actually Slot indices for DBG_VALUE looks correct without trimming : 512B,848B.
512B points to start of basic block:

512B    bb.2.for.body:

which match with definition of slot index of type Slot_Block:

// Basic block boundary.  Used for live ranges entering and leaving a
/// block without being live in the layout neighbor.  Also used as the
/// def slot of PHI-defs. 

i.e. 512B,848B actually match with lexical scope and does not require trimming. 

Also, trimming this slot index and leaving it of type Slot_Block is a error. Slot_Block should point to start of basic block.

I.setStartUnchecked(RStart);  <<< trimmed I.start() is a Slot_Block

This concrete problem could be solved by avoiding trimming for Slot_Block only. 

But it looks like trimming also is not required for the original problem test case : live-debug-variables.ll. With and without trimming it generates the same DBG_VALUE instructions : 

	  DBG_VALUE $ecx, $noreg, !"i4", !DIExpression(), debug-location !14; foo.c:3:35 @[ foo.c:8:10 ] line no:3
	  DBG_VALUE $ebx, $noreg, !"i4", !DIExpression(), debug-location !14; foo.c:3:35 @[ foo.c:8:10 ] line no:3

So in short : 

1. For the problem from D62650: SlotIndex was trimmed incorrectly. It does not require trimming and has incorrect value for Slot_Block after trimming.

2. Trimming for testcase from D35953 looks unnecessary. With and without trimming it generates the same DBG_VALUE instructions. 

Thus it looks like trimming could be removed at all.

================
Comment at: llvm/test/DebugInfo/X86/dbg-addr-dse.ll:35
   store i32 %x, i32* %x.addr, align 4
-  call void @llvm.dbg.addr(metadata i32* %x.addr, metadata !13, metadata !DIExpression()), !dbg !18
-  call void @escape(i32* %x.addr), !dbg !19
-  call void @llvm.dbg.value(metadata i32 1, metadata !13, metadata !DIExpression()), !dbg !20
-  store i32 1, i32* @global, align 4, !dbg !22
-  call void @llvm.dbg.addr(metadata i32* %x.addr, metadata !13, metadata !DIExpression()), !dbg !23
-  store i32 2, i32* %x.addr, align 4, !dbg !23
-  call void @escape(i32* %x.addr), !dbg !24
-  ret void, !dbg !25
+  call void @llvm.dbg.value(metadata i32* %x.addr, metadata !13, metadata !DIExpression(DW_OP_deref)), !dbg !14
+  call void @escape(i32* nonnull %x.addr) #3, !dbg !19
----------------
aprantl wrote:
> I don't think it's appropriate to change all dbg.addr instructions to dbg.values in a test that has dbg.addr in the name.
I noticed that current clang generates IR without @llvm.dbg.addr. 
That is why I regenerated the test. But I agree that this test should still 
check for @llvm.dbg.addr as it was created originally. So I would return it back ...

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62650/new/

https://reviews.llvm.org/D62650