[WinEHPrepare] Fix catchret successor phi demotion

[JosephTremoulet]

Summary

When demoting an SSA value that has a use on a phi and one of the phi's
predecessors terminates with catchret, the edge needs to be split and the
load inserted in the new block, else we'll still have a cross-funclet SSA
value.

Add a test for this, and for the similar case where a def to be spilled is
on and invoke and a critical edge, which was already implemented but
missing a test.

[majnemer]

Nice catch, LGTM.

As an aside, would it be more optimal (and legal) to have a single slot for the value contained within %phi? We'd still need two stores but I think we could save a load. If so, we might want a TODO for it if we don't already have an appropriate one elsewhere.

[JosephTremoulet]

If I'm understanding correctly, you're suggesting that we could use the same slot to demote %x that we use to demote %y, and then instead of inserting loads in each predecessor of %join (which requires splitting edges) we could just insert a load at the top of %join? I do think that's legal, and by static count it eliminates a load but by dynamic count it doesn't. I think what we could do to get cases like this is check to see if any to values we're demoting appear as arguments to the same phi and have non-interfering liveranges, and if so coalesce their spill slots and (split predecessors of the phi if it has incoming values we're not demoting and) insert a single load after the join rather than one load per pred. No, the TODOs already in the code don't cover that, LMK if you think it's worth adding one (or if I've misunderstood what you're suggesting).

[majnemer]

In D12065#225408, @JosephTremoulet wrote:

No, the TODOs already in the code don't cover that, LMK if you think it's worth adding one (or if I've misunderstood what you're suggesting).

Nah, I think it's fine to proceed.

lib/CodeGen/WinEHPrepare.cpp

if (auto *UsingPHI = dyn_cast<PHINode>(UsingInst)) {
     // corresponding to the incoming value.
     //
     // Note that if there are multiple edges from a basic block to this
     // PHI node that we cannot have multiple loads.  The problem is that
     // the resulting PHI node will have multiple values (from each load)
     // coming in from the same block, which is illegal SSA form.
     // For this reason, we keep track of and reuse loads we insert.
     BasicBlock *IncomingBlock = UsingPHI->getIncomingBlock(U);
+    if (auto *CatchRet =
+            dyn_cast<CatchReturnInst>(IncomingBlock->getTerminator())) {
+      // Putting a load above a catchret and use on the phi would still leave
+      // a cross-funclet def/use.  We need to split the edge, change the
+      // catchret to target the new block, and put the load there.
+      BasicBlock *PHIBlock = UsingInst->getParent();
+      BasicBlock *NewBlock = SplitEdge(IncomingBlock, PHIBlock);
+      // SplitEdge gives us:
+      //   IncomingBlock:
+      //     ...
+      //     br label %NewBlock
+      //   NewBlock:
+      //     catchret label %PHIBlock
+      // But we need:
+      //   IncomingBlock:
+      //     ...
+      //     catchret label %NewBlock
+      //   NewBlock:
+      //     br label %PHIBlock
+      // So move the terminators to each others' blocks and swap their
+      // successors.
+      BranchInst *Goto = cast<BranchInst>(IncomingBlock->getTerminator());
+      Goto->removeFromParent();
+      CatchRet->removeFromParent();
+      IncomingBlock->getInstList().push_back(CatchRet);
+      NewBlock->getInstList().push_back(Goto);
+      Goto->setSuccessor(0, PHIBlock);
+      CatchRet->setSuccessor(NewBlock);
+      // Update the color mapping for the newly split edge.
+      std::set<BasicBlock *> &ColorsForPHIBlock = BlockColors[PHIBlock];
+      BlockColors[NewBlock] = ColorsForPHIBlock;
+      for (BasicBlock *FuncletPad : ColorsForPHIBlock)
+        FuncletBlocks[FuncletPad].insert(NewBlock);
+      // Treat the new block as incoming for load insertion.
+      IncomingBlock = NewBlock;
+    }
     Value *&Load = Loads[IncomingBlock];
     // Insert the load into the predecessor block
     if (!Load)
       Load = new LoadInst(SpillSlot, Twine(V->getName(), ".wineh.reload"),
                           /*Volatile=*/false, IncomingBlock->getTerminator());
 
     U.set(Load);
   } else {
     // Reload right before the old use.
     auto *Load = new LoadInst(SpillSlot, Twine(V->getName(), ".wineh.reload"),
                               /*Volatile=*/false, UsingInst);
     U.set(Load);
   }
 }

