[PATCH] D72631: [DSE] Eliminate stores at the end of the function.

Wed Jun 24 05:22:35 PDT 2020

fhahn marked 4 inline comments as done.
fhahn added a comment.

In D72631#2109804 <https://reviews.llvm.org/D72631#2109804>, @asbirlea wrote:

> Thank you for the patch!
>
> Minor comments on the tests for keeping the patch clean, but otherwise lgtm.
>
> Please consider the performance comments for future improvements.

Thanks, I added TODO comments and plan to follow up to them soon. I'll try to collect a few cases with compile-time outliers for further analysis/tuning and share them.

================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/combined-partial-overwrites.ll:255
 ; CHECK-NEXT:    [[BPTR2:%.*]] = getelementptr inbounds i8, i8* [[BPTR]], i64 2
+; CHECK-NEXT:    [[BPTR3:%.*]] = getelementptr inbounds i8, i8* [[BPTR]], i64 3
 ; CHECK-NEXT:    [[WPTR:%.*]] = bitcast i8* [[BPTR]] to i16*
----------------
asbirlea wrote:
> Look unrelated to the patch? Perhaps commit separately as NFC?
Done, thanks!

================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/mda-with-dbg-values.ll:72

 ; Check that the store is removed and that the memcpy is still there
 ; CHECK-LABEL: foo
----------------
Whitney wrote:
> update comment.
Done, thanks!

================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll:38
 ; CHECK-LABEL: @test3(
+; CHECK-NEXT:    [[B:%.*]] = alloca i8, align 1
 ; CHECK-NEXT:    ret void
----------------
asbirlea wrote:
> Commit first as NFC?
> Same for below?
Done, thanks!

================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-captures.ll:210
 }
 ; TODO: Remove store in exit.
 ; Stores to stack objects can be eliminated if they are not captured inside the function.
----------------
Whitney wrote:
> Some of the TODO comments can also be removed.
Done, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72631