[WinEH] Fix two cases where instructions can incorrectly be inserted into EH pads

[andrew.w.kaylor]

Summary

I came across two cases where instructions were being inserted above EH pad instructions that don't allow non-PHI instructions before the terminator. In one case, CodeGenPrepare was trying to sink a bitcast into a catchpad. In another case, GVN was trying to PRE a load in front of a catchpadend.

The precise conditions under which these circumstances arise are a bit involved, I think. I saw it on a branched project while compiling template-based code and couldn't come up with reduced C++ code that reproduces the problem on trunk, but both of the new IR test cases I'm adding failed for me before I applied my code changes.

[majnemer]

I'd commit these as two different commits seeing as how they are independent of each other.

lib/CodeGen/CodeGenPrepare.cpp

732–735 ↗

(On Diff #40698)

Likewise, I'd change this to simply be UserBB->getTerminator()->isEHPad()

lib/Transforms/Scalar/GVN.cpp

1557–1560 ↗

(On Diff #40698)

I believe this can be simplified to if (Pred->getTerminator()->isEHPad()) because the only case where this comes up is when the terminator is also an EH pad.

[andrew.w.kaylor]

Simplified isEHPad condition as suggested.

I'm happy to commit these changes separately. You are correct that they are functionally independent. It seemed like it would be easier to review them together given how similar the cases are. Will Phabricator me unhappy with me if I try to tag two different commits with the same differential revision?

[majnemer]

I think phab will tolerate two reference to the same differential just fine.

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

for (Value::user_iterator UI = CI->user_begin(), E = CI->user_end();
     BasicBlock *UserBB = User->getParent();
     if (PHINode *PN = dyn_cast<PHINode>(User)) {
       UserBB = PN->getIncomingBlock(TheUse);
     }
 
     // Preincrement use iterator so we don't invalidate it.
     ++UI;
 
+    // If the block selected to receive the cast is an EH pad that does not
+    // allow non-PHI instructions before the terminator, we can't sink the
+    // cast.
+    if (UserBB->getTerminator()->isEHPad())
+      continue;
+
     // If this user is in the same block as the cast, don't change the cast.
     if (UserBB == DefBB) continue;
 
     // If we have already inserted a cast into this block, use it.
     CastInst *&InsertedCast = InsertedCasts[UserBB];
 
     if (!InsertedCast) {
       BasicBlock::iterator InsertPt = UserBB->getFirstInsertionPt();

llvm/trunk/test/Transforms/CodeGenPrepare/catchpad-phi-cast.ll

+; RUN: opt -codegenprepare -S < %s | FileCheck %s
+
+; The following target lines are needed for the test to exercise what it should.
+; Without these lines, CodeGenPrepare does not try to sink the bitcasts.
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+declare i32 @__CxxFrameHandler3(...)
+
+declare void @f()
+
+declare void @g(i8*)
+
+; CodeGenPrepare will want to sink these bitcasts, but it selects the catchpad
+; blocks as the place to which the bitcast should be sunk.  Since catchpads
+; do not allow non-phi instructions before the terminator, this isn't possible. 
+
+; CHECK-LABEL: @test(
+define void @test(i32* %addr) personality i32 (...)* @__CxxFrameHandler3 {
+; CHECK: entry:
+; CHECK-NEXT: %x = getelementptr i32, i32* %addr, i32 1
+; CHECK-NEXT: %p1 = bitcast i32* %x to i8*
+entry:
+  %x = getelementptr i32, i32* %addr, i32 1
+  %p1 = bitcast i32* %x to i8*
+  invoke void @f()
+          to label %invoke.cont unwind label %catch1
+
+; CHECK: invoke.cont:
+; CHECK-NEXT: %y = getelementptr i32, i32* %addr, i32 2
+; CHECK-NEXT: %p2 = bitcast i32* %y to i8*
+invoke.cont:
+  %y = getelementptr i32, i32* %addr, i32 2
+  %p2 = bitcast i32* %y to i8*
+  invoke void @f()
+          to label %done unwind label %catch2
+
+done:
+  ret void
+
+catch1:
+  %cp1 = catchpad [] to label %catch.dispatch unwind label %catchend1
+
+catch2:
+  %cp2 = catchpad [] to label %catch.dispatch unwind label %catchend2
+
+; CHECK: catch.dispatch:
+; CHECK-NEXT: %p = phi i8* [ %p1, %catch1 ], [ %p2, %catch2 ]
+catch.dispatch:
+  %p = phi i8* [ %p1, %catch1 ], [ %p2, %catch2 ]
+  call void @g(i8* %p)
+  unreachable
+
+catchend1:
+  catchendpad unwind to caller
+
+catchend2:
+  catchendpad unwind to caller
+}