[PATCH] D29149: NewGVN: Add basic dead store elimination

[Davide Italiano via Phabricator via llvm-commits]

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

LGTM (left some nits inline). You marked some earlier comments as done but the old code still shows up, probably you updated an earlier diff. Anyway, this can go in without a new round of review with the comments addressed.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:779
   MemoryAccess *StoreAccess = MSSA->getMemoryAccess(SI);
-  // See if we are defined by a previous store expression, it already has a
-  // value, and it's the same value as our current store. FIXME: Right now, we
-  // only do this for simple stores, we should expand to cover memcpys, etc.
+#if 1
+  // Get the expression, if any, for the RHS of the MemoryDef.
----------------
This `#if 1` can go away, I think.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1272-1276
+// This gets filled in by the moveValue
+#if 0
+        // First store is the leading memory access
+        NewClass->RepMemoryAccess = MSSA->getMemoryAccess(SI);
+#endif
----------------
This can go away, no?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2388
+        assert(DT->dominates(Leader->getParent(), Member->getParent()));
+        // Member is dominater by Leader, and thus dead
+        DEBUG(dbgs() << "Marking dead store " << *Member
----------------
Typo: s/dominater/dominated/


================
Comment at: test/Transforms/NewGVN/basic-cyclic-opt.ll:230-243
+;; This is an irreducible test case that will cause a memoryphi node loop
+;; in the two block.
+;; it's equivalent to something like
+;; *a = 0
+;; if (<....>) goto loopmiddle
+;; loopstart:
+;; loopmiddle:
----------------
thank you very much =)


================
Comment at: test/Transforms/NewGVN/pr31758.ll:1-19
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -newgvn %s -S -o - | FileCheck %s
 
 %struct.dipsy = type {}
 %struct.fluttershy = type { %struct.dipsy* }
 %struct.patatino = type {}
 
----------------
It seems this test is unrelated (no stores) and you already committed it, FWIW.


https://reviews.llvm.org/D29149