[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)) {
+    auto 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();
 }