[PATCH] D98288: [DSE] Translate killing locations through phis.

Wed Apr 7 06:04:26 PDT 2021

fhahn added a comment.

In D98288#2618947 <https://reviews.llvm.org/D98288#2618947>, @ebrevnov wrote:

> In general this patch seems to take exactly the same direction as D90095 <https://reviews.llvm.org/D90095> and is a strict subset of it. We can choose to either re-build functionality step by step or take D90095 <https://reviews.llvm.org/D90095> and split it to several smaller pieces. D90095 <https://reviews.llvm.org/D90095> adds the follwoing functionality:
>
> 1. Replaces main 'do-while' loop to 'for' loop in getDomMemoryDef. This is NFC and aimed at simplifying code structure and allows to minimize future changes.
> 2. Supports phi translation to any predecessor (not only immediate one)
> 3. Is able to phi translate in presence of geps and casts. In order to support this we have to track offests.
>
> Please let me know which way you prefer to go.

Now that this code is enabled by default (and the legacy implementation has gone away), I think we try to proceed in small steps if possible to keep things as stable as possible, because subtle correctness issues may remain dormant for a while.

If you'd be happy to break up your patch, that sounds great. I think the missing features (2. and 3. above) at least should be separate patches, with independent evaluation of the benefits & trade-offs. It might still make sense to start with the current patch as initial starting point and add functionality from D90095 <https://reviews.llvm.org/D90095> step-by-step.

In D98288#2672680 <https://reviews.llvm.org/D98288#2672680>, @asbirlea wrote:

> I'm seeing a crash with this. Repro: `opt -passes=dse` on:

Is it possible you did not apply the dependent patch D98287 <https://reviews.llvm.org/D98287> as well? Without this patch, I see a crash with the example you shared, but with D98287 <https://reviews.llvm.org/D98287> I do not see a crash.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98288/new/

https://reviews.llvm.org/D98288