Page MenuHomePhabricator

[DWARF] LICM should null out the debug loc of hoisted loop invariant instructions
ClosedPublic

Authored by wolfgangp on Jan 5 2017, 5:35 PM.

Details

Summary

Another instance of nulling out debug locations when instructions are moved to different basic blocks. This time it is LICM that is hoisting loop invariant code into the loop preheader. Not nulling out the debug locs can lead to poor stepping. Merging of debug locations does not apply here as the instructions is moved and not commoned.

One exception are call instructions. Even they can be hoisted out of loops, and if they are, we leave their debug locs in place in case the call is inlined later. This does not appear to be common, though.

Keeping in mind we'd like to do better than removing the debug loc eventually, I've added a FIXME comment.

Diff Detail

Repository
rL LLVM

Event Timeline

wolfgangp updated this revision to Diff 83327.Jan 5 2017, 5:35 PM
wolfgangp retitled this revision from to [DWARF] LICM should null out the debug loc of hoisted loop invariant instructions.
wolfgangp updated this object.
wolfgangp added a subscriber: llvm-commits.
aprantl added inline comments.Jan 6 2017, 8:36 AM
lib/Transforms/Scalar/LICM.cpp
774 ↗(On Diff #83327)

Whate happens when isa<DebugIntrinsicInst>?

aprantl accepted this revision.Jan 6 2017, 8:37 AM
aprantl edited edge metadata.

This is consistent with D27857.

lib/Transforms/Scalar/LICM.cpp
774 ↗(On Diff #83327)

Oh nevermind. They are calls.

This revision is now accepted and ready to land.Jan 6 2017, 8:37 AM
This revision was automatically updated to reflect the committed changes.
vsk added a subscriber: vsk.Apr 4 2019, 9:39 PM

As an alternative to removing the debug location, what about setting a line 0 location (with appropriate scope/inlinedAt info) to the hoisted instruction? It seems like that allows debuggers to give more helpful/specific backtraces. It also doesn't affect stepping (at least not in lldb, which collapses line 0 ranges). Example:

int load(int *p) { return *p; }

int licm(int seed, int n, int *p /* Points to garbage memory. */) {
  int hash = seed;
  for (int i = 0; i < n; ++i)
    hash ^= hash + (seed >> i) + load(p); // <- Crash occurs here.
  return hash;
}

With no location (current behavior, the crash looks like it occurs somewhere in 'main'):

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xdead)                                                          
  * frame #0: 0x0000000100000f73 licm`main at licm.cc:0:3 [opt]
    frame #1: 0x0000000100000f6c licm`main(argc=<unavailable>, argv=<unavailable>) at licm.cc:18 [opt]

With line 0 location:

  * frame #0: 0x0000000100000f73 licm`main [inlined] load(p=<unavailable>) at licm.cc:0:3 [opt]
...
(lldb) up
frame #1: 0x0000000100000f73 licm`main at licm.cc:9 [opt]
   6    int licm(int seed, int n, int *p /* Points to garbage memory. */) {
   7      int hash = seed;
   8      for (int i = 0; i < n; ++i)
-> 9        hash ^= hash + (seed >> i) + load(p); // <- Crash occurs here.

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 9:39 PM
Herald added a subscriber: asbirlea. · View Herald Transcript

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

It does indeed seem preferrable, since the hoisted instruction is currently attributed to the same line as the previous instruction, which is not what we want. I think our usage and understanding of 0 linenumbers has evolved since this change was made and we should examine all the places where we're removing the line number and see if setting it to 0 instead is better.

vsk added a comment.Apr 19 2019, 11:18 AM

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

It does indeed seem preferrable, since the hoisted instruction is currently attributed to the same line as the previous instruction, which is not what we want. I think our usage and understanding of 0 linenumbers has evolved since this change was made and we should examine all the places where we're removing the line number and see if setting it to 0 instead is better.

Sounds good!