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

[David Blaikie via llvm-commits]

If an instruction is hoisted across a BB without adjusting the location, a
sample based profile taken at that location would incorrectly count a hit
to the source BB even though it may not be executed. (eg: CSE moves similar
code from an 'if' and 'else' block to before the predecessor (or sinks it
to a successor) and picks the 'if' block's instructions as the ones to move
(deleting the 'else' block) - now every time the profile observes the
evaluation of the common subexpression it counts it as the 'if' block
executing - when it's possible the 'if' block never executes and only the
'else' block is ever chosen)

On Wed, Feb 22, 2017 at 4:29 PM Adrian Prantl <aprantl at apple.com> wrote:

>
> 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> wrote:
>
> aprantl added a comment.
>
> [apologies for iterating so slowly at the moment]
>
> In 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/20170223/01b2847c/attachment.html>