This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Unify logic to merge DILocations. NFC.
ClosedPublic

Authored by vsk on Nov 3 2017, 7:24 PM.

Details

Summary

This makes DILocation::getMergedLocation() do what its comment says it
does when merging locations for an Instruction: set the common inlineAt
scope. This simplifies Instruction::applyMergedLocation() a bit.

Testing: check-llvm, check-clang

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Nov 3 2017, 7:24 PM
aprantl edited edge metadata.Nov 4 2017, 8:20 AM

It looks like this will cause DILocation::getMergedLocation() to create new DILocations? When this happens at the MIR level this will cause problems with the textual MIR output because the new locations aren't printed. Should we introduce a flag to disable creation of new metadata?

vsk added a comment.Nov 6 2017, 10:52 AM

It looks like this will cause DILocation::getMergedLocation() to create new DILocations? When this happens at the MIR level this will cause problems with the textual MIR output because the new locations aren't printed. Should we introduce a flag to disable creation of new metadata?

getMergedLocation() shouldn't be able to create a new DILocation unless ForInst is-a CallInst, which shouldn't happen at the MIR level. If we added a flag to explicitly disable creation of new metadata, it would be 'true' iff ForInst is not a CallInst. Can I instead add more documentation for the ForInst parameter?

What about the common-parent-scope-finding logic?

vsk added a comment.Nov 6 2017, 11:10 AM

What about the common-parent-scope-finding logic?

Prior to this patch, the common-parent-scope finding logic could only kick in if this is-a CallInst. That behavior is preserved: DILocation::getMergedLocation() only finds a common parent scope when ForInst is-a CallInst.

aprantl accepted this revision.Nov 6 2017, 11:20 AM

Oh.. I overlooked the early exit. Yeah I guess we can do this.

Can I instead add more documentation for the ForInst parameter?

definitely.

This revision is now accepted and ready to land.Nov 6 2017, 11:20 AM
This revision was automatically updated to reflect the committed changes.
uweigand added a subscriber: uweigand.EditedFeb 16 2018, 8:03 AM

I've run into an internal compiler error that seems caused by this behavior of getMergedLocation: https://bugs.llvm.org/show_bug.cgi?id=36410