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

[Florian Hahn via Phabricator via llvm-commits]

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

Thanks!



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1828
 
     MemoryAccess *StartAccess = Current;
+    Instruction *KillingI = KillingDef->getMemoryInst();
----------------
asbirlea wrote:
> 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.
That makes more sense, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1856
+      MemoryDef *CurrentDef = cast<MemoryDef>(Current);
+      Instruction *DomI = CurrentDef->getMemoryInst();
+
----------------
asbirlea wrote:
> s/DomI/CurrentI to match above naming?
Yes that should be updated as well. Adjusted before committing. Some for `DomLoc`.


================
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"
----------------
asbirlea wrote:
> 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.
I updated the tests to use `--check-prefixes=CHECK,DEFAULT-LIMIT`` --check-prefixes=CHECK,LARGER-LIMIT` respectively. Now CHECK prefixes are shared for all functions that do not change.


================
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.
----------------
asbirlea wrote:
> Update comment?
That must have slipped through, sorry about that. I also removed it from the copied comment in `multiblock-overlap.ll`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86487