[WinEH] Fix single-block cleanup coloring

[JosephTremoulet]

Summary

The coloring code in WinEHPrepare queues cleanuprets' successors with the
correct color (the parent one) when it sees their cleanuppad, and so later
when iterating successors knows to skip processing cleanuprets since
they've already been queued. This latter check was incorrectly under an
'else' condition and so inadvertently was not kicking in for single-block
cleanups. This change sinks the check out of the 'else' to fix the bug.

[JosephTremoulet]

Fix typo in comment

[rnk]

lgtm, thanks!

test/CodeGen/WinEH/wineh-cloning.ll

406

I always like to check for define void @test10 to avoid matching against some other call that might get added later.

[JosephTremoulet]

• tighten 'CHECK-LABEL's

lib/CodeGen/WinEHPrepare.cpp

if (VisitingHead->isEHPad() && !isa<CatchEndPadInst>(VisitingHead) &&
       // Switch color to the current node, except for terminate pads which
       // have no bodies and only unwind successors and so need their successors
       // visited with the color of the parent.
       if (!isa<TerminatePadInst>(VisitingHead))
         Color = Visiting;
     } else {
       // Note that this is a member of the given color.
       FuncletBlocks[Color].insert(Visiting);
-      TerminatorInst *Terminator = Visiting->getTerminator();
-      if (isa<CleanupReturnInst>(Terminator) ||
-          isa<CatchReturnInst>(Terminator) ||
-          isa<CleanupEndPadInst>(Terminator)) {
-        // These block's successors have already been queued with the parent
-        // color.
-        continue;
-      }
-    }
+    }
+
+      // These blocks' successors have already been queued with the parent
     for (BasicBlock *Succ : successors(Visiting)) {
       if (isa<CatchEndPadInst>(Succ->getFirstNonPHI())) {
         // The catchendpad needs to be visited with the parent's color, not
         // the current color.  This will happen in the code above that visits
         // any catchpad unwind successor with the parent color, so we can
         // safely skip this successor here.
         continue;
       }

test/CodeGen/WinEH/wineh-cloning.ll

noreturn:
   call void @h(i32 %x)
   unreachable
 endcatch:
   catchendpad unwind to caller
 }
 ; Need two copies of the call to @h, one under entry and one under catch.
 ; Currently we generate a load for each, though we shouldn't need one
 ; for the use in entry's copy.
