Page MenuHomePhabricator

[DebugInfo] Honour variable fragments in LiveDebugValues
ClosedPublic

Authored by jmorse on Jun 5 2019, 7:38 AM.

Details

Summary

This patch makes the LiveDebugValues pass consider fragments when propagating DBG_VALUE insts between blocks, fixing PR41979. Fragment info for a variable location is added to the open-ranges key, which allows distinct fragments to be tracked separately. To handle overlapping fragments things become slightly funkier. To avoid excessive searching for overlaps in the data-flow part of LiveDebugValues, this patch:

  • Pre-computes pairings of fragments that overlap, for each DILocalVariable
  • During data-flow, whenever something happens that causes an open range to be terminated (via erase), any fragments pre-determined to overlap are also terminated.

The effect of which is that when encountering a DBG_VALUE fragment that overlaps others, the overlapped fragments do not get propagated to other blocks. We still rely on later location-list building to correctly handle overlapping fragments within blocks. Performance-wise, there are other options for implementing fast searching for overlapping fragments, but this seems alright.

Looking at the patch: DIExpression fragments grow a DenseMapInfo record so that fragments can be used in indexes. The "DebugVariable" class in LiveDebugValues loses its base of a std::pair: now that we have three things to index, we can't use std::pair's DenseMapInfo, so there's no point subclassing it. OpenRangesSet grows a reference to the precomputed map of overlapping fragments, and the LiveDebugValues class grows a method to build that map on the first run through the machine function. Finally, OpenRangesSet::erase now tries to close any overlapping fragments of the one being closed.

In terms of variable coverage, across a clang-3.4 build, applying this patch causes a grand total of _one_ new variable to have a location [applause]. Hacking llvm-dwarfdump to gather statistics on how _much_ (in bytes) of a variable is described when it has a location, for DW_TAG_structure_type's, we increase coverage from 98.9% of all possible bytes to 99.3%. It kind of sounds like missing fragment support wasn't causing too much of a loss in coverage for clang at least, however alternative coding styles that involve lots of classes that get broken up by SROA will have been worse hit.

In terms of performance, an average of three clang-3.4 builds with/without this patch shows a 0.8% increase in compile time. Looking closer at X86ISelLowering.cpp (a reference big-lump-of-c++), LiveDebugValues takes 0.32 seconds to run with this patch, versus 0.3 seconds without, out of a total of ~14 seconds total compilation time. IMHO, these increases in compile time are tolerable, seeing how this patch is an increase in functionality.

Diff Detail

Event Timeline

jmorse created this revision.Jun 5 2019, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 7:38 AM
jmorse updated this revision to Diff 203151.Jun 5 2019, 7:41 AM

Ninja-edit: improve the comment at the top of the added regression test.

dstenb added inline comments.Jun 7 2019, 7:09 AM
lib/CodeGen/LiveDebugValues.cpp
892

Is this guaranteed due to how SROA and other IR passes behave?

Would it cost a lot to drop this assumption and handle mixes of fragmented and non-fragmented debug values?

jmorse marked an inline comment as done.Jun 7 2019, 8:08 AM
jmorse added inline comments.
lib/CodeGen/LiveDebugValues.cpp
892

It's my belief that that's how SROA works, but I don't think it's a documented guarantee. SROA::splitAlloca certainly seems to completely rewrite dbg.declare's into ones that all use fragments. The verifier doesn't attempt to enforce this rule though. Most of this belief is due to running into this assertion [0] several times, which fires if you get a mix of fragment/non-fragment variable locations in a DebugLocEntry.

I've run a quick test in this function, maintaining a static set of Variable/InlinedAt pairs that have been seen without fragments, and asserting that they're never seen _with_ fragments. This doesn't fire on a build of clang-3.4, which gives me some confidence the guarantee exists.

If it turns out a mixture needs to be supported, it'd be easy to interpret the nonfragmented debug values as covering the whole variable, it should Just Work (TM).

[0] https://github.com/llvm/llvm-project/blob/2028ae975c6aa65df3d89c10c95bf9b7baa5e0ab/llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h#L131

dstenb added inline comments.Jun 7 2019, 8:34 AM
lib/CodeGen/LiveDebugValues.cpp
892

Most of this belief is due to running into this assertion [0] several times, which fires if you get a mix of fragment/non-fragment variable locations in a DebugLocEntry.

Oh, under which circumstances have you hit that assert? (Or was that supposed to say not?)

At least on trunk DbgEnitityHistoryCalculator should close all open fragmented debug values when encountering a non-fragment debug value, and vice versa. DwarfDebug::buildLocationList() should respect those ending indices, so it "should" not be possible to land in that assertion even if the MIR contains a mix of fragmented and non-fragmented locations.

