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

Joseph Tremoulet via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 26 14:06:11 PDT 2015


JosephTremoulet added inline comments.

================
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
----------------
andrew.w.kaylor wrote:
> 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.
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.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:945
@@ +944,3 @@
+                                    CloneParent);
+  }
+}
----------------
andrew.w.kaylor wrote:
> 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....
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...

================
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);
----------------
andrew.w.kaylor wrote:
> 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.
Oh, yeah, thanks for explaining.  No, I think what you have is just as easy to comprehend as std::count.

================
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.
----------------
andrew.w.kaylor wrote:
> 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.
FWIW, relying on the verifier makes sense to me.


Repository:
  rL LLVM

http://reviews.llvm.org/D13274





More information about the llvm-commits mailing list