Page MenuHomePhabricator

[LiveDebugValues] Close range for previous variable's location when deducing new variable's location
ClosedPublic

Authored by NikolaPrica on May 21 2019, 7:30 AM.

Details

Summary

When LiveDebugValues deduces new variable's location from spill, restore or register copy instruction it should close old variable's location. Otherwise we can have multiple block output locations for same variable. That could lead to inserting two DBG_VALUEs for same variable to the beginning of the successor block which results to ignoring of first DBG_VALUE.

  • fission-ranges.ll part which references llvm.dbg.value debuging location is changed in order to give function scope range to variables "b" and "e".

Diff Detail

Event Timeline

aprantl added inline comments.May 21 2019, 11:24 AM
lib/CodeGen/LiveDebugValues.cpp
436 ↗(On Diff #200501)

previous variable's -> this variable's previous

Does this do the right thing for two fragments of the same variable where one of them has a longer live range than the other? Or is this a non-issue here?

NikolaPrica marked 2 inline comments as done.May 22 2019, 2:34 AM
NikolaPrica added inline comments.
lib/CodeGen/LiveDebugValues.cpp
436 ↗(On Diff #200501)

I think it does the right thing. It does the same thing as transferDebugValue. Variable's locations are related to pair of DILocalVariable and DILocation thus this closes DILocalVariable location with matching DILocation.

NikolaPrica marked an inline comment as done.
  • Update comment
  • Changed label for test check-prefix

This looks good to me (I'll leave it for more comments).

lib/CodeGen/LiveDebugValues.cpp
436 ↗(On Diff #200501)

NB: as it stands, I don't believe LiveDebugValues handles fragments at all, which I've written up in https://bugs.llvm.org/show_bug.cgi?id=41979

(Thus this patch neither helps nor harms fragment handling).

aprantl added inline comments.May 22 2019, 8:52 AM
lib/CodeGen/LiveDebugValues.cpp
436 ↗(On Diff #200501)

Huh.

It does the same thing as transferDebugValue.

Looks like this is correct and LiveDebugValues can't propagate more than the last fragment of a variable. Unfortunate, but it does simplify the code. Carry on.

wolfgangp added inline comments.May 22 2019, 10:03 AM
test/DebugInfo/X86/fission-ranges.ll
16 ↗(On Diff #200682)

I would feel slightly better if we could make sure that these DBG_VALUEs occur in the correct blocks, or at least in different blocks. Perhaps with checks for block labels in between.

-Make sure DBG_VALUEs from the test case occur in different blocks.

dstenb added inline comments.May 23 2019, 8:32 AM
lib/CodeGen/LiveDebugValues.cpp
712 ↗(On Diff #200981)

Can this be VLS | OpenRanges.getVarLocs(); now?

test/DebugInfo/X86/fission-ranges.ll
14 ↗(On Diff #200981)

Perhaps it does not make a difference in this case, but I think it in general is preferred to use CHECK-LABEL directives for block label checks.

NikolaPrica marked an inline comment as done.May 23 2019, 9:14 AM
NikolaPrica added inline comments.
lib/CodeGen/LiveDebugValues.cpp
712 ↗(On Diff #200981)

Unfortunately that returns SparseBitVector object, we can't deduce from that whether changes were made.

probinson added inline comments.
test/DebugInfo/X86/fission-ranges.ll
14 ↗(On Diff #200981)

The LABEL suffix is widely misunderstood to mean "this is a label."
It would be better to match a longer string such as bb.6: to ensure it matches the label definition rather than a reference.

dstenb added inline comments.May 23 2019, 3:18 PM
test/DebugInfo/X86/fission-ranges.ll
14 ↗(On Diff #200981)

Right, but in cases like this it seems that label directives can help make FileCheck emit more pinpointed error messages (since the label strings are expected to be unique in the input)? It needs a longer string to match the definition rather than a reference though, as you point out.

# CHECK{,-LABEL}: bb.6:
# CHECK: DBG_VALUE $rsp
# CHECK{,-LABEL}: bb.7:
# CHECK: DBG_VALUE $rsp

bb.6:
bb.7:
DBG_VALUE $rsp

With normal check directives:

a.sh:3:10: error: CHECK: expected string not found in input
# CHECK: bb.7:
         ^
<stdin>:4:1: note: scanning from here

^

With label directives for the basic block labels:

a.sh:2:10: error: CHECK: expected string not found in input
# CHECK: DBG_VALUE $rsp
         ^
<stdin>:2:1: note: scanning from here
bb.7:
^
  • Address comments for test labels.
  • Perform conditional copy for new OutLocs set.
  • Rebase

Is this ready to land now?

aprantl accepted this revision.May 28 2019, 9:30 AM

I think so.

This revision is now accepted and ready to land.May 28 2019, 9:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 2:46 AM
Herald added a subscriber: hiraditya. · View Herald Transcript