[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: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)


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