[PATCH] D116200: [instcombine] Allow sinking of calls with known writes to uses

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3772
+      return false;
+    auto *AI = dyn_cast<AllocaInst>(Dest->Ptr->stripPointerCasts());
+    if (!AI)
----------------
nikic wrote:
> FWIW this could also use getUnderlyingObject() instead, as we don't care that the start of the alloca in particular is passed to the function, having an offset is fine.
Without handling for GEP below, underlying object doesn't gain us anything.  I was planning on leaving offsets to a follow up patch with separate testing.  


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3779
+    // account for cycles in doing so.
+    SmallVector<const User *, 4> AllocaUsers(AI->users());
+    while (!AllocaUsers.empty()) {
----------------
nikic wrote:
> Unless something else already prevents this, I think you need a Visited set here to prevent potential infinite loops in unreachable code with cyclic references.
Really good catch!


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

https://reviews.llvm.org/D116200