[WinEH] Clone funclets with multiple parents

[andrew.w.kaylor]

Summary

This patch clones the entire body of any funclet with more than one parent to avoid inconsistencies in the funclet hierarchy. Paths through the now multiple copies of the funclet which return to a funclet other than their intended parent are terminated with unreachable instructions.

Note that it was necessary to modify the wineh-demotion.ll and wineh-no-demotion.ll tests for this patch. The IR in the wineh-demotion.ll test appears to have been invalid and I just cleaned it up, attempting to preserve the intent of the test.

The wineh-no-demotion.ll test is directly impacted by the changes in this patch. Following the logic of the new funclet cloning algorithm for two of the test cases in this file results in multiple identical funclets with a unreachable terminator. This seems bad, but it wasn't clear to me whether it was preferable to not clone funclets in a situation like this given that the test cases in wineh-cloning.ll which were created in advance for this new cloning scenario include cleanup pads with unreachable terminators and seem to expect them to be cloned.

[JosephTremoulet]

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.

lib/CodeGen/WinEHPrepare.cpp

3292–3294

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?

3297–3298

I'm pretty sure you'd never hit this case if you pre-seeded IdentityMap to map the function entry block to itself.

3318

I think probably right here after the loop mapping children ends is the right place to fix up cleanupret/cleanupendpad (sibling) edges.

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

2980–2987

Looks like you found an elegant way to determinize the output. Cool.

2997–2998

typo: "to an unreachable to an unreachable"

3039–3053

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

3087–3100

As above, cleanupret should target a sibling, so I don't think this is right.

3168–3170

nit: you could use std::all_of to get this whole thing under the assert (or wrap it in #ifndef NDEBUG)

3206–3208

Why not just transform such invokes into calls?

3217–3219

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.

3250–3251

CleanupEndPad transfers to sibling, same as CleanupRet

3260–3261

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).

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

2978

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.

[majnemer]

Intuitively, I'd expect cloning a funclet results in the creation of a new PHI if there are live-outs in the original funclet. Is this case impossible?

lib/CodeGen/WinEHPrepare.cpp

3184–3196

Is this strictly necessary? Will the code in cleanupPreparedFunclets not handle these cases?

[andrew.w.kaylor]

Responding to comments.

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

lib/CodeGen/WinEHPrepare.cpp

2978

OK

3039–3053

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.

3184–3196

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.

3206–3208

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?

3292–3294

I see what you're saying, but I'm not sure I like OriginalMap any better. How about Clone2Orig?

3297–3298

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.

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

3039–3053

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?

3206–3208

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.

3292–3294

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...

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

3039–3053

That sounds right for IR generated in the expected ways. I'm not certain that it isn't possible to construct legal IR that does other things (like cleanret returning to an arbitrary block in the parent). I don't know why it would ever happen, but do our written rules disallow it?

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

3039–3053

1. Regarding

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)

I think that since the langref specifies:

It is undefined behavior to execute a catchendpad if, after the most recent execution of the normal successor edge of any catchpad chained to it, any other catchpad or cleanuppad has been executed but has not had a corresponding catchret/cleanupret/catchendpad/cleanupendpad executed

then it would be UB if the catchendpad were not ending a parent catch, and therefore having the grandparent color.

2. Regarding

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)

I think that since the langref specifies

It is undefined behavior to execute a cleanupendpad if, after the most recent execution of its cleanuppad, any other cleanuppad or catchpad has been executed but has not had a corresponding cleanupret/catchret/cleanupendpad/catchendpad executed

then it would be UB if the cleanupendpad were not ending a parent cleanup, and therefore having the parent color.

3. Regarding

if it unwinds to a catchpad or a cleanup pad or a terminatepad, it's an edge to a sibling

This is a direct consequence of the coloring algorithm. For these sibling edges, I can't see why we would want to do anything other than translate them to the corresponding cloned-sibling edges between children of the cloned node.

4. Regarding

like cleanret returning to an arbitrary block in the parent

• A cleanupret successor edge is an unwind edge, and so can only target an EH pad. I actually am not seeing this in the langref, but it's certainly there in the verifier.
• A catchret targeting a block in the middle of the "wrong" handler is an interesting case. I think you're right that the langref doesn't preclude it, so I guess either we should find a wording that would preclude it or continue cloning at the point it targets? Continuing cloning would complicate the cycle detection -- the UB rules in the langref indicate that you can't enter a pad twice (execute the foopad twice without an intervening fooret/fooendpad) and that you can't return from a pad twice (execute the catchret twice without an intervening catchpad), and currently you're building the graph/tree/paths with the assumption that any node entered is entered via its entry pad and so detecting cycles just on the pad...

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

3039–3053

Regarding

A catchret targeting a block in the middle of the "wrong" handler

We're actually already doing the extra cloning needed for that in cloneCommonBlocks (which runs just before this) -- we explicitly add the parent color to the catchret successor during coloring, so if that's "in the wrong handler" it will get two colors (the parent and the "wrong" one), and we'll clone so that the catchret only targets the parent; you should be able to assert at this point that every catchret (whose catchpad argument is the child catchpad) targets the parent.

[rnk]

Are we sure we need to handle this case with cloning? The intention of the design was that the rules around catchendpad and cleanupendpad would make it impossible for the optimizer to form invokes that don't unwind through the associated end pad.

For cleanupendpad, it directly consumes a token for the associated cleanuppad, so we can mark any invokes in a cleanuppad that don't transitively unwind to the cleanupendpad as unreachable.

For catchendpad, the rules about how catchpads are allowed to unwind should allow us to create a many-to-one association from catchpad to cleanupendpad and apply the same transformation to unreachable.

[JosephTremoulet]

In D13274#260936, @rnk wrote:

Are we sure we need to handle this case with cloning? The intention of the design was that the rules around catchendpad and cleanupendpad would make it impossible for the optimizer to form invokes that don't unwind through the associated end pad.

For cleanupendpad, it directly consumes a token for the associated cleanuppad, so we can mark any invokes in a cleanuppad that don't transitively unwind to the cleanupendpad as unreachable.

For catchendpad, the rules about how catchpads are allowed to unwind should allow us to create a many-to-one association from catchpad to cleanupendpad and apply the same transformation to unreachable.

We can do both the things you mention, and we do them in removeImplausibleTerminators. This cloning is about handling cases where what should be separate funclets branch to common code (like maybe they branch to a shared call void @abort() ; unreachable). If the common code itself has an invoke to another funclet (like maybe we started with the previous but then @abort got inlined), we can't tell which of the original two it is supposed to be a sub-funclet of, and so we clone it.

[andrew.w.kaylor]

As I've been reworking this to handle siblings, my test case has uncovered an ugly problem with the existing cloneCommonBlocks code.

Consider this test case:

define void @test13() personality i32 (...)* @__CxxFrameHandler3 {
entry:
  invoke void @f()
    to label %exit unwind label %outer

outer:
  %o = cleanuppad []
  invoke void @f()
    to label %outer.cont unwind label %left
outer.cont:
  invoke void @f()
    to label %unreachable unwind label %right
outer.end:
  cleanupendpad %o unwind to caller

left:
  %l = cleanuppad []
  invoke void @f()
    to label %left.cont unwind label %right
left.cont:
  cleanupret %l unwind label %outer.end

right:
  %r = catchpad []
    to label %right.catch unwind label %right.end
right.catch:
  invoke void @f()
    to label %unreachable unwind label %right.end
right.end:
  catchendpad unwind label %outer.end

unreachable:
  unreachable
}

colorFunclets() will assign the colors 'outer' (because of the cleanupret in 'left.cont') and 'left' (because of the catchendpad in 'right.end') to the 'outer.end' block. Subsequently, cloneCommonBlocks() will clone 'outer.end' to handle this and assign the color 'outer' to the clone and remove that color from the original, but it doesn't update the predecessors of 'outer.end' and so the cloned block is unreachable and the outer coloring is essentially lost.

I'm working on a solution to this problem now.

[majnemer]

In D13274#261224, @andrew.w.kaylor wrote:

As I've been reworking this to handle siblings, my test case has uncovered an ugly problem with the existing cloneCommonBlocks code.

Consider this test case:

