[PATCH] D137707: Move "auto-init" instructions to the dominator of their users

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:65
+    if (auto *M = dyn_cast<MemoryUseOrDef>(MA)) {
+      if (Instruction *MI = M->getMemoryInst()) {
+        if (MI->isLifetimeStartOrEnd() || MI == I)
----------------
Is it possible to have a MemoryUseOrDef without an associated instruction?  What would that represent?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:72
+      }
+    } else if (auto *M = dyn_cast<MemoryPhi>(MA)) {
+      for (unsigned i = 0, n = M->getNumIncomingValues(); i != n; ++i)
----------------
serge-sans-paille wrote:
> efriedma wrote:
> > I'm not sure I understand the PHI handling here... the incoming values of a PHI are essentially original store itself, and other stores that store to the same location.  I'm not sure how analyzing the other stores helps here.  You want the store to dominate the incoming edge of the PHI, not the other stores.
> > 
> Indeed, I had it the other way around. Fixed in latest revision of the patch.
I'm not confident that looking through PHIs like this is actually what you want.  A MemoryPHI implies there's a loop that modifies the value in question. Maybe the cycle protection code protects you from running into issues with that, though?

I haven't really worked with MemorySSA much; if someone wants to jump in to review this aspect, that would be welcome.  I'm generally happy with the approach using MemorySSA, just not sure about the finer details.


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

https://reviews.llvm.org/D137707