[PATCH] D59272: [DebugInfo] Select debug intrinsic line-numbers more carefully when promoting dbg.declare

Tue Mar 12 13:20:34 PDT 2019

jmorse created this revision.
jmorse added reviewers: aprantl, vsk, bjope, gbedwell.
Herald added subscribers: llvm-commits, jdoerfert, kristof.beyls, javed.absar.
Herald added a project: LLVM.

This patch changes the line-numbers given to dbg.value intrinsics when an alloca/dbg.declare is promoted to an SSA register. Currently we use the original line number of the dbg.declare, i.e. the variable declaration. I believe there is risk that well-meaning optimisations will accidentally acquire these line numbers from dbg.values, artificially introducing line-number changes that needlessly step back to the variable declaration. I wasn't able to immediately produce an input that demonstrated this, below is an explanation of the patch, followed by some evidence to demonstrate this is a desirable change.

As far as I understand it, the actual line number that a dbg.value gets is meaningless (although the scope / inlining information is important). The issue is that the line number could leak into surrounding instructions during optimisation.

~

What I've changed: In the three ConvertDebugDeclareToDebugValue functions changed, previously we assigned each created dbg.value intrinsic the DebugLoc from the dbg.declare it originates from. Now, if the store or PHI node that the dbg.value corresponds to has a line number in the same scope, the 'getDebugValueLoc' helper picks that, otherwise an unknown-location line number is used.

The added test tests three scenarios (explained in the file) of no-location, suitable location, and inlined location.

The formal_parameter.ll test that changes, is checking that the dbg.values for a lowered dbg.declare have the right scope. They now get different DILocations, so I've added extra CHECK lines for their scope fields.

~

To try and get some perspective on whether this is a good change to make, I built clang-3.4 with and without this patch and diff'd the line tables. Roughly 900 entries were altered, in some whole lines went away, in others they were replaced by unknown-locations.

Example 1: in "llvm::GPRGetRawAllocationOrder(llvm::MachineFunction const&)", some tablegen'd function for the ARM backend, this materializeFrameBaseRegister function [0] is inlined. Without the patch, the line table progression is:

  Line 577 (conditional test)
  Constructor for a DebugLoc
  Line 578 (conditional assignment to DL)
  This [1] variable declaration in BuildMI
  MachineBasicBlock::getParent
  MachineFunction::getTarget
  MachineFunction::getRegInfo
  Line 582

Most of that makes sense for lines 577 to 582 of the function [0], except for the early step into BuildMI [1], which seems to be part of the BuildMI call on line 586. That particular linetable entry disappears with this patch. It's unclear to me how it got there in the first place, but I don't believe it's helpful.

Example 2, in "llvm::ARMFrameLowering::hasFP", this [2] inlined call to emitRegPlusImmediate steps through the evaluation of the arguments, then to the declaration of one of the formal parameters in the target function [3], then back to evaluating the arguments again. This extra spurious step into the destination function isn't helpful IMO.

While I don't know where exactly instructions pick up line numbers from dbg.values, I believe these differences demonstrate that they do, and so it should be avoided.

~

tl;dr, if we don't spread the line numbers of variable declarations throughout instructions in the function, we'll have less jumpy line tables.

Credit to @CarlosAlbertoEnciso who found this originally but didn't get around to filing a patch.

[0] https://github.com/llvm-mirror/llvm/blob/release_34/lib/Target/ARM/ARMBaseRegisterInfo.cpp#L577
[1] https://github.com/llvm-mirror/llvm/blob/release_34/include/llvm/CodeGen/MachineInstrBuilder.h#L245
[2] https://github.com/llvm-mirror/llvm/blob/release_34/lib/Target/ARM/ARMFrameLowering.cpp#L285
[3] https://github.com/llvm-mirror/llvm/blob/release_34/lib/Target/ARM/ARMFrameLowering.cpp#L108

Repository:
  rL LLVM

https://reviews.llvm.org/D59272

Files:
  lib/Transforms/Utils/Local.cpp
  test/DebugInfo/Generic/dbg-value-lower-linenos.ll
  test/DebugInfo/X86/formal_parameter.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59272.190307.patch
Type: text/x-patch
Size: 8132 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190312/1802aa3d/attachment.bin>