[PATCH] D13274: [WinEH] Clone funclets with multiple parents

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added a comment.

Thanks for taking this on.  I think the logic for cleanupret/cleanupendpad needs to be a little different, and that `makeFuncletEdgeUnreachable` can defer to `removeUnwindEdge`; aside from that, just a few nits.


================
Comment at: lib/CodeGen/WinEHPrepare.cpp:2980-2987
@@ +2979,10 @@
+    for (BasicBlock *Parent : ColorMapItem) {
+      assert(std::find(FuncletChildren[Parent].begin(),
+                       FuncletChildren[Parent].end(),
+                       FuncletEntry) == std::end(FuncletChildren[Parent]));
+      FuncletChildren[Parent].push_back(FuncletEntry);
+      assert(std::find(FuncletParents[FuncletEntry].begin(),
+                       FuncletParents[FuncletEntry].end(),
+                       Parent) == std::end(FuncletParents[FuncletEntry]));
+      FuncletParents[FuncletEntry].push_back(Parent);
+    }
----------------
Looks like you found an elegant way to determinize the output.  Cool.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:2997-2998
@@ +2996,4 @@
+// original funclet are remapped to unwind to the clone.  Any terminator in the
+// original funclet which returned to this parent is converted to an unreachable
+// to an unreachable instruction. Likewise, any terminator in the cloned funclet
+// which returns to a parent funclet other than the specified parent is
----------------
typo: "to an unreachable to an unreachable"

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3039-3053
@@ +3038,17 @@
+      }
+    } else if (auto *CRI = dyn_cast<CleanupReturnInst>(Terminator)) {
+      // If we're cloning a cleanup pad and the uncloned cleanup pad unwinds
+      // to a block in the clone parent, we need to replace the cleanupret with
+      // an unreachable instruction.
+      BasicBlock *UnwindDest = CRI->getUnwindDest();
+      if (UnwindDest && FuncletBlocks[Parent].count(UnwindDest)) {
+        CRI->eraseFromParent();
+        new UnreachableInst(BB->getContext(), BB);
+        // Loop over all of the successors, removing BB's entry from any PHI
+        // nodes.
+        for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI != SE;
+             ++SI)
+          (*SI)->removePredecessor(BB);
+      }
+    }
+  }
----------------
Cleanupret targets a sibling, not a parent.  So,
 1. I don't think this code should be here.
 2. I guess you should have code somewhere that updates these sibling->sibling edges, though you probably can't do that until both siblings have been cloned...

Also, whatever you do about this with cleanupret, I think cleanupendpad should get the same treatment

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3087-3100
@@ +3086,16 @@
+      }
+    } else if (auto *CRI = dyn_cast<CleanupReturnInst>(Terminator)) {
+      // If we're cloning a cleanup pad and the cloned cleanup pad does not
+      // unwind to a block in the clone parent, we need to replace the
+      // cleanupret with an unreachable instruction.
+      BasicBlock *UnwindDest = CRI->getUnwindDest();
+      if (UnwindDest && !FuncletBlocks[Parent].count(UnwindDest)) {
+        CRI->eraseFromParent();
+        new UnreachableInst(NewBlock->getContext(), NewBlock);
+        // Loop over all of the successors, removing NewBlock's entry from any
+        // PHI nodes.
+        for (succ_iterator SI = succ_begin(NewBlock), SE = succ_end(NewBlock);
+             SI != SE; ++SI)
+          (*SI)->removePredecessor(NewBlock);
+      }
+    }
----------------
As above, cleanupret should target a sibling, so I don't think this is right.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3168-3170
@@ +3167,5 @@
+  // This asserts a condition that is relied upon inside the loop below.
+  for (auto *Pred : predecessors(FuncletEntry))
+    assert(std::find(pred_begin(ClonedFunclet), pred_end(ClonedFunclet),
+                     Pred) == pred_end(ClonedFunclet));
+
----------------
nit: you could use `std::all_of` to get this whole thing under the assert (or wrap it in `#ifndef NDEBUG`)

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3206-3208
@@ +3205,5 @@
+// previously unwound to the specified child funclet so that the unwind
+// path is unreachable.  In the case of invoke instructions which unwound to
+// the child funclet, a cleanup pad with an unreachable instruction as its
+// body is created and used as the unwind destination.
+void WinEHPrepare::makeFuncletEdgeUnreachable(BasicBlock *Parent,
----------------
Why not just transform such invokes into calls?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3217-3219
@@ +3216,5 @@
+    // An invoke that unwinds to the child funclet must be handled as a
+    // special case.  Simply converting the invoke to a call would leave
+    // the behavior in an ambiguous state.  We want the unwind edge to
+    // actually be flagged as unreachable.
+    //
----------------
I don't follow this logic.  If simplifycfg determines an invoke's EH pad to be unreachable, it changes it to a call, and I don't see why we couldn't do the same here.  It would be nice in both cases to mark the call as !MayThrow, but that can/should be a separate change.

I think the argument goes: since the unwind edge is unreachable, that must mean that no exception escapes the call/invoke, so modeling it as a call (that will happen not to throw) is sound.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3250-3251
@@ +3249,4 @@
+      UnwindDest = I->getUnwindDest();
+    else if (auto *I = dyn_cast<CleanupEndPadInst>(Terminator))
+      UnwindDest = I->getUnwindDest();
+    else if (auto *I = dyn_cast<TerminatePadInst>(Terminator))
----------------
CleanupEndPad transfers to sibling, same as CleanupRet

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3260-3261
@@ +3259,4 @@
+    if (UnwindDest == Child) {
+      Terminator->eraseFromParent();
+      new UnreachableInst(BB->getContext(), BB);
+    }
----------------
In the case that the terminator is a catchendpad or terminatepad, this has the same problem you called out above that an unreachable can't be an EH pad.

I think you could remove the special-casing for invoke (so add a case for it in the if/else ladder down here), and if it's a terminator whose unwind edge goes to the child and needs to be removed, call `llvm::removeUnwindEdge(BB)`.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3292-3294
@@ +3291,5 @@
+    bool IsCyclic = false;
+    BasicBlock *ChildIdentity = IdentityMap[ChildFunclet];
+    if (!ChildIdentity)
+      IdentityMap[ChildFunclet] = ChildIdentity = ChildFunclet;
+    for (BasicBlock *Ancestor : FuncletPath) {
----------------
FWIW, I had no idea what `IdentityMap` would be from its name.  I did find `Orig2Clone` above intuitive, so maybe something like `OriginalMap`  might be better?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3297-3298
@@ +3296,4 @@
+      BasicBlock *AncestorIdentity = IdentityMap[Ancestor];
+      if (!AncestorIdentity)
+        IdentityMap[Ancestor] = AncestorIdentity = Ancestor;
+      if (AncestorIdentity == ChildIdentity) {
----------------
I'm pretty sure you'd never hit this case if you pre-seeded `IdentityMap` to map the function entry block to itself.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3318
@@ +3317,3 @@
+  }
+}
+
----------------
I think probably right here after the loop mapping children ends is the right place to fix up cleanupret/cleanupendpad (sibling) edges.


Repository:
  rL LLVM

http://reviews.llvm.org/D13274