Page MenuHomePhabricator

Update getMergedLocation to check the instruction type and merge properly.
ClosedPublic

Authored by danielcdh on Sep 14 2017, 5:00 PM.

Details

Summary

If the merged instruction is call instruction, we need to set the scope to the closes common scope between 2 locations, otherwise it will cause trouble when the call is getting inlined.

Event Timeline

danielcdh created this revision.Sep 14 2017, 5:00 PM
dblaikie added inline comments.Sep 14 2017, 5:09 PM
include/llvm/IR/DebugInfoMetadata.h
1434–1435

Perhaps the API should change to be less error prone (so it's not the responsibility of the caller to know to pass the Instruction if it might be a call).

Could this be "applyMergedLocation" and have it take the Instruction as the first parameter & modify its debug location in place? (are there callers that don't have the destination Instruction on-hand at the point where they want to make this call?)

lib/IR/DebugInfo.cpp
680–687

I'd expect to walk more than the inlined-at attributes, but the plain scopes too. Which should look something like "keep calling getScope until it returns null, then use getInlinedAt, then go back to walking scopes, etc, until getInlinedAt returns null".

Probably not as important/necessary, but could still help smooth out debug_ranges which cost a bunch of debug info size.

test/Transforms/SimplifyCFG/remove-debug.ll
4–5

Might be worth checking the specific instructions that these lines apply to?

danielcdh marked an inline comment as done.Sep 14 2017, 5:52 PM
danielcdh added inline comments.
include/llvm/IR/DebugInfoMetadata.h
1434–1435

We will also have MachineInstruction to call this.

lib/IR/DebugInfo.cpp
680–687

Not sure if I understand... why would walking scopes help? And also, the case for merged call should be very rare, may not have big impact on overall size?

danielcdh updated this revision to Diff 115331.Sep 14 2017, 5:52 PM

update test

aprantl edited edge metadata.Sep 15 2017, 8:05 AM

Just wanted to say that I like the direction this is going and LGTM from my side once you sorted out the details David mentioned.

danielcdh added inline comments.Sep 15 2017, 6:23 PM
include/llvm/IR/DebugInfoMetadata.h
1434–1435

Updated the API:

MachineInstruction should use the original API
Instruction should use the new API to pass in the Instruction *

lib/IR/DebugInfo.cpp
680–687

Discussed offline with David. It is an overkill to add another level of complexity to traverse Scope chain because

  • it will not either help the user of the tool (the purpose of merge location is to prevent "wrong location" to be the "right location", not to get the right location, which is impossible to get anyway.
  • it will not affect debug info size much: our experiments on a very large binary shows that it only has 0.00008% impact on the debug_line size and 0.00003% impact on the debug_info size.
danielcdh updated this revision to Diff 115525.Sep 15 2017, 6:23 PM

update the API

dblaikie accepted this revision.Sep 18 2017, 12:47 PM

Looks good, thanks!

Adrian - dunno what you think about the API changes here. Open to iterating (probably just post-commit) if you reckon this doesn't add enough, or have other ideas.

lib/Transforms/Utils/SimplifyCFG.cpp
1541

This probably needs a similar assert/comment that I0 isn't a CallInst?

This revision is now accepted and ready to land.Sep 18 2017, 12:47 PM

I think this works for me.

include/llvm/IR/DebugInfoMetadata.h
1430

it has ?

1435

Please add something like /// See documentation of \p getMergedLocation.

aprantl added inline comments.Sep 18 2017, 1:53 PM
include/llvm/IR/DebugInfoMetadata.h
1436

Sorry, one more thing: Why is this a static function instead of a member?

danielcdh marked 2 inline comments as done.Sep 18 2017, 7:02 PM
danielcdh added inline comments.
include/llvm/IR/DebugInfoMetadata.h
1436

Moved the API to Instruction::applyMergedLocation.

Looks mostly good now. One more inline comment.

include/llvm/IR/Instruction.h
391

Should we document what happens to the debug location already attached to this? Or - looking at the use-cases - should it assert that the debugloc is empty?

danielcdh added inline comments.Sep 19 2017, 9:14 AM
include/llvm/IR/Instruction.h
391

Not sure if I understand this. this->debugloc will be either empty, or locA or locB. But it will be overwritten anyway, why do we care about its value?

ping...

Adrian, please let me know if you have further concerns about this patch.

Thanks,
Dehao

LGTM, I would add one extra line to the comment of applyMergedLocation (see inline).
Thanks!

include/llvm/IR/DebugInfoMetadata.h
1428

I think if we spell this as \p applyMergedLocation it gets turned into a link by Doxygen, might be worth trying.

include/llvm/IR/Instruction.h
391

I was thinking of adding an extra line like to make the behavior clear to the casual reader:
/// The DebugLoc attached to this instruction will be overwritten by the merged DebugLoc.

danielcdh updated this revision to Diff 117386.Oct 2 2017, 10:33 AM
danielcdh marked 2 inline comments as done.

update comments

aprantl accepted this revision.Oct 2 2017, 10:58 AM
danielcdh closed this revision.Oct 2 2017, 11:14 AM