define void @test13() personality i32 (...)* @__CxxFrameHandler3 {
entry:
  invoke void @f()
    to label %exit unwind label %outer

outer:
  %o = cleanuppad []
  invoke void @f()
    to label %outer.cont unwind label %left
outer.cont:
  invoke void @f()
    to label %unreachable unwind label %right
outer.end:
  cleanupendpad %o unwind to caller

left:
  %l = cleanuppad []
  invoke void @f()
    to label %left.cont unwind label %right
left.cont:
  cleanupret %l unwind label %outer.end

right:
  %r = catchpad []
    to label %right.catch unwind label %right.end
right.catch:
  invoke void @f()
    to label %unreachable unwind label %right.end
right.end:
  catchendpad unwind label %outer.end

unreachable:
  unreachable
}

colorFunclets() will assign the colors 'outer' (because of the cleanupret in 'left.cont') and 'left' (because of the catchendpad in 'right.end') to the 'outer.end' block. Subsequently, cloneCommonBlocks() will clone 'outer.end' to handle this and assign the color 'outer' to the clone and remove that color from the original, but it doesn't update the predecessors of 'outer.end' and so the cloned block is unreachable and the outer coloring is essentially lost.

I'm working on a solution to this problem now.

Isn't this IR busted because there is no associated cleanupendpad for %l?
Also, why does the cleanupendpad get more than one color? That sounds very strange because you'd never want to clone it without cloning %o... I'd expect that it either have it's own color or have cleanuppad's color (and propagate the colors feeding into cleanuppad).

[andrew.w.kaylor]

In D13274#261278, @majnemer wrote:

Isn't this IR busted because there is no associated cleanupendpad for %l?
Also, why does the cleanupendpad get more than one color? That sounds very strange because you'd never want to clone it without cloning %o... I'd expect that it either have it's own color or have cleanuppad's color (and propagate the colors feeding into cleanuppad).

This goes back to the motivation for this change set. The IR here is kind of busted, but if some paths are dynamically unreachable then it's potentially OK. Specifically, if the invoke in left.cont throws is caught by right.catch but the invoke of f() there neither throws nor returns, then it's kind of OK. The idea here is that some theoretical optimization pass combined blocks with unreachable terminators to create this state.

As for the cleanupendpad having more than one color, that's kind of a transitory issue, I think. Once the multiple parents issue has been sorted out, one of the colors should go away. Maybe this doesn't need to be resolved in cloneCommonBlocks() but it kind of alarmed me when I saw what was happening there, and I was hoping that maybe addressing it would simplify what needed to be done later to handle this case.

[andrew.w.kaylor]

-rebased
-adressed feedback from previous review
-added new test cases

I think the number of test cases I'm adding for cloning funclets with multiple parents justifies starting a new test file. The relevant tests in wineh-cloning.ll are currently duplicated for review purposes, but I'll remove those before checking in.

[JosephTremoulet]

Overall looks great, and thanks for doing this! I don't see where invoke->catchendpad edges are getting rewritten to the right parent, and there's a call to ->getUnwindDest() that I think should be a loop; other than that, just several minor nits and questions.

lib/CodeGen/WinEHPrepare.cpp

345–1

You can remove this now, right?

347–348

Worth asserting FuncletCloningRequired?

348–349

With this and all the other debug output, is there something better to print to identify the block than ->getName()? That works great for hand-written tests, but for debugging real-world failures I'd expect a front-end to either have not named these blocks (so this will be empty string if I'm reading the implementation correctly) or to have given them all the same name, like "catch.dispatch", so the printout wouldn't be helpful (again, if I'm reading getName() correctly). Printing the block's address would identify them uniquely, but then wouldn't correspond to anything in a dump of the function...

To be clear, I'm fine with this change as-is if that's just what's available, I'm really just asking to know what's available when I want to add similar debug traces in the future.

356–367

I see you reset BlockColors[BB] to the single parent here, but I don't see you remove BB from FuncletBlocks[OtherParent] for all the other parents. Should you be doing that?

358–363

Seems like this should assert there's only one CatchParent (in debug) and/or break out of the loop as soon as it finds one (in release).

Also, nit: You're getting the color of the parent of the catchpad that is a predecessor of the endpad, so the name CatchParent confused me a bit (I assumed it would be the parent of the catch); maybe CatchPred or even just CatchPad?

370

I don't understand this comment; we call this for more than just catchendpads...

377

if CloneParent is a catch that unwinds to another catch that unwinds to another catch that ... unwinds to a catchendpad, and that catchendpad is UnwindDest, then do you still want to take this path? I.e. I think instead of a single ->getUnwindDest() here, you may want a loop that doesn't terminate until it finds an unwind dest that's a catchendpad.

389

Confusing comment again; the thing that unwinds is an arbitrary EH pad, not necessarily a catchendpad, right?

415

same as above

464

Did you intend to update this pre-checkin? Fine with me to check in the FIXME as-is, just wanted to make sure it was intentional.

509–513

Do the new tests cover these cases? I.e. where funclet A includes a branch to block Shared, and funclet B has child catch funclet C, and Shared is the target of the catchret from C back to B?

515–518

does "caller" here mean the same thing as "parent"?

598

I think you may need to check if CloneTerminator is an invoke and UnwindDest is a catchendpad, and enforce that the catchendpad is in the parent if so.

696–698

Could this be assert(!FuncletParents[ClonedFunclet].count(Parent))?

715–716

This comment doesn't match the code (but the comment on line 1075 does) -- siblings aren't handled here.

791–792

Is cleanupendpad ever a parent-to-child edge? I'd have thought no.

865

I would have thought we could assert that Parents[UnwindDest].size() == 1 here, no (i.e. we should have already resolved ancestry path for all the siblings, and the above checks should ensure this is a sibling)?

878

As on line 1212, I'd have thought we could assume/assert that siblings have a single parent here.

test/CodeGen/WinEH/wineh-cloning.ll

135

Looks like a copy-paste error: I think you want [[I_R]] here instead of [[I_R:\%.+]]

test/CodeGen/WinEH/wineh-multi-parent-cloning.ll

71 ↗

