[PATCH] D13274: [WinEH] Clone funclets with multiple parents
Andy Kaylor via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 22 13:50:21 PDT 2015
andrew.w.kaylor added inline comments.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:717
@@ +716,3 @@
+ assert(UnwindDest->isEHPad());
+ // There are many places to which a catchendpad can unwind and each has
+ // slightly different rules for whether or not it fits with the given
----------------
JosephTremoulet wrote:
> I don't understand this comment; we call this for more than just catchendpads...
Oops. Slipped through refactoring.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:724
@@ +723,3 @@
+ dyn_cast<CatchPadInst>(CloneParent->getFirstNonPHI());
+ if (CloneParentCatch && CloneParentCatch->getUnwindDest() == UnwindDest) {
+ DEBUG_WITH_TYPE("winehprepare-coloring",
----------------
JosephTremoulet wrote:
> 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.
Yes, you're right. I'll add a test case to cover that.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:811
@@ +810,3 @@
+ // first parent that has the right ancestry.
+ // FIXME: Find a suitable parent.
+ for (auto *Parent : OrigParents) {
----------------
JosephTremoulet wrote:
> 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.
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:856-860
@@ +855,7 @@
+//
+// If the terminator is a catchret its successor is a block in its parent
+// funclet. If the instruction returns to a block in the parent for which the
+// cloned funclet was created, the terminator in the original block must be
+// replaced by an unreachable instruction. Otherwise the terminator in the
+// clone block must be replaced by an unreachable instruction.
+//
----------------
JosephTremoulet wrote:
> 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?
I don't think so. I'll add something.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:862-865
@@ +861,6 @@
+//
+// If the terminator is a cleanupret or cleanupendpad it unwinds to either its
+// caller, a sibling EH pad, a cleanup end pad in its parent funclet or a catch
+// end pad in its grandparent funclet (which must be coupled with the parent
+// funclet). If it unwinds to its caller, there is nothing to be done.
+// If the unwind destination is a sibling EH pad, we will update the terminators
----------------
JosephTremoulet wrote:
> does "caller" here mean the same thing as "parent"?
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:945
@@ +944,3 @@
+ CloneParent);
+ }
+}
----------------
JosephTremoulet wrote:
> 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.
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....
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1043-1045
@@ +1042,5 @@
+ // 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);
----------------
JosephTremoulet wrote:
> Could this be `assert(!FuncletParents[ClonedFunclet].count(Parent))`?
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1062-1063
@@ +1061,4 @@
+ // Find any blocks that unwound to the original funclet entry from either the
+ // clone parent block or a child of the cloned parent (and therefore a sibling
+ // of the block we just cloned) and remap them to the clone.
+ for (auto *U : FuncletEntry->users()) {
----------------
JosephTremoulet wrote:
> This comment doesn't match the code (but the comment on line 1075 does) -- siblings aren't handled here.
Yeah, I'm not sure what I was thinking here.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1138-1139
@@ +1137,4 @@
+ UnwindDest = I->getUnwindDest();
+ else if (auto *I = dyn_cast<CleanupEndPadInst>(Terminator))
+ UnwindDest = I->getUnwindDest();
+ else if (auto *I = dyn_cast<TerminatePadInst>(Terminator))
----------------
JosephTremoulet wrote:
> Is cleanupendpad ever a parent-to-child edge? I'd have thought no.
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1212
@@ +1211,3 @@
+ // If it is a sibling, no action is needed.
+ if (FuncletParents[UnwindDest].size() == 1 &&
+ FuncletParents[UnwindDest].front() == CurFunclet)
----------------
JosephTremoulet wrote:
> 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)?
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1354
@@ -680,3 +1353,3 @@
std::map<BasicBlock *, std::set<BasicBlock *>> FuncletBlocks;
std::map<BasicBlock *, std::set<BasicBlock *>> FuncletChildren;
// Figure out which basic blocks belong to which funclets.
----------------
JosephTremoulet wrote:
> You can remove this now, right?
Yes.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1465-1466
@@ +1464,4 @@
+ // If this block is a catchendpad, it shouldn't be cloned.
+ // We will only see a catchendpad with multiple colors in the case where
+ // some funclet has multiple parents. In that case, the color will be
+ // resolved during the resolveFuncletAncestry processing.
----------------
JosephTremoulet wrote:
> Worth asserting `FuncletCloningRequired`?
Yes, probably so.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1474-1485
@@ +1473,14 @@
+ if (auto *CEP = dyn_cast<CatchEndPadInst>(BB->getFirstNonPHI())) {
+ BlockColors[BB].clear();
+ BasicBlock *CatchParent = nullptr;
+ // There can only be one catchpad predecessor for a catchendpad.
+ for (BasicBlock *PredBB : predecessors(BB)) {
+ if (isa<CatchPadInst>(PredBB->getTerminator())) {
+ CatchParent = PredBB;
+ }
+ }
+ // There must be one catchpad predecessor for a catchendpad.
+ assert(CatchParent);
+ BlockColors[BB].insert(FuncletParents[CatchParent].front());
+ continue;
+ }
----------------
JosephTremoulet wrote:
> 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?
Yeah, probably so.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1476-1481
@@ +1475,8 @@
+ BasicBlock *CatchParent = nullptr;
+ // There can only be one catchpad predecessor for a catchendpad.
+ for (BasicBlock *PredBB : predecessors(BB)) {
+ if (isa<CatchPadInst>(PredBB->getTerminator())) {
+ CatchParent = PredBB;
+ }
+ }
+ // There must be one catchpad predecessor for a catchendpad.
----------------
JosephTremoulet wrote:
> 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`?
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1529-1530
@@ +1528,4 @@
+ DEBUG_WITH_TYPE("winehprepare-coloring",
+ dbgs() << " Removed color \'" << FuncletPadBB->getName()
+ << "\' from block \'" << OldBlock->getName()
+ << "\'.\n");
----------------
JosephTremoulet wrote:
> 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.
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.
================
Comment at: test/CodeGen/WinEH/wineh-multi-parent-cloning.ll:113-114
@@ +112,4 @@
+}
+; In this case left and right are both parents of inner, while entry and left
+; are both parents of right. This differs from @test1 in that inner is a
+; catchpad rather than a cleanuppad, which makes inner.end a block that gets
----------------
JosephTremoulet wrote:
> 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.
This comments seems to be left over from an earlier version of the test.
================
Comment at: test/CodeGen/WinEH/wineh-multi-parent-cloning.ll:119-121
@@ +118,5 @@
+; The catchendpad in %inner.end unwinds to %right.end (which belongs to the
+; entry funclet). A clone of %right.end will be made for the path from
+; the clone of %inner.end made for the %left funclet, but because %left is
+; not a catchpad, this should be removed as an implausible terminator.
+; CHECK-LABEL: define void @test2(
----------------
JosephTremoulet wrote:
> Did we stop making this clone with rL250534?
Yes, I think so.
================
Comment at: test/CodeGen/WinEH/wineh-multi-parent-cloning.ll:506-507
@@ +505,4 @@
+; %left.end.
+; Note: An extra clone of %right is created during the processing of this
+; function, but it becomes unreachable and is eliminated.
+; CHECK-LABEL: define void @test7(
----------------
JosephTremoulet wrote:
> Why does that happen?
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.
Repository:
rL LLVM
http://reviews.llvm.org/D13274
More information about the llvm-commits
mailing list