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

[Andy Kaylor via llvm-commits]

andrew.w.kaylor added a comment.

Responding to comments.

I'll update the patch when I've reworked the issues that were raised.


================
Comment at: lib/CodeGen/WinEHPrepare.cpp:2978
@@ -2969,2 +2977,3 @@
   for (BasicBlock *FuncletEntry : EntryBlocks) {
     std::set<BasicBlock *> &ColorMapItem = BlockColors[FuncletEntry];
+    for (BasicBlock *Parent : ColorMapItem) {
----------------
JosephTremoulet wrote:
> Since we expect multi-parent funclets to be extremely rare, it's probably worthwhile to record here whether you ever see a `ColorMapItem` with size > 1, then have `resolveFuncletAncestry(Function*)` check that and return immediately if there's no work for it to do.
OK

================
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);
+      }
+    }
+  }
----------------
JosephTremoulet wrote:
> 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
I see what you're saying, but cleanupret doesn't always unwind to a sibling.  If it's the last cleanup inside a catch it can unwind to the parent (as it does in most of the current test cases).

I'll add some test cases involving sibling and then see if I can work out a good way to handle them.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3184-3196
@@ +3183,15 @@
+
+    // Tidy up if the PHI node has become trivial.
+    unsigned RemainingValues = PN->getNumIncomingValues();
+    if (!RemainingValues) {
+      PHIsToErase.push_back(PN);
+    } else if (RemainingValues == 1) {
+      auto *PV = PN->getIncomingValue(0);
+      if (PV != PN)
+          PN->replaceAllUsesWith(PV);
+        else
+          // We are left with an infinite loop with no entries: kill the PHI.
+          PN->replaceAllUsesWith(UndefValue::get(PN->getType()));
+      PHIsToErase.push_back(PN);
+    }
+  }
----------------
majnemer wrote:
> Is this strictly necessary? Will the code in cleanupPreparedFunclets not handle these cases?
I'm not sure.  I believe I was seeing one-entry PHI nodes when I was debugging these changes, but I was working with an old version of the code.  I'll try taking this out and if things aren't being cleaned up I'll figure out why.

================
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,
----------------
JosephTremoulet wrote:
> Why not just transform such invokes into calls?
I originally had it that way, but I wasn't sure it accomplished the same thing.  If we convert the invoke to a call then it will unwind to whatever exception context happens to be around it, whereas my understanding was that if we get into this case it will be undefined behavior for the invoked function to throw an exception.

Then again, it did occur to me that some later pass is pretty likely to convert an invoke that unwinds to an unreachable instruction into a call.

I guess this gets at the question of what we're really trying to accomplish here.  By inserting these unreachable instructions we're saying that if the code should never dynamically reach that point, but what if it does?  Should we be inserting llvm.trap calls?

================
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) {
----------------
JosephTremoulet wrote:
> 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?
I see what you're saying, but I'm not sure I like OriginalMap any better.  How about Clone2Orig?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3297-3298
@@ +3296,4 @@
+      BasicBlock *AncestorIdentity = IdentityMap[Ancestor];
+      if (!AncestorIdentity)
+        IdentityMap[Ancestor] = AncestorIdentity = Ancestor;
+      if (AncestorIdentity == ChildIdentity) {
----------------
JosephTremoulet wrote:
> I'm pretty sure you'd never hit this case if you pre-seeded `IdentityMap` to map the function entry block to itself.
Yes, that's true.  I was thinking that by only inserting funclets as I see them it would save some search time for the earlier funclets encountered if we ever had a case with a lot of funclets.  Thinking about it now, I'm not sure that's worth the trouble.


Repository:
  rL LLVM

http://reviews.llvm.org/D13274