[PATCH] D51801: [MemorySSAUpdater] Avoid creating self-referencing MemoryDefs

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:276
-  // We change them in this order otherwise we will appear in the use list
-  // above and reset ourselves.
   MD->setDefiningAccess(DefBefore);
----------------
Removing the comment as this is already happening and it's what we are trying to fix.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:406
   What->replaceAllUsesWith(What->getDefiningAccess());
 
   // Let MemorySSA take care of moving it around in the lists.
----------------
Another way to fix this would be to add `What->replaceUsesOfWith(What->getDefiningAccess(), nullptr);` here, so that `What` gets removed from the Uses of its defining  memory access. This would make sure that the graph of Uses/Users is in a correct state before calling insertDef(). However I haven't tested it thoroughly and it's also more expensive so I am in favor of the current fix.


================
Comment at: test/Transforms/GVNHoist/pr38807.ll:32
+;CHECK-NOT:   store i64 undef, i64* %{.*}
+
+bb2:
----------------
I am not sure why but the gep remains after the hoist, and goes away with dead code elimination. Is that correct @sebpop ?


================
Comment at: test/Transforms/GVNHoist/pr38807.ll:42
+;CHECK-NOT:   store i64 undef, i64* %{.*}
+
+bb3:
----------------
Same here.


https://reviews.llvm.org/D51801