[ConstantHoisting] Fix bug where constant materialization could insert into EH pad

[Holman]

Summary

If the incoming block to a phi node is an EH pad, then we will materialize into an EH pad, which is not supposed to happen. To fix this, I added a check to see if incoming block of a phi node is an EH pad before using it as the insertion point.

[Holman]

This bug is causing Edge builds to fail when using new builds of Clang, so I would appreciate if someone could please take a look.

Also, CI failures look unrelated to this change and I was not able to repro them if I rebase change onto latest main.

[nikic]

This needs a test case. See llvm/test/Transforms/ConstantHoisting for existing tests for this pass.

[rnk]

Out of curiosity, have you bisected this to a particular change?

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

190

Are you sure you meant to shadow the outer variable here? InsertionBlock will be null in the case you are trying to handle, a phi, preceded by a catchswitch, and I'm not sure what DT->getNode(nullptr) returns.

[Holman]

In D95019#2513466, @rnk wrote:

Out of curiosity, have you bisected this to a particular change?

I haven't. It was crashing LTO build so repro was time consuming. I was trying to find recent changes that might have allowed more hoisting to happen, but there wasn't anything obvious to me. We took a compiler drop around 11/25 and then the next one we took on 1/3 was failing.

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

190

Good catch, thanks! I originally just copy/pasted the EH loop when I was testing and must have introduced that bug when I refactored.

[Holman]

In D95019#2513251, @nikic wrote:

This needs a test case. See llvm/test/Transforms/ConstantHoisting for existing tests for this pass.

I'm working on a test case, but my repro was massive, and hand writing the IR for this is a bit tricky.

[Holman]

Fix shadowing bug.

[Holman]

Added a unit test

[Holman]

Looks like my new test is passing in CI, but Debian CI is failing for some unrelated reason.

[Holman]

Address clang-tidy comment.

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

Instruction *ConstantHoistingPass::findMatInsertPt(Instruction *Inst,
 
   // The simple and common case. This also includes constant expressions.
   if (!isa<PHINode>(Inst) && !Inst->isEHPad())
     return Inst;
 
   // We can't insert directly before a phi node or an eh pad. Insert before
   // the terminator of the incoming or dominating block.
   assert(Entry != Inst->getParent() && "PHI or landing pad in entry block!");
-  if (Idx != ~0U && isa<PHINode>(Inst))
-    return cast<PHINode>(Inst)->getIncomingBlock(Idx)->getTerminator();
+  BasicBlock *InsertionBlock = nullptr;
+  if (Idx != ~0U && isa<PHINode>(Inst)) {
+    InsertionBlock = cast<PHINode>(Inst)->getIncomingBlock(Idx);
+    if (!InsertionBlock->isEHPad()) {
+      return InsertionBlock->getTerminator();
+    }
+  } else {
+    InsertionBlock = Inst->getParent();
+  }
 
   // This must be an EH pad. Iterate over immediate dominators until we find a
   // non-EH pad. We need to skip over catchswitch blocks, which are both EH pads
   // and terminators.
-  auto IDom = DT->getNode(Inst->getParent())->getIDom();
+  auto IDom = DT->getNode(InsertionBlock)->getIDom();
   while (IDom->getBlock()->isEHPad()) {
     assert(Entry != IDom->getBlock() && "eh pad in entry block");
     IDom = IDom->getIDom();
   }
 
   return IDom->getBlock()->getTerminator();
 }
 

llvm/test/Transforms/ConstantHoisting/X86/ehphi.ll

+; RUN: opt -S -consthoist < %s | FileCheck %s
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define internal fastcc void @baz(i8* %arg) unnamed_addr personality i8* bitcast (i32 (...)* @wobble to i8*) {
+; CHECK-LABEL:  @baz
+bb:
+  %tmp = invoke noalias dereferenceable(40) i8* @wibble.2(i64 40)
+          to label %bb6 unwind label %bb1
+
+bb1:                                              ; preds = %bb
+; CHECK: bb1:
+; CHECK-NEXT:  %tmp2 = catchswitch within none [label %bb3] unwind label %bb16
+  %tmp2 = catchswitch within none [label %bb3] unwind label %bb16
+
+bb3:                                              ; preds = %bb1
+  %tmp4 = catchpad within %tmp2 [i8* null, i32 64, i8* null]
+  invoke void @spam(i8* null) [ "funclet"(token %tmp4) ]
+          to label %bb5 unwind label %bb16
+
+bb5:                                              ; preds = %bb3
+  unreachable
+
+bb6:                                              ; preds = %bb
+  %tmp7 = icmp eq i8* %arg, null
+  br label %bb9
+
+
+bb9:                                              ; preds = %bb8, %bb6
+  %tmp10 = inttoptr i64 -6148914691236517376 to i16*
+  %tmp11 = invoke noalias dereferenceable(40) i8* @wibble.2(i64 40)
+          to label %bb15 unwind label %bb12
+
+bb12:                                             ; preds = %bb9
+  %tmp13 = cleanuppad within none []
+  br label %bb14
+
+bb14:                                             ; preds = %bb12
+  cleanupret from %tmp13 unwind label %bb16
+
+bb15:                                             ; preds = %bb9
+  ret void
+
+bb16:                                             ; preds = %bb14, %bb3, %bb1
+  %tmp17 = phi i16* [ inttoptr (i64 -6148914691236517376 to i16*), %bb1 ], [ inttoptr (i64 -6148914691236517376 to i16*), %bb3 ], [ %tmp10, %bb14 ]
+  %tmp18 = cleanuppad within none []
+  br label %bb19
+
+bb19:                                             ; preds = %bb16
+  cleanupret from %tmp18 unwind to caller
+}
+
+declare i8* @wibble.2(i64)
+
+declare dso_local void @spam(i8*) local_unnamed_addr
+
+declare i32 @wobble(...)