[WinEH] Fix inttoptr+phi optimization in presence of catchswitch

[rnk]

Summary

getFirstInsertionPt's return value must be checked for validity before
casting it to Instruction*. Don't attempt to insert casts after a phi in
a catchswitch block.

Fixes PR45033, introduced in D37832.

[hfinkel]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

222

When you say, "a block with no insertion point", I'd add, "for example, a catchswitch block." I don't think that it's otherwise obvious how one has a block with no insertion point.

[rnk]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

222

Sure, will do.

---

I think when we added catchswitch, part of what we were thinking was that getFirstInsertionPt returns an iterator, and an end iterator is a valid iterator, so it wouldn't be unreasonable to expect callers to check them for validity. In practice, people expect the function to do what it says and return a suitable insertion point, and not a failure value.

At the time we considered adding a multi-unwind-destination version of invoke, but I considered it to be too radical. Instead we settled for this unwind-edge-multiplex instruction, catchswitch. Now we have callbr though, so we should rethink this sharp edge of our WinEH (wasm uses it too, I guess) representation.

[rnk]

• elaborate

[davidxl]

looks ok to me, but wait for Hal to have a final say.

[hfinkel]

In D75381#1899202, @davidxl wrote:

looks ok to me, but wait for Hal to have a final say.

LGTM

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

Instruction *InstCombiner::FoldIntegerTypedPHI(PHINode &PN) {
 
   // If it requires a conversion for every PHI operand, do not do it.
   if (all_of(AvailablePtrVals, [&](Value *V) {
         return (V->getType() != IntToPtr->getType()) || isa<IntToPtrInst>(V);
       }))
     return nullptr;
 
   // If any of the operand that requires casting is a terminator
-  // instruction, do not do it.
+  // instruction, do not do it. Similarly, do not do the transform if the value
+  // is PHI in a block with no insertion point, for example, a catchswitch
+  // block, since we will not be able to insert a cast after the PHI.
   if (any_of(AvailablePtrVals, [&](Value *V) {
         if (V->getType() == IntToPtr->getType())
           return false;
-
         auto *Inst = dyn_cast<Instruction>(V);
-        return Inst && Inst->isTerminator();
+        if (!Inst)
+          return false;
+        if (Inst->isTerminator())
+          return true;
+        auto *BB = Inst->getParent();
+        if (isa<PHINode>(Inst) && BB->getFirstInsertionPt() == BB->end())
+          return true;
+        return false;
       }))
     return nullptr;
 
   PHINode *NewPtrPHI = PHINode::Create(
       IntToPtr->getType(), PN.getNumIncomingValues(), PN.getName() + ".ptr");
 
   InsertNewInstBefore(NewPtrPHI, PN);
   SmallDenseMap<Value *, Instruction *> Casts;
#endif
     // %v.cast = load float *, float ** %v.ptrp, align 8
     Instruction *&CI = Casts[IncomingVal];
     if (!CI) {
       CI = CastInst::CreateBitOrPointerCast(IncomingVal, IntToPtr->getType(),
                                             IncomingVal->getName() + ".ptr");
       if (auto *IncomingI = dyn_cast<Instruction>(IncomingVal)) {
         BasicBlock::iterator InsertPos(IncomingI);
         InsertPos++;
+        BasicBlock *BB = IncomingI->getParent();
         if (isa<PHINode>(IncomingI))
-          InsertPos = IncomingI->getParent()->getFirstInsertionPt();
+          InsertPos = BB->getFirstInsertionPt();
+        assert(InsertPos != BB->end() && "should have checked above");
         InsertNewInstBefore(CI, *InsertPos);
       } else {
         auto *InsertBB = &IncomingBB->getParent()->getEntryBlock();
         InsertNewInstBefore(CI, *InsertBB->getFirstInsertionPt());
       }
     }
     NewPtrPHI->addIncoming(CI, IncomingBB);
   }

llvm/test/Transforms/InstCombine/intptr8.ll