test/CodeGen/WinEH/wineh-demotion.ll

catch:
   catchret void to label %exit
 
 catchend:
   catchendpad unwind to caller
 
 exit:
   ret void
 }
+
+; CHECK-LABEL: @test6(
+define void @test6() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  ; Since %x needs to be stored but the edge to loop is critical,
+  ; it needs to be split
+  ; CHECK: entry:
+  ; CHECK: invoke i32 @g
+  ; CHECK-NEXT: to label %[[SplitBlock:[^ ]+]] unwind label %to_caller
+  %x = invoke i32 @g()
+          to label %loop unwind label %to_caller
+  ; The store should be in the split block
+  ; CHECK: [[SplitBlock]]:
+  ; CHECK: store i32 %x, i32* [[SpillSlot:%[^ ]+]]
+  ; CHECK: br label %loop
+to_caller:
+  cleanuppad void []
+  cleanupret void unwind to caller
+loop:
+  invoke void @f()
+          to label %loop unwind label %cleanup
+cleanup:
+  ; CHECK: cleanup:
+  ; CHECK:   [[Load:%[^ ]+]] = load i32, i32* [[SpillSlot]]
+  ; CHECK:   call void @h(i32 [[Load]])
+  cleanuppad void []
+  call void @h(i32 %x)
+  cleanupret void unwind to caller
+}
+
+; CHECK-LABEL: @test7(
+define void @test7() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  ; %x is an EH pad phi, so gets stored in pred here
+  ; CHECK: entry:
+  ; CHECK:   store i32 1, i32* [[SlotX:%[^ ]+]]
+  ; CHECK:   invoke void @f()
+  invoke void @f()
+     to label %invoke.cont unwind label %catchpad
+invoke.cont:
+  ; %x is an EH pad phi, so gets stored in pred here
+  ; CHECK: invoke.cont:
+  ; CHECK:   store i32 2, i32* [[SlotX]]
+  ; CHECK:   invoke void @f()
+  invoke void @f()
+    to label %exit unwind label %catchpad
+catchpad:
+  ; %x phi should be eliminated
+  ; CHECK: catchpad:
+  ; CHECK-NEXT: catchpad void
+  %x = phi i32 [ 1, %entry ], [ 2, %invoke.cont ]
+  catchpad void [] to label %catch unwind label %catchend
+catch:
+  %b = call i1 @i()
+  br i1 %b, label %left, label %right
+left:
+  ; Edge from %left to %join needs to be split so that
+  ; the load of %x can be inserted *after* the catchret
+  ; CHECK: left:
+  ; CHECK-NEXT: catchret void to label %[[SplitLeft:[^ ]+]]
+  catchret void to label %join
+  ; CHECK: [[SplitLeft]]:
+  ; CHECK:   [[LoadX:%[^ ]+]] = load i32, i32* [[SlotX]]
+  ; CHECK:   br label %join
+right:
+  ; Edge from %right to %join needs to be split so that
+  ; the load of %y can be inserted *after* the catchret
+  ; CHECK: right:
+  ; CHECK:   store i32 %y, i32* [[SlotY:%[^ ]+]]
+  ; CHECK:   catchret void to label %[[SplitRight:[^ ]+]]
+  %y = call i32 @g()
+  catchret void to label %join
+  ; CHECK: [[SplitRight]]:
+  ; CHECK:   [[LoadY:%[^ ]+]] = load i32, i32* [[SlotY]]
+  ; CHECK:   br label %join
+catchend:
+  catchendpad unwind to caller
+join:
+  ; CHECK: join:
+  ; CHECK:   %phi = phi i32 [ [[LoadX]], %[[SplitLeft]] ], [ [[LoadY]], %[[SplitRight]] ]
+  %phi = phi i32 [ %x, %left ], [ %y, %right ]
+  call void @h(i32 %phi)
+  br label %exit
+exit:
+  ret void
+}