[PATCH] D86487: [DSE,MemorySSA] Check if DomAccess is valid for elimination first.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea accepted this revision.
asbirlea added a comment.
This revision is now accepted and ready to land.

A few nits, but lgtm.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1828
 
     MemoryAccess *StartAccess = Current;
+    Instruction *KillingI = KillingDef->getMemoryInst();
----------------
nit: flip naming. Argument to `StartAccess` and local var that gets modified in loop to `Current`. Seems easier to follow IMO and it's only 2 lines to update.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1856
+      MemoryDef *CurrentDef = cast<MemoryDef>(Current);
+      Instruction *DomI = CurrentDef->getMemoryInst();
+
----------------
s/DomI/CurrentI to match above naming?


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/combined-partial-overwrites.ll:3
 ; RUN: opt -S -dse -enable-dse-memoryssa -enable-dse-partial-store-merging=false < %s | FileCheck %s
+; RUN: opt -S -dse -enable-dse-memoryssa -enable-dse-partial-store-merging=false -dse-memoryssa-partial-store-limit=10 < %s | FileCheck --check-prefix=LARGER-LIMIT %s
 target datalayout = "E-m:e-i64:64-n32:64"
----------------
use `--check-prefixes=CHECK,LARGER-LIMIT` to avoid duplication of the check lines that are the same and highlight the optimizations missed by the default limit.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/combined-partial-overwrites.ll:293
 
+; We miss this case, because of aggressive an aggressive limit of partial
+; overlap analysis. With a larger partial store limit, we remove the memset.
----------------
Update comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86487