+; RUN: opt < %s  -instcombine -S | FileCheck %s
+
+; PR45033: Don't try to insert a cast into a catchswich block.
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+%struct.intrusive_ptr = type { %struct.C* }
+%struct.C = type { %struct.intrusive_ref_counter }
+%struct.intrusive_ref_counter = type { i32 }
+
+declare dso_local %struct.C* @"?mk@@YAPEAUC@@XZ"() #3
+declare dso_local void @"?intrusive_ptr_release@@YAXPEBUintrusive_ref_counter@@@Z"(%struct.intrusive_ref_counter*) #3
+declare dso_local void @"?terminate@@YAXXZ"()
+declare dso_local i32 @__CxxFrameHandler3(...)
+
+define dso_local void @"?crash@@YAXXZ"() local_unnamed_addr #0 personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  %call1 = invoke %struct.C* @"?mk@@YAPEAUC@@XZ"()
+          to label %invoke.cont2 unwind label %catch.dispatch
+
+invoke.cont2:                                     ; preds = %entry
+  %0 = ptrtoint %struct.C* %call1 to i64
+  %call5 = invoke %struct.C* @"?mk@@YAPEAUC@@XZ"()
+          to label %try.cont unwind label %catch.dispatch
+
+catch.dispatch:                                   ; preds = %invoke.cont2, %entry
+  %a.sroa.0.0 = phi i64 [ %0, %invoke.cont2 ], [ 0, %entry ]
+  %1 = catchswitch within none [label %catch] unwind label %ehcleanup
+
+catch:                                            ; preds = %catch.dispatch
+  %2 = catchpad within %1 [i8* null, i32 64, i8* null]
+  catchret from %2 to label %try.cont
+
+try.cont:                                         ; preds = %invoke.cont2, %catch
+  %a.sroa.0.1 = phi i64 [ %0, %invoke.cont2 ], [ %a.sroa.0.0, %catch ]
+  %3 = inttoptr i64 %a.sroa.0.1 to %struct.C*
+  %tobool.i3 = icmp eq %struct.C* %3, null
+  br i1 %tobool.i3, label %"??1?$intrusive_ptr@UC@@@@[e-mail]6", label %if.then.i4
+
+if.then.i4:                                       ; preds = %try.cont
+  %4 = getelementptr %struct.C, %struct.C* %3, i64 0, i32 0
+  invoke void @"?intrusive_ptr_release@@YAXPEBUintrusive_ref_counter@@@Z"(%struct.intrusive_ref_counter* %4)
+          to label %"??1?$intrusive_ptr@UC@@@@[e-mail]6" unwind label %terminate.i5
+
+terminate.i5:                                     ; preds = %if.then.i4
+  %5 = cleanuppad within none []
+  call void @"?terminate@@YAXXZ"() #4 [ "funclet"(token %5) ]
+  unreachable
+
+"??1?$intrusive_ptr@UC@@@@[e-mail]6":         ; preds = %try.cont, %if.then.i4
+  ret void
+
+ehcleanup:                                        ; preds = %catch.dispatch
+  %6 = cleanuppad within none []
+  %7 = inttoptr i64 %a.sroa.0.0 to %struct.C*
+  %tobool.i = icmp eq %struct.C* %7, null
+  br i1 %tobool.i, label %"??1?$intrusive_ptr@UC@@@@[e-mail]", label %if.then.i
+
+if.then.i:                                        ; preds = %ehcleanup
+  %8 = getelementptr %struct.C, %struct.C* %7, i64 0, i32 0
+  invoke void @"?intrusive_ptr_release@@YAXPEBUintrusive_ref_counter@@@Z"(%struct.intrusive_ref_counter* %8) [ "funclet"(token %6) ]
+          to label %"??1?$intrusive_ptr@UC@@@@[e-mail]" unwind label %terminate.i
+
+terminate.i:                                      ; preds = %if.then.i
+  %9 = cleanuppad within %6 []
+  call void @"?terminate@@YAXXZ"() #4 [ "funclet"(token %9) ]
+  unreachable
+
+"??1?$intrusive_ptr@UC@@@@[e-mail]":          ; preds = %ehcleanup, %if.then.i
+  cleanupret from %6 unwind to caller
+}
+
+; CHECK-LABEL: define dso_local void @"?crash@@YAXXZ"
+; CHECK: catch.dispatch:
+; CHECK-NEXT: %a.sroa.0.0 = phi i64
+; CHECK-NEXT: catchswitch within none [label %catch] unwind label %ehcleanup