Page MenuHomePhabricator

[DebugInfo] Create merged locations for instructions other than calls
ClosedPublic

Authored by vsk on Apr 6 2018, 6:02 PM.

Details

Summary

This lifts a restriction on DILocation::getMergedLocation(), allowing it
to create merged locations for instructions other than calls.

Instruction::applyMergedLocation() now defaults to creating merged
locations for all instructions.

The default behavior of getMergedLocation() is unchanged: callers which
invoke it directly are unaffected.

This change will enable a follow-up Mem2Reg fix which improves crash
reporting.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Apr 6 2018, 6:02 PM

Do you have the stats on this change in terms of line table size increase?

Which callers use Instruction::applyMergedLocation versus DILocation::getMergedLocation directly?

lib/IR/DebugInfo.cpp
678–679 ↗(On Diff #141457)

Maybe have the API use an enum instead of a bool (so callers pass the enum & are inherently readable), rather than relying on callers to name the parameter for readability?

rnk added a subscriber: rnk.Apr 10 2018, 2:45 PM
rnk added inline comments.
lib/IR/DebugInfoMetadata.cpp
73 ↗(On Diff #141457)

Who are the primary callers that pass false here? mem2reg, I guess? If the bool is short-lived, sounds good.

vsk marked an inline comment as done.Apr 10 2018, 2:58 PM

Do you have the stats on this change in terms of line table size increase?

I looked at a stage2 Release build of clang (-O3 -g), and there's about a 0.6% size increase:

$ size -m ~/Desktop/baseline.clang.dSYM/Contents/Resources/DWARF/clang | grep debug_line 
        Section __debug_line: 34433811
$ size -m ~/Desktop/after-diloc-patch.clang.dSYM/Contents/Resources/DWARF/clang | grep debug_line 
        Section __debug_line: 34646430

Which callers use Instruction::applyMergedLocation versus DILocation::getMergedLocation directly?

Users of applyMergedLocation (all of these use GenerateLocation = true, now):

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
lib/Transforms/InstCombine/InstCombinePHI.cpp
lib/Transforms/Utils/PromoteMemoryToRegister.cpp
lib/Transforms/Utils/SimplifyCFG.cpp

Users of DILocation::getMergedLocation (all of these use GenerateLocation = false):

lib/CodeGen/BranchFolding.cpp
lib/CodeGen/MachineBasicBlock.cpp
lib/CodeGen/MachineSink.cpp
lib/Transforms/Scalar/ConstantHoisting.cpp

vsk updated this revision to Diff 141919.Apr 10 2018, 2:59 PM

I'm fine with this change since the result is more accurate than before (where the instruction would inherit whatever the previous instruction's location was.

David: how do you feel about the size increase, since you brought that up?

include/llvm/IR/DebugInfoMetadata.h
1512 ↗(On Diff #141919)

... common scope the the two locations.

dblaikie added a subscriber: vsk.Apr 10 2018, 7:13 PM

0.6% increase to debug_line (assuming you checked & this didn't increase
anything else? Can't see how it could, but just checking - /maybe/ some
inlining/debug_ranges stuff?) seems acceptable to me.

aprantl accepted this revision.Apr 11 2018, 11:09 AM
This revision is now accepted and ready to land.Apr 11 2018, 11:09 AM
This revision was automatically updated to reflect the committed changes.