-; CHECK-LABEL: @test1(
+; CHECK-LABEL: define void @test1(
 ; CHECK: entry:
 ; CHECK:   store i32 %x, i32* [[Slot:%[^ ]+]]
 ; CHECK:   invoke void @f()
 ; CHECK:     to label %[[EntryCopy:[^ ]+]] unwind label %catch
 ; CHECK: catch:
 ; CHECK:   catchpad [] to label %[[CatchCopy:[^ ]+]] unwind
 ; CHECK: [[CatchCopy]]:
 ; CHECK:   [[LoadX2:%[^ ]+]] = load i32, i32* [[Slot]]
cleanup:
   br label %exit
 exit:
   call void @f()
   ret void
 }
 ; Need two copies of %exit's call to @f -- the subsequent ret is only
 ; valid when coming from %entry, but on the path from %cleanup, this
 ; might be a valid call to @f which might dynamically not return.
-; CHECK-LABEL: @test2(
+; CHECK-LABEL: define void @test2(
 ; CHECK: entry:
 ; CHECK:   invoke void @f()
 ; CHECK:     to label %[[exit:[^ ]+]] unwind label %cleanup
 ; CHECK: cleanup:
 ; CHECK:   cleanuppad []
 ; CHECK:   call void @f()
 ; CHECK-NEXT: unreachable
 ; CHECK: [[exit]]:

 shared:
   call void @f()
   br label %exit
 exit:
   ret void
 }
 ; Need two copies of %shared's call to @f (similar to @test2 but
 ; the two regions here are siblings, not parent-child).
-; CHECK-LABEL: @test3(
+; CHECK-LABEL: define void @test3(
 ; CHECK:   invoke void @f()
 ; CHECK:   invoke void @f()
 ; CHECK:     to label %[[exit:[^ ]+]] unwind
 ; CHECK: catch:
 ; CHECK:   catchpad [] to label %[[shared:[^ ]+]] unwind
 ; CHECK: cleanup:
 ; CHECK:   cleanuppad []
 ; CHECK:   call void @f()
loop.tail:
   br i1 %done, label %exit, label %loop
 exit:
   call void @h(i32 %x)
   unreachable
 }
 ; Make sure we can clone regions that have internal control
 ; flow and SSA values.  Here we need two copies of everything
 ; from %shared to %exit.
-; CHECK-LABEL: @test4(
+; CHECK-LABEL: define void @test4(
 ; CHECK:  entry:
 ; CHECK:    to label %[[shared_E:[^ ]+]] unwind label %catch
 ; CHECK:  catch:
 ; CHECK:    to label %[[shared_C:[^ ]+]] unwind label %endcatch
 ; CHECK:  [[shared_C]]:
 ; CHECK:    [[x_C:%[^ ]+]] = call i32 @g()
 ; CHECK:    [[i_C:%[^ ]+]] = call i32 @g()
 ; CHECK:    [[zt_C:%[^ ]+]] = icmp eq i32 [[i_C]], 0

 outer.ret:
   cleanupret %o unwind to caller
 exit:
   ret void
 }
 ; Simple nested case (catch-inside-cleanup).  Nothing needs
 ; to be cloned.  The def and use of %x are both in %outer
 ; and so don't need to be spilled.
-; CHECK-LABEL: @test5(
+; CHECK-LABEL: define void @test5(
 ; CHECK:      outer:
 ; CHECK:        %x = call i32 @g()
 ; CHECK-NEXT:   invoke void @f()
 ; CHECK-NEXT:     to label %outer.ret unwind label %inner
 ; CHECK:      inner:
 ; CHECK:          to label %inner.catch unwind label %inner.endcatch
 ; CHECK:      inner.catch:
 ; CHECK-NEXT:   catchret %i to label %outer.post-inner

 ; copy under each parent.  The cleanupret in %inner
 ; unwinds to the catchendpad for %right, so the copy
 ; of %inner under %right should include it; the copy
 ; of %inner under %left should instead have an
 ; `unreachable` inserted there, but the copy under
 ; %left still needs to be created because it's possible
 ; the dynamic path enters %left, then enters %inner,
 ; then calls @h, and that the call to @h doesn't return.
-; CHECK-LABEL: @test6(
+; CHECK-LABEL: define void @test6(
 ; TODO: CHECKs
 
 
 define void @test7() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %invoke.cont unwind label %left
 invoke.cont:
inner:
   call void @h(i32 %x)
   cleanupret %i unwind label %right.end
 unreachable:
   unreachable
 }
 ; Another case of a two-parent child (like @test6), this time
 ; with the join at the entry itself instead of following a
 ; non-pad join.
-; CHECK-LABEL: @test7(
+; CHECK-LABEL: define void @test7(
 ; TODO: CHECKs
 
 
 define void @test8() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %invoke.cont unwind label %left
 invoke.cont:
inner.child:
   %x = call i32 @g()
   call void @h(i32 %x)
   unreachable
 unreachable:
   unreachable
 }
 ; %inner is a two-parent child which itself has a child; need
 ; to make two copies of both the %inner and %inner.child.
-; CHECK-LABEL: @test8(
+; CHECK-LABEL: define void @test8(
 ; TODO: CHECKs
 
 
 define void @test9() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()
     to label %invoke.cont unwind label %left
 invoke.cont:

 ; need to make two copies of each funclet (one a child of root, the
 ; other a child of the opposite funclet), but also make sure not to
 ; clone self-descendants (if we tried to do that we'd need to make an
 ; infinite number of them).  Presumably if optimizations ever generated
 ; such a thing it would mean that one of the two cleanups was originally
 ; the parent of the other, but that we'd somehow lost track in the CFG
 ; of which was which along the way; generating each possibility lets
 ; whichever case was correct execute correctly.
-; CHECK-LABEL: @test9(
+; CHECK-LABEL: define void @test9(
 ; TODO: CHECKs
+
+define void @test10() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  invoke void @f()
+    to label %unreachable unwind label %inner
+inner:
+  %cleanup = cleanuppad []
+  ; make sure we don't overlook this cleanupret and try to process
+  ; successor %outer as a child of inner.
+  cleanupret %cleanup unwind label %outer
+outer:
+  %catch = catchpad [] to label %catch.body unwind label %endpad
+catch.body:
+  catchret %catch to label %exit
+endpad:
+  catchendpad unwind to caller
+exit:
+  ret void
+unreachable:
+  unreachable
+}
+; CHECK-LABEL: define void @test10(
+; CHECK-NEXT: entry:
+; CHECK-NEXT:   invoke
+; CHECK-NEXT:     to label %unreachable unwind label %inner
+; CHECK:      inner:
+; CHECK-NEXT:   %cleanup = cleanuppad
+; CHECK-NEXT:   cleanupret %cleanup unwind label %outer
+; CHECK:      outer:
+; CHECK-NEXT:   %catch = catchpad [] to label %catch.body unwind label %endpad
+; CHECK:      catch.body:
+; CHECK-NEXT:   catchret %catch to label %exit
+; CHECK:      endpad:
+; CHECK-NEXT:   catchendpad unwind to caller
+; CHECK:      exit:
+; CHECK-NEXT:   ret void