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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

I'm reposting your reply with an easier to read format:

> 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 :
> 
> For the problem from D62650 <https://reviews.llvm.org/D62650>: SlotIndex was trimmed incorrectly. It does not require trimming and has incorrect value for Slot_Block after trimming.
>  Trimming for testcase from D35953 <https://reviews.llvm.org/D35953> looks unnecessary. With and without trimming it generates the same DBG_VALUE instructions.
>  Thus it looks like trimming could be removed at all.




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

https://reviews.llvm.org/D62650