(On Diff #37951)

[[I_R]]

113–114 ↗

(On Diff #37951)

I'm not seeing why left is a parent of right. I thought that "is a parent of" meant "has an invoke or catchendpad that unwinds to, or has a child cleanuppad whose cleanupret/cleanupendpad unwinds to", and I don't see that happening anywhere in this test.

119–121 ↗

(On Diff #37951)

Did we stop making this clone with rL250534?

506–507 ↗

(On Diff #37951)

Why does that happen?

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

345–1

Yes.

347–348

Yes, probably so.

348–349

I would expect that anytime you're trying to use the debug option you'd have names for all the blocks. They may be very similar names (catch.dipatch. catch.dipatch.1, etc.), but they will be unique.

I'm not sure if there's a way to match what the IR dumper does in the case where the blocks are unnamed. I don't know a better way to get a useful label.

356–367

Yeah, probably so.

358–363

The verifier checks that condition, but I suppose it wouldn't hurt to assert it here too. I did mean to break out of the loop when I found the catch parent.

370

Oops. Slipped through refactoring.

377

Yes, you're right. I'll add a test case to cover that.

464

I'll probably do it before checkin, but I felt like I needed to draw the line somewhere to get these changes back up for review.

509–513

I don't think so. I'll add something.

515–518

No. What I meant is that the unwind destination is null. I was trying to use the same terminology the IR dumps use ("unwind to caller"). I guess that's not entirely clear.

598

Hmmm. That's a bit sticky. If the funclet being cloned is a catchpad that unwinds to a catchendpad, I'm cloning the catchendpad, so if the invoke occurs in that sort of funclet it should get remapped naturally (though it's possible that the VMap isn't being updated properly in the case where the cloned catchendpad has to be associated with the original funclet rather than the clone).

On the other hand, if the funclet being cloned is a catchpad that unwinds to another catchpad, the unwind destination for the invoke may need to be a catchendpad clone that hasn't been created yet. I think I'll need to handle this case in the same place that I'm doing the sibling terminator fix-up.

More test cases....

696–698

No, because FuncletParents maps to a vector, not a set.

I could make it

assert(!std::count(FuncletParents[ClonedFunclet].begin(),

FuncletParents[ClonedFunclet].end(),
Parent));

if you think that's easier to comprehend at a glance.

715–716

Yeah, I'm not sure what I was thinking here.

791–792

Maybe not. Part of what's given me trouble in this change set is figuring out which cases that I can sneak past the verifier are legitimate things that could happen and which just don't make any sense. I'm not sure I even had a case that looked like this. I'll see if I can come up with something and if I do I'll report back.

865

I don't think so. If a funclet has three parents, then the first time we encounter it we'll clone it, but the original funclet will still have two parents.

test/CodeGen/WinEH/wineh-multi-parent-cloning.ll

113–114 ↗

(On Diff #37951)

This comments seems to be left over from an earlier version of the test.

119–121 ↗

(On Diff #37951)

Yes, I think so.

506–507 ↗

(On Diff #37951)

I believe it's because of the cleanupret in inner.sibling unwinding to left.end, which unwinds to right. I'll take a closer look and see exactly where this is happening and why.

It may be that this isn't happening anymore since I added the code to prevent cloning of catchendpads.

[JosephTremoulet]

lib/CodeGen/WinEHPrepare.cpp

358–363

FWIW, relying on the verifier makes sense to me.

515–518

Ah, ok. I think what confused me was the phrase "its caller", since you're referring to the caller of the function, and I took "it" to mean the cleanup; just changing "its caller" to "the caller" or "the calling function", or even just changing "unwinds to its caller" to "unwinds to caller" so it would more obviously match the IR text, would have cleared it up for me.

598

Ugh, yeah. If it's cleaner, you could try to mess with the cloning order so that you handle the unwinds-to-catchendpad catch before you handle any of the unwinds-to-catchpad catches. Not at all clear to me that that would be cleaner, just wanted to toss out the idea...

696–698

Oh, yeah, thanks for explaining. No, I think what you have is just as easy to comprehend as std::count.

[JosephTremoulet]

Another thought: the only reason I gave catchendpads the parent color was because it simplified the coloring algorithm a bit. But we could switch to another convention like giving a catchendpad the color of the catchpad that unwinds directly to it; that would only complicate the coloring algorithm a little (when visiting a catchpad we'd have to scroll through the chain to find the catchendpad and give its successor the parent color), so if it would drastically simplify this part it might be worthwhile.

[andrew.w.kaylor]

Added new test cases.
Added support for catchret in cloned funclets.
Added support for invokes that unwind to parent EH pads.

[JosephTremoulet]

LGTM, thanks for all the updates and new tests!

lib/CodeGen/WinEHPrepare.cpp

3200

MIssing right-paren: "catch pad)."

3386

typo: removinv -> removing

3422–3423

typo: "reached from a reached from a" -> "reached from a"

[chapuni]

Reverted in r252279.

We should assume that iterating pointer key values in std::set and std::map would be nondeterministic.

[andrew.w.kaylor]

I understood that iterating sets and maps was not deterministic and thought I had accounted for that. There are several places in this code where the non-determinism is OK because it doesn't lead to non-deterministic output. I just had one bug where I was looking at unwind edges that hadn't been processed yet.

In any event, reverting was the right thing to do. I should have this ready to go back in shortly.

lib/CodeGen/WinEHPrepare.cpp

509–513

This isn't working and I'm not sure how it should be handled. Consider the following function:

define void @test13() personality i32 (...)* @__CxxFrameHandler3 {
entry:
  invoke void @f()
    to label %invoke.cont unwind label %left
invoke.cont:
  invoke void @f()
    to label %exit unwind label %right
left:
  %l = catchpad []
    to label %left.cont unwind label %left.end
left.cont:
  invoke void @f()
    to label %left.ret unwind label %inner
left.ret:
  catchret %l to label %invoke.cont
left.end:
  catchendpad unwind to caller
right:
  %r = catchpad []
    to label %right.catch unwind label %right.end
right.catch:
  invoke void @f()
    to label %right.ret unwind label %inner
right.ret:
  catchret %r to label %exit
right.end:
  catchendpad unwind to caller
shared:
  call void @h(i32 0)
  unreachable
inner:
  %i = catchpad []
    to label %inner.catch unwind label %inner.end
inner.catch:
  call void @h(i32 1)
  catchret %i to label %shared
inner.end:
  catchendpad unwind label %left.end
exit:
  ret void
}

The existing funclet coloring code will recognize that it needs a copy of %shared for both %left and %right, but because both are reached from the same catchret instruction one of the clones will be unreachable following cloneCommonBlocks(). Then when my new code gets to the point of cloning %inner it will remove the catchret in %inner.catch for whichever of the parents received the unreachable clone of %shared (because the reachable %shared belongs to the other funclet).

I could instead make yet another clone of %shared at this point, but that really just pushes the problem to the next block in cases where %shared has successors. What I really need to do is to be able to find the unreachable clone. Does that sound reasonable?

[andrew.w.kaylor]

lib/CodeGen/WinEHPrepare.cpp

509–513

This was an old comment. I'm not sure why it only showed up now.

include/llvm/Transforms/Utils/Local.h

 
 /// MergeBasicBlockIntoOnlyPred - BB is a block with one predecessor and its
 /// predecessor is known to have one successor (BB!).  Eliminate the edge
 /// between them, moving the instructions in the predecessor into BB.  This
 /// deletes the predecessor block.
 ///
 void MergeBasicBlockIntoOnlyPred(BasicBlock *BB, DominatorTree *DT = nullptr);
 
+/// Replace an invoke instruction with a call of the same value followed by an
+/// unconditional branch to the normal destination of the invoke.
+void convertInvokeToCall(InvokeInst *II);
+
 /// TryToSimplifyUncondBranchFromEmptyBlock - BB is known to contain an
 /// unconditional branch, and contains no instructions other than PHI nodes,
 /// potential debug intrinsics and the branch.  If possible, eliminate BB by
 /// rewriting all the predecessors to branch to the successor block and return
 /// true.  If we can't transform, return false.
 bool TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB);
 
 /// EliminateDuplicatePHINodes - Check for and eliminate duplicate PHI

lib/CodeGen/WinEHPrepare.cpp

private:
   void replaceUseWithLoad(Value *V, Use &U, AllocaInst *&SpillSlot,
                           DenseMap<BasicBlock *, Value *> &Loads, Function &F);
   void demoteNonlocalUses(Value *V, std::set<BasicBlock *> &ColorsForBB,
                           Function &F);
   bool prepareExplicitEH(Function &F,
                          SmallVectorImpl<BasicBlock *> &EntryBlocks);
   void replaceTerminatePadWithCleanup(Function &F);
   void colorFunclets(Function &F, SmallVectorImpl<BasicBlock *> &EntryBlocks);
+  void resolveFuncletAncestry(Function &F);
+  void resolveFuncletAncestryForPath(
+      Function &F, SmallVectorImpl<BasicBlock *> &FuncletPath,
+      std::map<BasicBlock *, BasicBlock *> &IdentityMap);
+  void makeFuncletEdgeUnreachable(BasicBlock *Parent, BasicBlock *Child);
+  BasicBlock *cloneFuncletForParent(Function &F, BasicBlock *FuncletEntry,
+                                    BasicBlock *Parent);
   void demotePHIsOnFunclets(Function &F);
   void demoteUsesBetweenFunclets(Function &F);
   void demoteArgumentUses(Function &F);
   void cloneCommonBlocks(Function &F,
                          SmallVectorImpl<BasicBlock *> &EntryBlocks);
   void removeImplausibleTerminators(Function &F);
   void cleanupPreparedFunclets(Function &F);
   void verifyPreparedFunclets(Function &F);
private:
   // Map from outlined handler to call to parent local address. Only used for
   // 32-bit EH.
   DenseMap<Function *, Value *> HandlerToParentFP;
 
   AllocaInst *SEHExceptionCodeSlot = nullptr;
 
   std::map<BasicBlock *, std::set<BasicBlock *>> BlockColors;
   std::map<BasicBlock *, std::set<BasicBlock *>> FuncletBlocks;
-  std::map<BasicBlock *, std::set<BasicBlock *>> FuncletChildren;
+  std::map<BasicBlock *, std::vector<BasicBlock *>> FuncletChildren;
+  std::map<BasicBlock *, std::vector<BasicBlock *>> FuncletParents;
 };
 
 class WinEHFrameVariableMaterializer : public ValueMaterializer {
 public:
   WinEHFrameVariableMaterializer(Function *OutlinedFn, Value *ParentFP,
                                  FrameVarInfoMap &FrameVarInfo);
   ~WinEHFrameVariableMaterializer() override {}
 
private:
   // This will be a reference to the field of the same name in the WinEHPrepare
   // object which instantiates this WinEHCatchDirector object.
   DenseMap<LandingPadInst *, const LandingPadInst *> &NestedLPtoOriginalLP;
   DominatorTree *DT;
   SmallPtrSetImpl<BasicBlock *> &EHBlocks;
 };
 
 class WinEHCleanupDirector : public WinEHCloningDirectorBase {
 public:
   WinEHCleanupDirector(Function *CleanupFn, Value *ParentFP,
                        FrameVarInfoMap &VarInfo, LandingPadMap &LPadMap)
       : WinEHCloningDirectorBase(CleanupFn, ParentFP, VarInfo,
                                  LPadMap) {}
 
   CloningAction handleBeginCatch(ValueToValueMapTy &VMap,
                                  const Instruction *Inst,
                                  BasicBlock *NewBB) override;
   CloningAction handleEndCatch(ValueToValueMapTy &VMap, const Instruction *Inst,
                                BasicBlock *NewBB) override;
   CloningAction handleTypeIdFor(ValueToValueMapTy &VMap,
                                 const Instruction *Inst,
                                 BasicBlock *NewBB) override;
   CloningAction handleIndirectBr(ValueToValueMapTy &VMap,
                                  const IndirectBrInst *IBr,
                                  BasicBlock *NewBB) override;
   CloningAction handleInvoke(ValueToValueMapTy &VMap, const InvokeInst *Invoke,
                              BasicBlock *NewBB) override;
   CloningAction handleResume(ValueToValueMapTy &VMap, const ResumeInst *Resume,
                              BasicBlock *NewBB) override;
   CloningAction handleCompare(ValueToValueMapTy &VMap, const CmpInst *Compare,
                               BasicBlock *NewBB) override;
   CloningAction handleLandingPad(ValueToValueMapTy &VMap,
                                  const LandingPadInst *LPad,
                                  BasicBlock *NewBB) override;
 };
 
 class LandingPadActions {
 public:
   LandingPadActions() : HasCleanupHandlers(false) {}
 
   void insertCatchHandler(CatchHandler *Action) { Actions.push_back(Action); }
   void insertCleanupHandler(CleanupHandler *Action) {
     Actions.push_back(Action);
     HasCleanupHandlers = true;
   }
 
   bool includesCleanup() const { return HasCleanupHandlers; }
 
   SmallVectorImpl<ActionHandler *> &actions() { return Actions; }
   SmallVectorImpl<ActionHandler *>::iterator begin() { return Actions.begin(); }
   SmallVectorImpl<ActionHandler *>::iterator end() { return Actions.end(); }
 
 private:
   // Note that this class does not own the ActionHandler objects in this vector.
   // The ActionHandlers are owned by the CatchHandlerMap and CleanupHandlerMap
   // in the WinEHPrepare class.
   SmallVector<ActionHandler *, 4> Actions;
   bool HasCleanupHandlers;
 };

 }
 
 bool WinEHPrepare::runOnFunction(Function &Fn) {
   if (!Fn.hasPersonalityFn())
     return false;
 
   // No need to prepare outlined handlers.
   if (Fn.hasFnAttribute("wineh-parent"))
     return false;
 
   // Classify the personality to see what kind of preparation we need.
   Personality = classifyEHPersonality(Fn.getPersonalityFn());
 
   // Do nothing if this is not an MSVC personality.
   if (!isMSVCEHPersonality(Personality))
     return false;
 

 }
 
 bool WinEHPrepare::doFinalization(Module &M) { return false; }
 
 void WinEHPrepare::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<DominatorTreeWrapperPass>();
   AU.addRequired<TargetLibraryInfoWrapperPass>();
 }
 
 static bool isSelectorDispatch(BasicBlock *BB, BasicBlock *&CatchHandler,
                                Constant *&Selector, BasicBlock *&NextBB);
 
 // Finds blocks reachable from the starting set Worklist. Does not follow unwind
 // edges or blocks listed in StopPoints.
 static void findReachableBlocks(SmallPtrSetImpl<BasicBlock *> &ReachableBBs,
                                 SetVector<BasicBlock *> &Worklist,
                                 const SetVector<BasicBlock *> *StopPoints) {
static Instruction *findBeginCatchSplitPoint(BasicBlock *BB,
   // don't split.
   Instruction *FirstNonPHI = BB->getFirstNonPHI();
   if (II == FirstNonPHI)
     return nullptr;
 
   // If either operand is in the same basic block as the instruction and
   // isn't used by another instruction before the begincatch call, include it
   // in the split block.
   auto *Op0 = dyn_cast<Instruction>(II->getOperand(0));
   auto *Op1 = dyn_cast<Instruction>(II->getOperand(1));
 
   Instruction *I = II->getPrevNode();
   Instruction *LastI = II;
 
   while (I == Op0 || I == Op1) {
     // If the block begins with one of the operands and there are no other
     // instructions between the operand and the begincatch call, don't split.
     if (I == FirstNonPHI)
       return nullptr;
 
     LastI = I;
     I = I->getPrevNode();
   }
 
   // If there is at least one instruction in the block before the begincatch
   // call and its operands, split the block at either the begincatch or
if (isCatchAllLandingPad(BB)) {
       EHReturnBlocks.insert(BB);
       continue;
     }
 
     BasicBlock *CatchHandler;
     BasicBlock *NextBB;
     Constant *Selector;
     if (isSelectorDispatch(BB, CatchHandler, Selector, NextBB)) {
       // Split the edge if there are multiple predecessors. This creates a place
       // where we can insert EH recovery code.
       if (!CatchHandler->getSinglePredecessor()) {
         DEBUG(dbgs() << "splitting EH return edge from " << BB->getName()
                      << " to " << CatchHandler->getName() << '\n');
         BBI = CatchHandler = SplitCriticalEdge(
             BB, std::find(succ_begin(BB), succ_end(BB), CatchHandler));
       }
       EHReturnBlocks.insert(CatchHandler);
for (BasicBlock &BB : F) {
     EHVals.insert(LP);
     for (User *U : LP->users()) {
       auto *EI = dyn_cast<ExtractValueInst>(U);
       if (!EI)
         continue;
       EHVals.insert(EI);
       for (User *U2 : EI->users()) {
         if (auto *PN = dyn_cast<PHINode>(U2))
           EHVals.insert(PN);
       }
     }
   }
 
   SetVector<Argument *> ArgsToDemote;
   SetVector<Instruction *> InstrsToDemote;
   for (BasicBlock &BB : F) {
     bool IsNormalBB = NormalBlocks.count(&BB);
     bool IsEHBB = EHBlocks.count(&BB);
     if (!IsNormalBB && !IsEHBB)
       continue; // Blocks that are neither normal nor EH are unreachable.
     for (Instruction &I : BB) {
       for (Value *Op : I.operands()) {
         // Don't demote static allocas, constants, and labels.
         if (isa<Constant>(Op) || isa<BasicBlock>(Op) || isa<InlineAsm>(Op))
           continue;
         auto *AI = dyn_cast<AllocaInst>(Op);
         if (AI && AI->isStaticAlloca())
           continue;
 
         if (auto *Arg = dyn_cast<Argument>(Op)) {
           if (IsEHBB) {
             DEBUG(dbgs() << "Demoting argument " << *Arg
                          << " used by EH instr: " << I << "\n");
             ArgsToDemote.insert(Arg);
           }
           continue;
         }

 }
 
 bool WinEHPrepare::prepareExceptionHandlers(
     Function &F, SmallVectorImpl<LandingPadInst *> &LPads) {
   // Don't run on functions that are already prepared.
   for (LandingPadInst *LPad : LPads) {
     BasicBlock *LPadBB = LPad->getParent();
     for (Instruction &Inst : *LPadBB)
       if (match(&Inst, m_Intrinsic<Intrinsic::eh_actions>()))
         return false;
   }
 
   identifyEHBlocks(F, LPads);
   demoteValuesLiveAcrossHandlers(F, LPads);
 
   // These containers are used to re-map frame variables that are used in
   // outlined catch and cleanup handlers.  They will be populated as the
   // handlers are outlined.
for (LandingPadInst *LPad : LPads) {
 
     LandingPadActions Actions;
     mapLandingPadBlocks(LPad, Actions);
 
     HandlersOutlined |= !Actions.actions().empty();
     for (ActionHandler *Action : Actions) {
       if (Action->hasBeenProcessed())
         continue;
       BasicBlock *StartBB = Action->getStartBlock();
 
       // SEH doesn't do any outlining for catches. Instead, pass the handler
       // basic block addr to llvm.eh.actions and list the block as a return
       // target.
       if (isAsynchronousEHPersonality(Personality)) {
         if (auto *CatchAction = dyn_cast<CatchHandler>(Action)) {
           processSEHCatchHandler(CatchAction, StartBB);
           continue;
         }
       }
 
       outlineHandler(Action, &F, LPad, StartBB, FrameVarInfo);
     }
 
     // Split the block after the landingpad instruction so that it is just a
     // call to llvm.eh.actions followed by indirectbr.
     assert(!isa<PHINode>(LPadBB->begin()) && "lpad phi not removed");
     SplitBlock(LPadBB, LPad->getNextNode(), DT);
     // Erase the branch inserted by the split so we can insert indirectbr.
     LPadBB->getTerminator()->eraseFromParent();
 
void WinEHPrepare::colorFunclets(Function &F,
   }
 
   // The processing above actually accumulated the parent set for this
   // funclet into the color set for its entry; use the parent set to
   // populate the children map, and reset the color set to include just
   // the funclet itself (no instruction can target a funclet entry except on
   // that transitions to the child funclet).
   for (BasicBlock *FuncletEntry : EntryBlocks) {
     std::set<BasicBlock *> &ColorMapItem = BlockColors[FuncletEntry];
-    for (BasicBlock *Parent : ColorMapItem)
-      FuncletChildren[Parent].insert(FuncletEntry);
+    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);
+    }
     ColorMapItem.clear();
     ColorMapItem.insert(FuncletEntry);
   }
 }
 
+// Clones all blocks used by the specified funclet to avoid the funclet having
+// multiple parent funclets.  All terminators in the parent that unwind to the
+// 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
+// converted to an unreachable instruction.
+BasicBlock *WinEHPrepare::cloneFuncletForParent(Function &F,
+                                                BasicBlock *FuncletEntry,
+                                                BasicBlock *Parent) {
+  std::set<BasicBlock *> &BlocksInFunclet = FuncletBlocks[FuncletEntry];
+
+  std::map<BasicBlock *, BasicBlock *> Orig2Clone;
+  ValueToValueMapTy VMap;
+  for (BasicBlock *BB : BlocksInFunclet) {
+    // Create a new basic block and copy instructions into it.
+    BasicBlock *CBB =
+        CloneBasicBlock(BB, VMap, Twine(".from.", Parent->getName()));
+
+    // Insert the clone immediately after the original to ensure determinism
+    // and to keep the same relative ordering of any funclet's blocks.
+    CBB->insertInto(&F, BB->getNextNode());
+
+    // Add basic block mapping.
+    VMap[BB] = CBB;
+
+    // Record delta operations that we need to perform to our color mappings.
+    Orig2Clone[BB] = CBB;
+
+    // Any path in the uncloned funclet that returns to the clone's parent
+    // must become unreachable.
+    TerminatorInst *Terminator = BB->getTerminator();
+    if (auto *CRI = dyn_cast<CatchReturnInst>(Terminator)) {
+      // If we're cloning a catch pad and the uncloned catch pad returns to
+      // a block in the clone parent, we need to replace the catchret with
+      // an unreachable instruction.
+      if (FuncletBlocks[Parent].count(CRI->getSuccessor())) {
+        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);
+      }
+    } 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);
+      }
+    }
+  }
+
+  BasicBlock *ClonedFunclet = Orig2Clone[FuncletEntry];
+  assert(ClonedFunclet);
+
+  // Set the coloring for the blocks we just cloned.
+  std::set<BasicBlock *> &ClonedBlocks = FuncletBlocks[ClonedFunclet];
+  for (auto &BBMapping : Orig2Clone) {
+    BasicBlock *NewBlock = BBMapping.second;
+    ClonedBlocks.insert(NewBlock);
+    BlockColors[NewBlock].insert(ClonedFunclet);
+
+    // Use the VMap to remap the instructions in this cloned block.
+    for (Instruction &I : *NewBlock)
+      RemapInstruction(&I, VMap, RF_IgnoreMissingEntries);
+
+    // Any unwind destination in the cloned funclet that isn't an expected
+    // funclet child or a block in the designated parent funclet must become
+    // unreachable.
+    TerminatorInst *Terminator = NewBlock->getTerminator();
+    if (auto *CRI = dyn_cast<CatchReturnInst>(Terminator)) {
+      // If we're cloning a catch pad and the cloned catch pad returns to
+      // a block that is not in the clone parent, we need to replace the
+      // catchret with an unreachable instruction.
+      if (!FuncletBlocks[Parent].count(CRI->getSuccessor())) {
+        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);
+      }
+    } 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);
+      }
+    }
+
+    // Check to see if the cloned block successor has PHI nodes. If so, we need
+    // to add entries to the PHI nodes for the cloned block now.
+    BasicBlock *OldBlock = BBMapping.first;
+    for (BasicBlock *SuccBB : successors(NewBlock)) {
+      for (Instruction &SuccI : *SuccBB) {
+        auto *SuccPN = dyn_cast<PHINode>(&SuccI);
+        if (!SuccPN)
+          break;
+
+        // Ok, we have a PHI node.  Figure out what the incoming value was for
+        // the OldBlock.
+        int OldBlockIdx = SuccPN->getBasicBlockIndex(OldBlock);
+        if (OldBlockIdx == -1)
+          break;
+        Value *IV = SuccPN->getIncomingValue(OldBlockIdx);
+
+        // Remap the value if necessary.
+        if (auto *Inst = dyn_cast<Instruction>(IV)) {
+          ValueToValueMapTy::iterator I = VMap.find(Inst);
+          if (I != VMap.end())
+            IV = I->second;
+        }
+
+        SuccPN->addIncoming(IV, NewBlock);
+      }
+    }
+  }
+
+
+  // Remove the clone's parent from the original funclet's parent list.
+  std::remove(FuncletParents[FuncletEntry].begin(),
+              FuncletParents[FuncletEntry].end(), Parent);
+
+  // Store the cloned funclet's parent.
+  assert(std::find(FuncletParents[ClonedFunclet].begin(),
+                   FuncletParents[ClonedFunclet].end(),
+                   Parent) == std::end(FuncletParents[ClonedFunclet]));
+  FuncletParents[ClonedFunclet].push_back(Parent);
+
+  // Copy any children of the original funclet to the clone.  We'll either
+  // clone them too or make that path unreachable when we take the next step
+  // in resolveDuplicateFuncletAncestry().
+  for (auto *Child : FuncletChildren[FuncletEntry]) {
+    assert(std::find(FuncletChildren[ClonedFunclet].begin(),
+                     FuncletChildren[ClonedFunclet].end(),
+                     Child) == std::end(FuncletChildren[ClonedFunclet]));
+    FuncletChildren[ClonedFunclet].push_back(Child);
+    assert(std::find(FuncletParents[Child].begin(), FuncletParents[Child].end(),
+                     ClonedFunclet) == std::end(FuncletParents[Child]));
+    FuncletParents[Child].push_back(ClonedFunclet);
+  }
+
+  // Find any place in the parent block that branched to the original funclet
+  // and remap it to the clone.
+  for (BasicBlock *BB : FuncletBlocks[Parent]) {
+    if (!FuncletEntry->isUsedInBasicBlock(BB))
+      continue;
+    // We're about to break the path from this block to the uncloned funclet
+    // entry, so remove it as a predeccessor to clean up the PHIs.
+    FuncletEntry->removePredecessor(BB);
+    TerminatorInst *Terminator = BB->getTerminator();
+    RemapInstruction(Terminator, VMap, RF_IgnoreMissingEntries);
+  }
+
+  // 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));
+
+  // Remove any invalid PHI node entries in the cloned funclet.
+  std::vector<PHINode *> PHIsToErase;
+  for (Instruction &I : *ClonedFunclet) {
+    auto *PN = dyn_cast<PHINode>(&I);
+    if (!PN)
+      break;
+
+    // Predecessors of the original funclet do not reach the cloned funclet,
+    // but the cloning process assumes they will.  Remove them now.
+    for (auto *Pred : predecessors(FuncletEntry))
+      PN->removeIncomingValue(Pred, false);
+
+    // 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);
+    }
+  }
+  for (auto *PN : PHIsToErase)
+    PN->eraseFromParent();
+
+  return ClonedFunclet;
+}
+
+// Modifies exceptional terminators within the specified parent funclet that
+// 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,
+                                              BasicBlock *Child) {
+  for (BasicBlock *BB : FuncletBlocks[Parent]) {
+    TerminatorInst *Terminator = BB->getTerminator();
+    if (!Terminator->isExceptional())
+      continue;
+
+    // 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.
+    //
+    // The verifier requires the unwind edge of an invoke to be an exception
+    // block so we need to create one and populate it with an unreachable
+    // instruction.
+    if (auto *II = dyn_cast<InvokeInst>(Terminator)) {
+      BasicBlock *UnwindDest = II->getUnwindDest();
+      if (UnwindDest == Child) {
+        Function *F = Parent->getParent();
+        LLVMContext &Context = F->getContext();
+
+        // Create an unreachable block.
+        BasicBlock *UnreachableBB = BasicBlock::Create(
+            Context, Twine(Parent->getName(), ".unreachable"), F, UnwindDest);
+        CleanupPadInst::Create(Context, {}, "", UnreachableBB);
+        new UnreachableInst(Context, UnreachableBB);
+        II->setUnwindDest(UnreachableBB);
+
+        // Set up the coloring for this new funclet.
+        BlockColors[UnreachableBB].insert(UnreachableBB);
+        // This doesn't undate FuncletBlocks, FuncletChildren, FuncletParents or
+        // EntryBlocks, but since the funclet is unreachable, that should be OK.
+      }
+      continue;
+    }
+
+    // For other exceptional terminators, look for terninators that
+    // unwind to the child funclet.
+    BasicBlock *UnwindDest = nullptr;
+    if (auto *I = dyn_cast<CatchEndPadInst>(Terminator))
+      UnwindDest = I->getUnwindDest();
+    else if (auto *I = dyn_cast<CleanupEndPadInst>(Terminator))
+      UnwindDest = I->getUnwindDest();
+    else if (auto *I = dyn_cast<TerminatePadInst>(Terminator))
+      UnwindDest = I->getUnwindDest();
+    // catchret and cleanupret don't represent a parent-to-child funclet
+    // transition, so we don't need to consider them here.
+
+    // If the child funclet is the unwind destination, replace the terminator
+    // with an unreachable instruction.
+    if (UnwindDest == Child) {
+      Terminator->eraseFromParent();
+      new UnreachableInst(BB->getContext(), BB);
+    }
+  }
+  // The specified parent is no longer a parent of the specified child.
+  std::remove(FuncletChildren[Parent].begin(), FuncletChildren[Parent].end(),
+              Child);
+}
+
+void WinEHPrepare::resolveFuncletAncestry(Function &F) {
+  // Make sure each funclet has a unique parent.
+  std::map<BasicBlock *, BasicBlock *> IdentityMap;
+  SmallVector<BasicBlock *, 4> FuncletPath;
+  FuncletPath.push_back(&(F.getEntryBlock()));
+  resolveFuncletAncestryForPath(F, FuncletPath, IdentityMap);
+}
+
+// Walks the funclet control flow, cloning any funclets that have more than one
+// parent funclet and breaking any cyclic unwind chains so that the path becomes
+// unreachable at the point where a funclet would have unwound to a funclet that
+// was already in the chain.
+void WinEHPrepare::resolveFuncletAncestryForPath(
+    Function &F, SmallVectorImpl<BasicBlock *> &FuncletPath,
+    std::map<BasicBlock *, BasicBlock *> &IdentityMap) {
+  BasicBlock *CurFunclet = FuncletPath.back();
+  // Copy the children vector because we might changing it.
+  std::vector<BasicBlock *> Children(FuncletChildren[CurFunclet]);
+  for (BasicBlock *ChildFunclet : Children) {
+    // Don't allow the funclet chain to unwind back on itself.
+    // If this funclet is already in the current funclet chain, make the
+    // path to it through the current funclet unreachable.
+    bool IsCyclic = false;
+    BasicBlock *ChildIdentity = IdentityMap[ChildFunclet];
+    if (!ChildIdentity)
+      IdentityMap[ChildFunclet] = ChildIdentity = ChildFunclet;
+    for (BasicBlock *Ancestor : FuncletPath) {
+      BasicBlock *AncestorIdentity = IdentityMap[Ancestor];
+      if (!AncestorIdentity)
+        IdentityMap[Ancestor] = AncestorIdentity = Ancestor;
+      if (AncestorIdentity == ChildIdentity) {
+        IsCyclic = true;
+        break;
+      }
+    }
+    // If the unwind chain wraps back on itself, break the chain.
+    if (IsCyclic) {
+      makeFuncletEdgeUnreachable(CurFunclet, ChildFunclet);
+      continue;
+    }
+    // If this child funclet has other parents, clone the entire funclet.
+    if (FuncletParents[ChildFunclet].size() > 1) {
+      ChildFunclet = cloneFuncletForParent(F, ChildFunclet, CurFunclet);
+      IdentityMap[ChildFunclet] = ChildIdentity;
+    }
+    FuncletPath.push_back(ChildFunclet);
+    resolveFuncletAncestryForPath(F, FuncletPath, IdentityMap);
+    FuncletPath.pop_back();
+  }
+}
+
 void WinEHPrepare::demotePHIsOnFunclets(Function &F) {
   // Strip PHI nodes off of EH pads.
   SmallVector<PHINode *, 16> PHINodes;
   for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE;) {
     BasicBlock *BB = FI++;
     if (!BB->isEHPad())
       continue;
     for (BasicBlock::iterator BI = BB->begin(), BE = BB->end(); BI != BE;) {
void WinEHPrepare::cloneCommonBlocks(
   for (BasicBlock *FuncletPadBB : EntryBlocks) {
     std::set<BasicBlock *> &BlocksInFunclet = FuncletBlocks[FuncletPadBB];
 
     std::map<BasicBlock *, BasicBlock *> Orig2Clone;
     ValueToValueMapTy VMap;
     for (BasicBlock *BB : BlocksInFunclet) {
       std::set<BasicBlock *> &ColorsForBB = BlockColors[BB];
       // We don't need to do anything if the block is monochromatic.
       size_t NumColorsForBB = ColorsForBB.size();
       if (NumColorsForBB == 1)
         continue;
 
       // Create a new basic block and copy instructions into it!
       BasicBlock *CBB =
           CloneBasicBlock(BB, VMap, Twine(".for.", FuncletPadBB->getName()));
       // Insert the clone immediately after the original to ensure determinism
       // and to keep the same relative ordering of any funclet's blocks.
for (auto &BBMapping : Orig2Clone) {
       BlockColors[OldBlock].erase(FuncletPadBB);
     }
 
     // Loop over all of the instructions in the function, fixing up operand
     // references as we go.  This uses VMap to do all the hard work.
     for (BasicBlock *BB : BlocksInFunclet)
       // Loop over all instructions, fixing each one as we find it...
       for (Instruction &I : *BB)
         RemapInstruction(&I, VMap, RF_IgnoreMissingEntries);
 
     // Check to see if SuccBB has PHI nodes. If so, we need to add entries to
     // the PHI nodes for NewBB now.
     for (auto &BBMapping : Orig2Clone) {
       BasicBlock *OldBlock = BBMapping.first;
       BasicBlock *NewBlock = BBMapping.second;
       for (BasicBlock *SuccBB : successors(NewBlock)) {
         for (Instruction &SuccI : *SuccBB) {
           auto *SuccPN = dyn_cast<PHINode>(&SuccI);
if (!DisableDemotion) {
 
     demoteUsesBetweenFunclets(F);
 
     demoteArgumentUses(F);
   }
 
   cloneCommonBlocks(F, EntryBlocks);
 
+  resolveFuncletAncestry(F);
+
   if (!DisableCleanups) {
     removeImplausibleTerminators(F);
 
     cleanupPreparedFunclets(F);
   }
 
   verifyPreparedFunclets(F);
 
   BlockColors.clear();
   FuncletBlocks.clear();
   FuncletChildren.clear();
+  FuncletParents.clear();
 
   return true;
 }
 
 // TODO: Share loads when one use dominates another, or when a catchpad exit
 // dominates uses (needs dominators).
 AllocaInst *WinEHPrepare::insertPHILoads(PHINode *PN, Function &F) {
   BasicBlock *PHIBlock = PN->getParent();

lib/Transforms/Utils/Local.cpp

if (DT) {
     BasicBlock *PredBBIDom = DT->getNode(PredBB)->getIDom()->getBlock();
     DT->changeImmediateDominator(DestBB, PredBBIDom);
     DT->eraseNode(PredBB);
   }
   // Nuke BB.
   PredBB->eraseFromParent();
 }
 
+
+void llvm::convertInvokeToCall(InvokeInst *II) {
+  SmallVector<Value*, 8> Args(II->op_begin(), II->op_end() - 3);
+  // Insert a call instruction before the invoke.
+  CallInst *Call = CallInst::Create(II->getCalledValue(), Args, "", II);
+  Call->takeName(II);
+  Call->setCallingConv(II->getCallingConv());
+  Call->setAttributes(II->getAttributes());
+  Call->setDebugLoc(II->getDebugLoc());
+
+  // Anything that used the value produced by the invoke instruction now uses
+  // the value produced by the call instruction.  Note that we do this even
+  // for void functions and calls with no uses so that the callgraph edge is
+  // updated.
+  II->replaceAllUsesWith(Call);
+  II->getUnwindDest()->removePredecessor(II->getParent());
+
+  // Insert a branch to the normal destination right before the invoke.
+  BranchInst::Create(II->getNormalDest(), II);
+
+  // Finally, delete the invoke instruction!
+  II->eraseFromParent();
+}
+
+
 /// CanMergeValues - Return true if we can choose one of these values to use
 /// in place of the other. Note that we will always choose the non-undef
 /// value to keep.
 static bool CanMergeValues(Value *First, Value *Second) {
   return First == Second || isa<UndefValue>(First) || isa<UndefValue>(Second);
 }
 
 /// CanPropagatePredecessorsForPHIs - Return true if we can fold BB, an
static void changeToUnreachable(Instruction *I, bool UseLLVMTrap) {
   BasicBlock::iterator BBI = I, BBE = BB->end();
   while (BBI != BBE) {
     if (!BBI->use_empty())
       BBI->replaceAllUsesWith(UndefValue::get(BBI->getType()));
     BB->getInstList().erase(BBI++);
   }
 }
 
-/// changeToCall - Convert the specified invoke into a normal call.
-static void changeToCall(InvokeInst *II) {
-  SmallVector<Value*, 8> Args(II->op_begin(), II->op_end() - 3);
-  CallInst *NewCall = CallInst::Create(II->getCalledValue(), Args, "", II);
-  NewCall->takeName(II);
-  NewCall->setCallingConv(II->getCallingConv());
-  NewCall->setAttributes(II->getAttributes());
-  NewCall->setDebugLoc(II->getDebugLoc());
-  II->replaceAllUsesWith(NewCall);
-
-  // Follow the call by a branch to the normal destination.
-  BranchInst::Create(II->getNormalDest(), II);
-
-  // Update PHI nodes in the unwind destination
-  II->getUnwindDest()->removePredecessor(II->getParent());
-  II->eraseFromParent();
-}
-
 static bool markAliveBlocks(Function &F,
                             SmallPtrSetImpl<BasicBlock*> &Reachable) {
 
   SmallVector<BasicBlock*, 128> Worklist;
   BasicBlock *BB = F.begin();
   Worklist.push_back(BB);
   Reachable.insert(BB);
   bool Changed = false;
if (InvokeInst *II = dyn_cast<InvokeInst>(BB->getTerminator())) {
         Changed = true;
       } else if (II->doesNotThrow() && canSimplifyInvokeNoUnwind(&F)) {
         if (II->use_empty() && II->onlyReadsMemory()) {
           // jump to the normal destination branch.
           BranchInst::Create(II->getNormalDest(), II);
           II->getUnwindDest()->removePredecessor(II->getParent());
           II->eraseFromParent();
         } else
-          changeToCall(II);
+          convertInvokeToCall(II);
         Changed = true;
       }
     }
 
     Changed |= ConstantFoldTerminator(BB, true);
     for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI != SE; ++SI)
       if (Reachable.insert(*SI).second)
         Worklist.push_back(*SI);
   } while (!Worklist.empty());
   return Changed;
 }
 
 void llvm::removeUnwindEdge(BasicBlock *BB) {
   TerminatorInst *TI = BB->getTerminator();
 
   if (auto *II = dyn_cast<InvokeInst>(TI)) {
-    changeToCall(II);
+    convertInvokeToCall(II);
     return;
   }
 
   TerminatorInst *NewTI;
   BasicBlock *UnwindDest;
 
   if (auto *CRI = dyn_cast<CleanupReturnInst>(TI)) {
     NewTI = CleanupReturnInst::Create(CRI->getCleanupPad(), nullptr, CRI);

test/CodeGen/WinEH/wineh-cloning.ll

 loop:
   %i.loop = phi i32 [ %i, %shared ], [ %i.dec, %loop.tail ]
   %b = call i1 @b()
   br i1 %b, label %left, label %right
 left:
   %y = call i32 @g()
   br label %loop.tail
 right:
   call void @h(i32 %x)
   br label %loop.tail
 loop.tail:
   %i.dec = sub i32 %i.loop, 1
   %done = icmp eq i32 %i.dec, 0
   br i1 %done, label %exit, label %loop
 exit:
   call void @h(i32 %x)
   unreachable

 ; unwinds to the catchendpad for %right, so the copy
 ; of %inner under %right should include it; the copy
 ; of %inner under %left should instead have an
 ; `unreachable` inserted there, but the copy under
 ; %left still needs to be created because it's possible
 ; the dynamic path enters %left, then enters %inner,
 ; then calls @h, and that the call to @h doesn't return.
 ; CHECK-LABEL: define void @test6(
-; TODO: CHECKs
+; CHECK:     left:
+; CHECK:           to label %[[SHARED_CONT_LEFT:.+]] unwind label %[[INNER_LEFT:.+]]
+; CHECK:     right:
+; CHECK:           to label %right.catch unwind label %right.end
+; CHECK:     right.catch:
+; CHECK:       %x = call i32 @g()
+; CHECK:       store i32 %x, i32* %x.wineh.spillslot
+; CHECK:           to label %shared.cont unwind label %[[INNER_RIGHT:.+]]
+; CHECK:     right.end:
+; CHECK:       catchendpad unwind to caller
+; CHECK:     shared.cont:
+; CHECK:       unreachable
+; CHECK:     [[SHARED_CONT_LEFT]]:
+; CHECK:       unreachable
+; CHECK:     [[INNER_RIGHT]]:
+; CHECK:       [[I_R:\%.+]] = cleanuppad []
+; CHECK:       [[X_RELOAD_R:\%.+]] = load i32, i32* %x.wineh.spillslot
+; CHECK:       call void @h(i32 [[X_RELOAD_R]])
+; CHECK:       cleanupret [[I_R:\%.+]] unwind label %right.end
+; CHECK:     [[INNER_LEFT]]:
+; CHECK:       [[I_L:\%.+]] = cleanuppad []
+; CHECK:       [[X_RELOAD_L:\%.+]] = load i32, i32* %x.wineh.spillslot
+; CHECK:       call void @h(i32 [[X_RELOAD_L]])
+; CHECK:       unreachable
 
 
 define void @test7() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %invoke.cont unwind label %left
 invoke.cont:
   invoke void @f()
inner:
   cleanupret %i unwind label %right.end
 unreachable:
   unreachable
 }
 ; Another case of a two-parent child (like @test6), this time
 ; with the join at the entry itself instead of following a
 ; non-pad join.
 ; CHECK-LABEL: define void @test7(
-; TODO: CHECKs
+; CHECK:     invoke.cont:
+; CHECK:           to label %[[UNREACHABLE_ENTRY:.+]] unwind label %right
+; CHECK:     left:
+; CHECK:           to label %[[UNREACHABLE_LEFT:.+]] unwind label %[[INNER_LEFT:.+]]
+; CHECK:     right:
+; CHECK:           to label %right.catch unwind label %right.end
+; CHECK:     right.catch:
+; CHECK:           to label %unreachable unwind label %[[INNER_RIGHT:.+]]
+; CHECK:     right.end:
+; CHECK:       catchendpad unwind to caller
+; CHECK:     [[INNER_RIGHT]]:
+; CHECK:       [[I_R:\%.+]] = cleanuppad []
+; CHECK:       [[X_R:\%.+]] = call i32 @g()
+; CHECK:       call void @h(i32 [[X_R]])
+; CHECK:       cleanupret [[I_R]] unwind label %right.end
+; CHECK:     [[INNER_LEFT]]:
+; CHECK:       [[I_L:\%.+]] = cleanuppad []
+; CHECK:       [[X_L:\%.+]] = call i32 @g()
+; CHECK:       call void @h(i32 [[X_L]])
+; CHECK:       unreachable
+; CHECK:     unreachable:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_LEFT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_ENTRY]]:
+; CHECK:       unreachable
 
 
 define void @test8() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %invoke.cont unwind label %left
 invoke.cont:
   invoke void @f()
inner.child:
   call void @h(i32 %x)
   unreachable
 unreachable:
   unreachable
 }
 ; %inner is a two-parent child which itself has a child; need
 ; to make two copies of both the %inner and %inner.child.
 ; CHECK-LABEL: define void @test8(
-; TODO: CHECKs
+; CHECK:     invoke.cont:
+; CHECK:               to label %[[UNREACHABLE_ENTRY:.+]] unwind label %right
+; CHECK:     left:
+; CHECK:               to label %[[UNREACHABLE_LEFT:.+]] unwind label %[[INNER_LEFT:.+]]
+; CHECK:     right:
+; CHECK:               to label %right.catch unwind label %right.end
+; CHECK:     right.catch:
+; CHECK:               to label %[[UNREACHABLE_RIGHT:.+]] unwind label %[[INNER_RIGHT:.+]]
+; CHECK:     right.end:
+; CHECK:       catchendpad unwind to caller
+; CHECK:     [[INNER_RIGHT]]:
+; CHECK:               to label %[[UNREACHABLE_INNER_RIGHT:.+]] unwind label %[[INNER_CHILD_RIGHT:.+]]
+; CHECK:     [[INNER_LEFT]]:
+; CHECK:               to label %[[UNREACHABLE_INNER_LEFT:.+]] unwind label %[[INNER_CHILD_LEFT:.+]]
+; CHECK:     [[INNER_CHILD_RIGHT]]:
+; CHECK:       [[TMP:\%.+]] = cleanuppad []
+; CHECK:       [[X:\%.+]] = call i32 @g()
+; CHECK:       call void @h(i32 [[X]])
+; CHECK:       unreachable
+; CHECK:     [[INNER_CHILD_LEFT]]:
+; CHECK:       [[TMP:\%.+]] = cleanuppad []
+; CHECK:       [[X:\%.+]] = call i32 @g()
+; CHECK:       call void @h(i32 [[X]])
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_INNER_RIGHT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_INNER_LEFT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_RIGHT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_LEFT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_ENTRY]]:
+; CHECK:       unreachable
 
 
 define void @test9() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %invoke.cont unwind label %left
 invoke.cont:
   invoke void @f()

 ; other a child of the opposite funclet), but also make sure not to
 ; clone self-descendants (if we tried to do that we'd need to make an
 ; infinite number of them).  Presumably if optimizations ever generated
 ; such a thing it would mean that one of the two cleanups was originally
 ; the parent of the other, but that we'd somehow lost track in the CFG
 ; of which was which along the way; generating each possibility lets
 ; whichever case was correct execute correctly.
 ; CHECK-LABEL: define void @test9(
-; TODO: CHECKs
+; CHECK:     entry:
+; CHECK:               to label %invoke.cont unwind label %[[LEFT:.+]]
+; CHECK:     invoke.cont:
+; CHECK:               to label %[[UNREACHABLE_ENTRY:.+]] unwind label %[[RIGHT:.+]]
+; CHECK:     [[LEFT_FROM_RIGHT_UNREACHABLE:.+]]:
+; CHECK-NEXT:  cleanuppad []
+; CHECK-NEXT:  unreachable
+; CHECK:     [[LEFT_FROM_RIGHT:.+]]:
+; CHECK:       call void @h(i32 1)
+; CHECK:       invoke void @f()
+; CHECK:               to label %[[UNREACHABLE_LEFT_FROM_RIGHT:.+]] unwind label %[[RIGHT_FROM_LEFT_UNREACHABLE:.+]]
+; CHECK:     [[LEFT]]:
+; CHECK:       call void @h(i32 1)
+; CHECK:       invoke void @f()
+; CHECK:               to label %[[UNREACHABLE_LEFT:.+]] unwind label %[[RIGHT_FROM_LEFT:.+]]
+; CHECK:     [[RIGHT_FROM_LEFT_UNREACHABLE]]:
+; CHECK-NEXT:  cleanuppad []
+; CHECK-NEXT:  unreachable
+; CHECK:     [[RIGHT]]:
+; CHECK:       call void @h(i32 2)
+; CHECK:       invoke void @f()
+; CHECK:               to label %[[UNREACHABLE_RIGHT:.+]] unwind label %[[LEFT_FROM_RIGHT]]
+; CHECK:     [[RIGHT_FROM_LEFT]]:
+; CHECK:       call void @h(i32 2)
+; CHECK:       invoke void @f()
+; CHECK:               to label %[[UNREACHABLE_RIGHT_FROM_LEFT:.+]] unwind label %[[LEFT_FROM_RIGHT_UNREACHABLE]]
+; CHECK:     [[UNREACHABLE_RIGHT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_RIGHT_FROM_LEFT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_LEFT_FROM_RIGHT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_LEFT]]:
+; CHECK:       unreachable
+; CHECK:     [[UNREACHABLE_ENTRY]]:
+; CHECK:       unreachable
+
 
 define void @test10() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %unreachable unwind label %inner
 inner:
   %cleanup = cleanuppad []
   ; make sure we don't overlook this cleanupret and try to process

test/CodeGen/WinEH/wineh-demotion.ll

 
 catch.inner:
   ; Need just one store here because only %y is affected
   ; CHECK: catch.inner:
   %z = call i32 @g()
   ; CHECK:   store i32 %z
   ; CHECK-NEXT: invoke void @f
   invoke void @f()
-          to label %catchret.inner unwind label %merge.outer
+          to label %catchret.inner unwind label %catchend.inner
 
 catchret.inner:
   catchret %cpinner to label %exit
 catchend.inner:
+  ; CHECK-NOT: = phi
+  %y = phi i32 [ %x, %merge.inner ], [ %z, %catch.inner ]
   catchendpad unwind label %merge.outer
 
 merge.outer:
   ; CHECK: merge.outer:
-  ; CHECK-NOT: = phi
   ; CHECK: [[CatchPad:%[^ ]+]] = catchpad []
-  %y = phi i32 [ %x, %catchend.inner ], [ %z, %catch.inner ]
   %cpouter = catchpad [] to label %catch.outer unwind label %catchend.outer
 
 catchend.outer:
   catchendpad unwind to caller
 
 catch.outer:
   ; Need to load x and y from two different slots since they're both live
   ; and can have different values (if we came from catch.inner)

test/CodeGen/WinEH/wineh-no-demotion.ll

shared:
   %x = call i32 @g()
   invoke void @f()
           to label %shared.cont unwind label %inner
 
 shared.cont:
   unreachable
 
 inner:
-  ; CHECK: %phi = phi i32 [ %x, %right ], [ 0, %invoke.cont2 ], [ %x.for.left, %left ]
   %phi = phi i32 [ %x, %shared ], [ 0, %invoke.cont2 ]
   %i = cleanuppad []
   call void @h(i32 %phi)
   unreachable
 
+; CHECK [[INNER_INVOKE_CONT2:inner.*]]:
+  ; CHECK: call void @h(i32 0)
+
+; CHECK [[INNER_RIGHT:inner.*]]:
+  ; CHECK: call void @h(i32 %x)
+
+; CHECK [[INNER_LEFT:inner.*]]:
+  ; CHECK: call void @h(i32 %x.for.left)
+
 exit:
   unreachable
 }
 
 ; CHECK-LABEL: @test2(
 define void @test2() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
shared:
   %x = call i32 @g()
   invoke void @f()
           to label %shared.cont unwind label %inner
 
 shared.cont:
   unreachable
 
 inner:
-  ; CHECK: %x1 = phi i32 [ %x.for.left, %left ], [ %x, %right ]
-  ; CHECK: call void @h(i32 %x1)
   %i = cleanuppad []
   call void @h(i32 %x)
   unreachable
 
+; CHECK [[INNER_RIGHT:inner.*]]:
+  ; CHECK: call void @h(i32 %x)
+
+; CHECK [[INNER_LEFT:inner.*]]:
+  ; CHECK: call void @h(i32 %x.for.left)
+
 exit:
   unreachable
 }
 
 ; CHECK-LABEL: @test3(
 define void @test3() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()