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

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
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);
+      }
+    }
+  }
----------------
andrew.w.kaylor wrote:
> 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.
Oh, yeah, huh, good point.

Thinking about that, I think that given a cleanupret/cleanupendpad:
 - if it unwinds to a catchendpad, it's an edge to the grandparent (we give the catchendpad the color of the parent of the catch, and the catch it's exiting is the parent of the cleanup)
 - if it unwinds to a cleanupendpad, it's an edge to the parent (we give the cleanupendpad the color of its cleanup, so this is a child cleanup unwinding to its parent cleanup)
 - if it unwinds to a catchpad or a cleanup pad or a terminatepad, it's an edge to a sibling

Does that sound right?

================
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,
----------------
andrew.w.kaylor wrote:
> 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?
It's my understanding that executing an 'unreachable' is UB and UB indicates conditions that the IR generator claimed could not possibly occur at runtime, and so the compiler has freedom to generate whatever dynamic behavior on that path is most convenient for it to generate.  And that in this case it's most convenient to convert the invoke to a call that would unwind to whatever parent it happens to get.

Even if I'm wrong about some detail of UB in the above, given the fact that SimplifyCFG turns invokes-to-unreachable into calls, I think that's a pretty good indication that this is a legal transform.

And the langref explicitly spells out that a catchpad/cleanuppad dynamically following a cycle of unwinds is UB, so yes the IR producer in that case is giving their guarantee that it will never dynamically happen.

Honestly I think it would be preferable to mark the call nounwind while we're at it, and to also have a way to indicate that an endpad is nounwind (and SimplifyCFG has a comment to that effect, unless it got dropped in the refactoring).  But it's my understanding that using call/unwind-to-caller is still legal though it's suboptimal.

What we're trying to accomplish is avoiding miscompiles if upstream passes have somehow (legally) bludgeoned the CFG into an unrecognizable derivative of itself.  No matter what those upstream passes did, they must have preserved legal dynamic paths, so one or more of the acyclic paths that we see may be a preservation of one/some of the original legal dynamic paths.  The langref precludes creating a well-behaved cyclic path in the original input because we don't have a way to encode that on the targets we're using this representation to target and neither do the source languages we're coming from have a way to express that so it's not a case we need to allow/handle.

================
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) {
----------------
andrew.w.kaylor wrote:
> 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?
Heh, I had originally suggested "OriginalMap or Clone2Orig", then I cut out the Clone2Orig suggestion before posting because I thought it would be confusing since the keys blocks aren't always clones.  If that doesn't bother you, I'm fine with Clone2Orig.  Or even IdentityMap if you still prefer it, I just wanted to point out that I got confused on first read.  And I can't think of a better suggestion at the moment...


Repository:
  rL LLVM

http://reviews.llvm.org/D13274