[WinEH] Fix endpad coloring/numbering

[JosephTremoulet]

Summary

When a cleanup's cleanupendpad or cleanupret targets a catchendpad, stop
trying to propagate the cleanup's parent's color to the catchendpad, since
what's needed is the cleanup's grandparent's color and the catchendpad
will get that color from the catchpad linkage already. We already had
this exclusion for invokes, but were missing it for
cleanupendpad/cleanupret.

Also add a missing line that tags cleanupendpads' states in the
EHPadStateMap, without with lowering invokes that target cleanupendpads
which unwind to other handlers (and so don't have the -1 state) will fail.

This fixes the reduced IR repro in PR25163.

[andrew.w.kaylor]

Looks good to me.

I noticed this extra cloning while working on the multi-parent cloning, but it seemed like the extra cloned blocks were generally cleared away by the cleanup code (at least in the cases I was debugging).

[JosephTremoulet]

I noticed this extra cloning while working on the multi-parent cloning, but it seemed like the extra cloned blocks were generally cleared away by the cleanup code (at least in the cases I was debugging).

Yeah, the trick to get an extra catchendpad to stick around (which PR25163 uncovered) is that a catch chained to the catchendpad needs to contain both an inner cleanup whose cleanupret/cleanupendpad unwinds to the catchendpad (so we give it the extra color) and an invoke, inside the catch but outside its cleanup, which also unwinds to the catchendpad (so that cloning will rewrite the invoke unwind dest to the cloned catchendpad).

[rnk]

lgtm as well. However, it makes me feel like we overfit the C++ personality with the catchendpad design.

lib/CodeGen/WinEHPrepare.cpp

for (const BasicBlock *PredBlock : predecessors(&BB))
       if ((PredBlock = getEHPadFromPredecessor(PredBlock)))
         calculateExplicitCXXStateNumbers(FuncInfo, *PredBlock, CleanupState);
   } else if (auto *CEPI = dyn_cast<CleanupEndPadInst>(FirstNonPHI)) {
     // Propagate ParentState to the cleanuppad in case it doesn't have
     // any cleanuprets.
     BasicBlock *CleanupBlock = CEPI->getCleanupPad()->getParent();
     calculateExplicitCXXStateNumbers(FuncInfo, *CleanupBlock, ParentState);
     // Anything unwinding through CleanupEndPadInst is in ParentState.
+    FuncInfo.EHPadStateMap[FirstNonPHI] = ParentState;
     for (const BasicBlock *PredBlock : predecessors(&BB))
       if ((PredBlock = getEHPadFromPredecessor(PredBlock)))
         calculateExplicitCXXStateNumbers(FuncInfo, *PredBlock, ParentState);
   } else if (isa<TerminatePadInst>(FirstNonPHI)) {
     report_fatal_error("Not yet implemented!");
   } else {
     llvm_unreachable("unexpected EH Pad!");
   }
while (!Worklist.empty()) {
     BasicBlock *Visiting;
     BasicBlock *Color;
     std::tie(Visiting, Color) = Worklist.pop_back_val();
     Instruction *VisitingHead = Visiting->getFirstNonPHI();
     if (VisitingHead->isEHPad() && !isa<CatchEndPadInst>(VisitingHead) &&
         !isa<CleanupEndPadInst>(VisitingHead)) {
       // Mark this as a funclet head as a member of itself.
       FuncletBlocks[Visiting].insert(Visiting);
-      // Queue exits with the parent color.
+      // Queue exits (i.e. successors of rets/endpads) with the parent color.
+      // Skip any exits that are catchendpads, since the parent color must then
+      // represent one of the catches chained to that catchendpad, but the
+      // catchendpad should get the color of the common parent of all its
+      // chained catches (i.e. the grandparent color of the current pad).
+      // We don't need to worry abou catchendpads going unvisited, since the
+      // catches chained to them must have unwind edges to them by which we will
+      // visit them.
       for (User *U : VisitingHead->users()) {
         if (auto *Exit = dyn_cast<TerminatorInst>(U)) {
           for (BasicBlock *Succ : successors(Exit->getParent()))
+            if (!isa<CatchEndPadInst>(*Succ->getFirstNonPHI()))
               if (BlockColors[Succ].insert(Color).second)
                 Worklist.push_back({Succ, Color});
         }
       }
       // Handle CatchPad specially since its successors need different colors.
       if (CatchPadInst *CatchPad = dyn_cast<CatchPadInst>(VisitingHead)) {
         // Visit the normal successor with the color of the new EH pad, and
         // visit the unwind successor with the color of the parent.
         BasicBlock *NormalSucc = CatchPad->getNormalDest();
         if (BlockColors[NormalSucc].insert(Visiting).second) {

test/CodeGen/WinEH/wineh-cloning.ll

 entry:
   ret void
 
 unreachable:
   cleanuppad []
   unreachable
 }
 
+define void @test14() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  invoke void @f()
+    to label %exit unwind label %catch1.pad
+catch1.pad:
+  %catch1 = catchpad [i32 1]
+    to label %catch1.body unwind label %catch2.pad
+catch1.body:
+  invoke void @h(i32 1)
+    to label %catch1.body2 unwind label %catch.end
+catch1.body2:
+  invoke void @f()
+    to label %catch1.ret unwind label %cleanup1.pad
+cleanup1.pad:
+  %cleanup1 = cleanuppad []
+  call void @f()
+  cleanupret %cleanup1 unwind label %catch.end
+catch1.ret:
+  catchret %catch1 to label %exit
+catch2.pad:
+  %catch2 = catchpad [i32 2]
+    to label %catch2.body unwind label %catch.end
+catch2.body:
+  invoke void @h(i32 2)
+    to label %catch2.body2 unwind label %catch.end
+catch2.body2:
+  invoke void @f()
+    to label %catch2.ret unwind label %cleanup2.pad
+cleanup2.pad:
+  %cleanup2 = cleanuppad []
+  call void @f()
+  cleanupret %cleanup2 unwind label %catch.end
+catch2.ret:
+  catchret %catch2 to label %exit
+catch.end:
+  catchendpad unwind to caller
+exit:
+  ret void
+}
+; Make sure we don't clone the catchendpad even though the
+; cleanupendpads targeting it would naively imply that it
+; should get their respective parent colors (catch1 and catch2),
+; as well as its properly getting the root function color.  The
+; references from the invokes ensure that if we did make clones
+; for each catch, they'd be reachable, as those invokes would get
+; rewritten
+; CHECK-LABEL: define void @test14()
+; CHECK-NOT:  catchendpad
+; CHECK:      invoke void @h(i32 1)
+; CHECK-NEXT:   unwind label %catch.end
+; CHECK-NOT:  catchendpad
+; CHECK:      invoke void @h(i32 2)
+; CHECK-NEXT:   unwind label %catch.end
+; CHECK-NOT:   catchendpad
+; CHECK:     catch.end:
+; CHECK-NEXT:  catchendpad
+; CHECK-NOT:   catchendpad
+
+;; Debug info (from test12)
+
 ; Make sure the DISubprogram doesn't get cloned
 ; CHECK-LABEL: !llvm.module.flags
 ; CHECK-NOT: !DISubprogram
 ; CHECK: !{{[0-9]+}} = distinct !DISubprogram(name: "test12"
 ; CHECK-NOT: !DISubprogram
 !llvm.module.flags = !{!0}
 !llvm.dbg.cu = !{!1}