[PATCH] D29833: Improve the API of DILocation::getMergedLocation()

[Adrian Prantl via llvm-commits]

> On Feb 22, 2017, at 4:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Feb 22, 2017 at 4:21 PM Adrian Prantl via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> aprantl added a comment.
> 
> [apologies for iterating so slowly at the moment]
> 
> In https://reviews.llvm.org/D29833#682595 <https://reviews.llvm.org/D29833#682595>, @twoh wrote:
> 
> > Personally I prefer the existing API, because it doesn't allow the transient state before merging. For example, in lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp in this patch, DebugLoc of NewSI is not actually valid after line 1430. Existing API doesn't have this problem.
> 
> 
> If that is the only concern, would an additional convenience function solve (= sufficiently hide) the problem?
> 
>   Instruction::setMergedDebugLoc(DebugLoc A, DebugLoc B) {
>     setDebugLoc(A);
>     mergeDebugLocWith(B);
>   }
> 
> The primary benefit of having the API on Instruction is that we can make it impossible to accidentally create a locationless call instruction in a function with debuginfo (which would crash/assert the backend when creating inline scopes).
> 
> Either way we have a bug - either the profile will be buggy or the inlining broken, honestly since the buggy profile is harder to diagnose (because it has a location, but an incorrect one) I'd sort of favor an API that makes those bugs easy/the default, rather than the subtlety of incorrect locations.

If we have to make a choice between buggy profile and a broken inline info (that will assert) I agree with you. But can you remind me why the DILocation::getMergedDebugLocation() API prevents the buggy profile?

-- adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170222/d9d373ab/attachment.html>