(I noticed that the commit in [0] is a bit older, but there I think the same logic holds since we in DwarfDebug::buildLocationList() used fragmentsOverlap() to close all preceding locations?)

jmorse marked an inline comment as done.Jun 7 2019, 10:00 AM
jmorse added inline comments.
lib/CodeGen/LiveDebugValues.cpp
892

Oh, under which circumstances have you hit that assert? (Or was that supposed to say not?)

Ah, whoops, by that I meant that when I made mistakes in other lines of development, that assertion has occasionally fired when I accidentally dropped a fragment. It's never fired for me with a clean checkout, to be clear.

It's my belief that that's how SROA works, but I don't think it's a documented guarantee. SROA::splitAlloca certainly seems to completely rewrite dbg.declare's into ones that all use fragments. The verifier doesn't attempt to enforce this rule though.

Please note that SROA is not the only producer of DW_OP_LLVM_fragments. The Swift frontend, for example, can produce fragments from the point where LLVM IR is generated long before LLVM's SROA is invoked. If a condition is not enforced by the Verifier it is not generally safe to rely on it because LLVM IR is generated by many producers, not just Clang.

include/llvm/IR/DebugInfoMetadata.h
2579

(Frag.SizeInBits & 0xffff) << 16 | (Frag.OffsetInBits & 0xffff) should be a practically perfect hash function.

jmorse updated this revision to Diff 204082.Jun 11 2019, 9:18 AM

Add a "default" fragment for DBG_VALUEs that don't have one, which covers all possible fragments. This causes non-fragment DBG_VALUEs to overlap all fragmented DBG_VALUEs.

jmorse marked 3 inline comments as done.Jun 11 2019, 9:24 AM

Please note that SROA is not the only producer of DW_OP_LLVM_fragments. The Swift frontend, for example, can produce fragments from the point where LLVM IR is generated long before LLVM's SROA is invoked. If a condition is not enforced by the Verifier it is not generally safe to rely on it because LLVM IR is generated by many producers, not just Clang.

Cool, I've implemented the alternative of having non-fragmented DBG_VALUEs overlap all fragmented ones, and added an additional function to the tests to check that those overlaps are recognised.

I feel like this should be rejected by the verifier, but I'm not confident enough to add such a check, and it's not a great burden to support anyway.

Stylistic nit: unless this will push a lot of lines > 80 columns and kill readability, I would prefer Fragment over Frag.

lib/CodeGen/LiveDebugValues.cpp
473

does OverlappingFrags.find({Var.getVar(), ThisFrag}); work, too?

478

Perhaps

LiveDebugValues::OptFragInfo FragHolder;
if (!DebugVariable::isFragDefault(Fragment))
  FragHolder = LiveDebugValues::OptFragInfo(Fragment);
481

does DoErase({Var.getVar(), FragHolder, Var.getInlinedAt()}) work?

887

FragmentMap?

919

Should we have a wrapper for the == 0 case that contains the word overlap in the name?

jmorse updated this revision to Diff 204101.Jun 11 2019, 10:34 AM

Switch instances of "frag" to "fragment" -- this only forces more wrapping on a few lines and is well worth it. I've also scattered a few more initializer-list replacements for make_pair, and added a fragmentsOverlap helper to DIExpression.

jmorse marked 5 inline comments as done.Jun 11 2019, 10:34 AM

I think this is starting to look good!

lib/CodeGen/LiveDebugValues.cpp
148

llvm::Optional has getValueOr() could we use that instead of this function?

910

Instead of having all these references to .second (and only .second) in this function, would it make sense to store a reference to .second in the auto variables and give them a different name?

i.e., auto CurrentFragments = IsInOLapMap.first.second;

jmorse updated this revision to Diff 204257.Jun 12 2019, 4:31 AM

Use getValueOr when extracting the value of a fragment with a default; assign some better variable names to container references in accumulateFragmentMap

jmorse marked 3 inline comments as done.Jun 12 2019, 4:33 AM
jmorse added inline comments.
lib/CodeGen/LiveDebugValues.cpp
910

I've added the ThisFragmentsOverlaps and AllSeenFragments references -- hopefully this makes the loop clearer, showing which parts are previously-seen fragments and which is the current (unseen) fragment.

aprantl accepted this revision.Jun 12 2019, 8:32 AM

One last nit inline. Thanks!

lib/CodeGen/LiveDebugValues.cpp
1099

I think this should be sunk into process().

This revision is now accepted and ready to land.Jun 12 2019, 8:32 AM
This revision was automatically updated to reflect the committed changes.
jmorse marked an inline comment as done.

Blast, missed the final nit, will patch that in a second.