[PATCH] D38088: Fix out-of-order stepping behavior in programs with hoisted constants.

[David Blaikie via llvm-commits]

In case it got lost in all the banter: Merge may not be sufficient if this
causes two instructions with the same location to be merged into a
different (common predecessor) basic block. So may be a bit subtle/tricky.
(in theory, for a profile, those would have distinct discriminators I would
think, so maybe it doesn't come up often/in practice - not entirely sure)

On Wed, Oct 25, 2017 at 2:59 PM Robinson, Paul <paul.robinson at sony.com>
wrote:

> I talked to Matthew this morning about this debate.  He will update the
> patch to use the merge API and repost it.
>
> --paulr
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* Tuesday, October 24, 2017 10:01 AM
> *To:* Adrian Prantl
> *Cc:* Robinson, Paul;
> reviews+D38088+public+4bc8f3de9212eeb7 at reviews.llvm.org; Voss, Matthew;
> craig.topper at gmail.com; echristo at gmail.com; llvm-commits at lists.llvm.org
>
>
> *Subject:* Re: [PATCH] D38088: Fix out-of-order stepping behavior in
> programs with hoisted constants.
>
>
>
>
>
> On Tue, Oct 24, 2017 at 9:52 AM Adrian Prantl <aprantl at apple.com> wrote:
>
>
>
> On Oct 24, 2017, at 9:23 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
>
>
> On Tue, Oct 24, 2017 at 9:20 AM Adrian Prantl <aprantl at apple.com> wrote:
>
>
>
> On Oct 24, 2017, at 9:12 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
>
>
> On Tue, Oct 24, 2017 at 9:08 AM Adrian Prantl <aprantl at apple.com> wrote:
>
>
>
> On Oct 24, 2017, at 9:05 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> It merges as well as hoisting
>
>
>
> Ok. But re-reading the motivation of the patch, it looks like merging the
> locations (let's assume they are identical) would not actually solve the
> issue the hoisted constant's location being unexpected, right?
>
> Were you suggesting to merge the location of the constant with the
> location of the InsertPoint?
>
>
> Yeah, I'm really not sure how to address it - if the constant is used in
> two different basic blocks but has the same location in both, but the
> constant is hoisted into a third basic block? We probably want to null out
> the location in that case (so I'm not sure merging it with the insertion
> point is ideal - letting the insertion point's location flow into this
> instruction if that's how it shakes out in the backend is fine, but
> actually specifying that location seems a bit off - if the instruction gets
> moved around some more, etc)
>
>
>
>
> Yes. Based on what the patch is trying to do the current implementation
> seems right.
>
>
> I guess the outstanding question I have is: is it worth trying to merge
> the location in the cases where it isn't hoisted into some other basic
> block? Any thoughts/strong feelings? Otherwise I'll probably just accept it
> as-is, even if the constant's used in two places in a single basic block
> and could have its location preserved witohut tainting profiling data.
>
>
>
> Ah I see. The distinction between it being hoisted into some other basic
> block is relevant for PGO because it only confuses PGO when the constant is
> moved to a different basic block. Yes, it would be better to merge to
> locations in that case, since it doesn't affect the PGO workflow or
> stepping in the debugger negatively, so we shouldn't throw away the
> location.
>
>
> Well it can/does still make for weird stepping behavior, I think - I think
> the constant can get hoisted to the start of the basic block, not just the
> first use (can anyone confirm this? If it only gets hoisted to the first
> use then, within a basic block, merging the locations would be fine - since
> you were already going to step there before this transformation) - so it
> could move far from any of the constant's uses.
>
> - Dave
>
>
>
>
> -- adrian
>
>
>
>
>
> In the long term I don't think we should be erasing accurate (but
> confusing) source locations, and instead (for example) be more principled
> about what we mark with is_stmt in the linetable, so we could have multiple
> classes of line table entries and a debugger may choose to ignore the more
> accurate ones if it messes with stepping (but have them available for
> crashes/breakpoints/etc). I'm not sure if this also applies exactly the
> same way to profilers, but in the worst case we could add another flag to
> the line table to mark is_interesting_profiler_location.
>
>
>
> -- adrian
>
>
>
>
>
>
>
> -- adrian
>
>
>
>
>
> On Tue, Oct 24, 2017 at 9:00 AM Adrian Prantl <aprantl at apple.com> wrote:
>
>
>
> > On Oct 24, 2017, at 8:48 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Tue, Sep 26, 2017 at 2:05 PM Robinson, Paul <paul.robinson at sony.com>
> wrote:
> > Doesn't need to be merged with the insertion point, I don't think? Does
> this merge across basic blocks? or just raise the uses of a constant to the
> beginning of a single basic block?
> >
> >
> >
> > I read the pass as looking for a dominating insertion point across
> multiple basic blocks.  Doesn't mean the insertion point necessarily is in
> a different block, but I *think* it could be.
> >
> >
> > Yeah, still seems like it might be nice to keep the location if it
> doesn't cross a basic block boundary.
> >
> > Adrian - any ideas/thoughts on all this?
>
> Looks like I missed the original review... Does Constant Hoisting just
> move a single constant to the top, or does it merge multiple identical
> constants (while hoisting them)? In the latter case getMergedLocation
> should definitely be used. In the former case I'm not so sure if erasing
> the location is the right thing to do in the first place (but it probably
> doesn't hurt much in practice). So I guess my question is: What are you
> suggesting should the location be merged with?
>
> -- adrian
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171025/aa5bceb9/attachment.html>