[PATCH] D21076: [DSE] Ignore dbg value when instruction iterator is reset.

[Daniel Berlin via llvm-commits]

SGTM

On Tue, Jun 14, 2016 at 9:08 AM, Henric Karlsson <
henric.karlsson at ericsson.com> wrote:

> On 2016-06-14 17:34, Daniel Berlin wrote:
>
>
>
> On Tue, Jun 14, 2016 at 2:02 AM, Henric Karlsson <
> <henric.karlsson at ericsson.com>henric.karlsson at ericsson.com> wrote:
>
>> Henric added a comment.
>>
>> In http://reviews.llvm.org/D21076#456942, @dberlin wrote:
>>
>> > Note: bb->eraseFromParent now returns an iterator, so you can fix this
>> by returning and using that instead  :)
>>
>>
>> This will change the behavior quite a bit, and then I think it's better
>> to change it so we set BBI = BB.begin()
>>
>> We typically have this case:
>> ----------------------------
>>
>> Store -> A   (Dead)
>> nextInst      (so iterator from eraseFromParent() will point here)
>> ...
>> Store -> A  (current instruction)
>>
>> nextInst      (BBI typically points here)
>> -----------------------------------------
>>
>> But this also made me realize that all this --BBI might also be
>> unnecessary, just setting BBI to Inst->getIterator() might be good enough.
>> On the other hand if we also remove loads and not only the store
>> instruction, then we might open up for detecting new dead stores, so could
>> be worth it to revisiting the basic block from the beginning again.
>>
>
> Err, you if you process remove stores in the right order, you should never
> have to revisit the basic block.
>
> If it's already iterating in reverse order, and erasing the later stores,
> then yeah, just keep it working.
>
> Ok, sounds like the DSE algorithm has room for improvements. And that it's
> better to keep the patch as it is, which shouldn't change  the current
> behavior, but make output from -g the same.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160614/acb3d6e5/attachment.html>