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

[David Blaikie via llvm-commits]

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.


> 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/20171024/a22ba334/attachment.html>