[PATCH] D13274: [WinEH] Clone funclets with multiple parents
Joseph Tremoulet via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 21 15:26:18 PDT 2015
JosephTremoulet added a comment.
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.
================
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
----------------
I don't understand this comment; we call this for more than just catchendpads...
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:724
@@ +723,3 @@
+ dyn_cast<CatchPadInst>(CloneParent->getFirstNonPHI());
+ if (CloneParentCatch && CloneParentCatch->getUnwindDest() == UnwindDest) {
+ DEBUG_WITH_TYPE("winehprepare-coloring",
----------------
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.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:736
@@ +735,3 @@
+ } else if (auto *CleanupEnd = dyn_cast<CleanupEndPadInst>(EHPadInst)) {
+ // If the catchendpad unwinds to a cleanupendpad, that cleanupendpad
+ // must be ending a cleanuppad of either our clone parent or one
----------------
Confusing comment again; the thing that unwinds is an arbitrary EH pad, not necessarily a catchendpad, right?
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:762
@@ +761,3 @@
+ } else {
+ // If the catchendpad unwinds to a catchpad, cleanuppad or
+ // terminatepad that EH pad must be a sibling of the funclet we're
----------------
same as above
================
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) {
----------------
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.
================
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.
+//
----------------
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?
================
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
----------------
does "caller" here mean the same thing as "parent"?
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:945
@@ +944,3 @@
+ CloneParent);
+ }
+}
----------------
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.
================
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);
----------------
Could this be `assert(!FuncletParents[ClonedFunclet].count(Parent))`?
================
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()) {
----------------
This comment doesn't match the code (but the comment on line 1075 does) -- siblings aren't handled 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))
----------------
Is cleanupendpad ever a parent-to-child edge? I'd have thought no.
================
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)
----------------
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)?
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1225
@@ +1224,3 @@
+ BasicBlock *MappedFunclet = Mapping.first;
+ if (FuncletParents[MappedFunclet].size() == 1 &&
+ FuncletParents[MappedFunclet].front() == CurFunclet) {
----------------
As on line 1212, I'd have thought we could assume/assert that siblings have a single parent here.
================
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.
----------------
You can remove this now, right?
================
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.
----------------
Worth asserting `FuncletCloningRequired`?
================
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;
+ }
----------------
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?
================
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.
----------------
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`?
================
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");
----------------
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.
================
Comment at: test/CodeGen/WinEH/wineh-cloning.ll:301
@@ +300,3 @@
+; CHECK: call void @h(i32 [[X_RELOAD_R]])
+; CHECK: cleanupret [[I_R:\%.+]] unwind label %right.end
+; CHECK: [[INNER_LEFT]]:
----------------
Looks like a copy-paste error: I think you want `[[I_R]]` here instead of `[[I_R:\%.+]]`
================
Comment at: test/CodeGen/WinEH/wineh-multi-parent-cloning.ll:71
@@ +70,3 @@
+; CHECK: call void @h(i32 [[X_RELOAD_R]])
+; CHECK: cleanupret [[I_R:\%.+]] unwind label %right.end
+; CHECK: [[INNER_LEFT]]:
----------------
`[[I_R]]`
================
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
----------------
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.
================
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(
----------------
Did we stop making this clone with rL250534?
================
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(
----------------
Why does that happen?
Repository:
rL LLVM
http://reviews.llvm.org/D13274
More information about the llvm-commits
mailing list