[PATCH] D12433: [WinEH] Add cleanupendpad instruction

Joseph Tremoulet via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 28 08:43:33 PDT 2015


JosephTremoulet added inline comments.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:3308-3313
@@ -3294,3 +3307,8 @@
         IsUnreachableCleanupret = CRI->getCleanupPad() != CleanupPad;
-      if (IsUnreachableRet || IsUnreachableCatchret || IsUnreachableCleanupret) {
+      // The token consumed by a CleanupEndPadInst must match the funclet token.
+      bool IsUnreachableCleanupendpad = false;
+      if (auto *CEPI = dyn_cast<CleanupEndPadInst>(TI))
+        IsUnreachableCleanupendpad = CEPI->getCleanupPad() != CleanupPad;
+      if (IsUnreachableRet || IsUnreachableCatchret ||
+          IsUnreachableCleanupret || IsUnreachableCleanupendpad) {
         new UnreachableInst(BB->getContext(), TI);
----------------
This part is likely wrong or incomplete; I have some questions about how to proceed that I felt were best asked in the context of a code review:
 1. As written, this will leave an `unreachable` as the first instruction in a block that's the target of unwind edges, which doesn't verify.  I can see that if we were to run `SimplifyCFGOpt::SimplifyUnreachable` on it, it would rewrite any invoke predecessors as calls.  Does that mean we need to run SimplifyCFGOpt on the function?
 2. Should `SimplifyCFGOpt::SimplifyUnreachable` be extended to rewrite catchendpad/cleanupendpad/terminatepad/cleanupret predecessors as 'unwind to caller'?  Does it matter that doing so would make those predecessors 'mayThrow()' where they weren't before?  From the comment "This would be a good place to note that the call does not throw though" in SimplifyUnreachable, I'm guessing the same happens for the invoke->call rewrite so it's ok (or at least won't make things worse), but not sure I'm intepreting that correctly.
 3. I think we'll need to do something similar for unwind edges that target implausible catchendpads, but since the plausiblity check there is 'is this catchendpad in the parent funclet', I think that will need to happen after the treeifying duplication that makes "*the* parent funclet" well-defined.  [I guess this one was a comment, not a question.]


http://reviews.llvm.org/D12433





More information about the llvm-commits mailing list