[InstCombine] Don't combine PHI before catchswitch

[aheejin]

Summary

This tries to bail out if the PHI is in a catchswitch BB in
InstCombine. A PHI cannot be combined into a non-PHI instruction if it
is in a catchswitch BB, because catchswitch BB cannot have any
non-PHI instruction other than catchswitch itself.

The given test case started crashing after D98058.

[lebedev.ri]

llvm/test/Transforms/InstCombine/catchswitch-phi.ll

2

This isn't testing anything

[lebedev.ri]

This is seems like a too heavy hammer.
foldPHIArgZextsIntoPHI() has:

// We cannot create a new instruction after the PHI if the terminator is an
// EHPad because there is no valid insertion point.
if (Instruction *TI = Phi.getParent()->getTerminator())
  if (TI->isEHPad())
    return nullptr;

Is that the same check as here? (which means that the check here is incomplete?)
If yes, then can you add a similar check to whatever fold is missing it, specifically?

[rnk]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

1329

Every other return path of InstCombinerImpl::visitPHINode appears to create and return an existing instruction of a new phi node. This one is special in that it creates a new non-phi instruction.

The insertion location is not determined here, but it is predictable, it's the first available insertion point after the phi, but the phi's block has no insertion point. The generic way to test for this is to compare getInsertionPoint against the end iterator.

So, all the other transforms seem fine. It's reasonable to put a more targeted fix right here:

&& PN.getParent()->getFirstNonPHI() != PN.getParent()->end()

llvm/test/Transforms/InstCombine/catchswitch-phi.ll

2

That's true, and I don't see any cast instructions to trigger the codepath we are looking at.

[aheejin]

Address comments

[aheejin]

In D105309#2853898, @lebedev.ri wrote:

This is seems like a too heavy hammer.
foldPHIArgZextsIntoPHI() has:

// We cannot create a new instruction after the PHI if the terminator is an
// EHPad because there is no valid insertion point.
if (Instruction *TI = Phi.getParent()->getTerminator())
  if (TI->isEHPad())
    return nullptr;

Is that the same check as here? (which means that the check here is incomplete?)

This check is fine, but as we can see here, if the return value of foldPHIArgZextsIntoPHI is nullptr, it doesn't exit visitPHINode but goes on to the next optimization within the function, so it couldn't have prevented the crash.

If yes, then can you add a similar check to whatever fold is missing it, specifically?

I'll move the check to that specific part added in D98058.

llvm/test/Transforms/InstCombine/catchswitch-phi.ll

2

Oh sorry, I forgot to add the RUN line. Added now. Basically this shouldn't crash.
@rnk phi in bb4 is the one that triggers the crash (without this patch).

34

This is the instruction that triggers the crash without this patch

[lebedev.ri]

LG with nits

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

1319–1320

1320–1322

The fix should go here - what's the point of doing all that computation if we know by now that we are going to throw it away?

[aheejin]

Address comments

[aheejin]

Thank you for the feedback!

[rnk]

lgtm

llvm/test/Transforms/InstCombine/catchswitch-phi.ll

2

I think I was looking for bitcast instructions, but the zero-index GEPs are essentially cast instructions here. Looks good.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
   if (isa<Instruction>(PN.getIncomingValue(0)) &&
       isa<Instruction>(PN.getIncomingValue(1)) &&
       cast<Instruction>(PN.getIncomingValue(0))->getOpcode() ==
           cast<Instruction>(PN.getIncomingValue(1))->getOpcode() &&
       PN.getIncomingValue(0)->hasOneUser())
     if (Instruction *Result = foldPHIArgOpIntoPHI(PN))
       return Result;
 
   // If the incoming values are pointer casts of the same original value,
-  // replace the phi with a single cast.
-  if (PN.getType()->isPointerTy()) {
+  // replace the phi with a single cast iff we can insert a non-PHI instruction.
+  if (PN.getType()->isPointerTy() &&
+      PN.getParent()->getFirstInsertionPt() != PN.getParent()->end()) {
     Value *IV0 = PN.getIncomingValue(0);
     Value *IV0Stripped = IV0->stripPointerCasts();
     // Set to keep track of values known to be equal to IV0Stripped after
     // stripping pointer casts.
     SmallPtrSet<Value *, 4> CheckedIVs;
     CheckedIVs.insert(IV0);
     if (IV0 != IV0Stripped &&
         all_of(PN.incoming_values(), [&CheckedIVs, IV0Stripped](Value *IV) {
           return !CheckedIVs.insert(IV).second ||
                  IV0Stripped == IV->stripPointerCasts();
         })) {
       return CastInst::CreatePointerCast(IV0Stripped, PN.getType());
     }
   }
 

llvm/test/Transforms/InstCombine/catchswitch-phi.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128-ni:1"
+target triple = "wasm32-unknown-unknown"
+
+%struct.quux = type { i32 }
+%struct.blam = type <{ %struct.quux }>
+
+declare void @foo()
+declare void @bar(%struct.quux*)
+declare i32 @__gxx_wasm_personality_v0(...)
+
+define void @test() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+bb:
+  %tmp0 = alloca %struct.blam, align 4
+  br i1 undef, label %bb1, label %bb2
+
+bb1:                                              ; preds = %bb
+  %tmp1 = getelementptr inbounds %struct.blam, %struct.blam* %tmp0, i32 0, i32 0
+  invoke void @foo()
+          to label %bb3 unwind label %bb4
+
+bb2:                                              ; preds = %bb
+  %tmp2 = getelementptr inbounds %struct.blam, %struct.blam* %tmp0, i32 0, i32 0
+  invoke void @foo()
+          to label %bb3 unwind label %bb4
+
+bb3:                                              ; preds = %bb2, %bb1
+  unreachable
+
+bb4:                                              ; preds = %bb2, %bb1
+  ; This PHI should not be combined into a non-PHI instruction, because
+  ; catchswitch BB cannot have any non-PHI instruction other than catchswitch
+  ; itself.
+  ; CHECK: bb4:
+  ; CHECK-NEXT: phi
+  ; CHECK-NEXT: catchswitch
+  %tmp3 = phi %struct.quux* [ %tmp1, %bb1 ], [ %tmp2, %bb2 ]
+  %tmp4 = catchswitch within none [label %bb5] unwind label %bb7
+
+bb5:                                              ; preds = %bb4
+  %tmp5 = catchpad within %tmp4 [i8* null]
+  invoke void @foo() [ "funclet"(token %tmp5) ]
+          to label %bb6 unwind label %bb7
+
+bb6:                                              ; preds = %bb5
+  unreachable
+
+bb7:                                              ; preds = %bb5, %bb4
+  %tmp6 = cleanuppad within none []
+  call void @bar(%struct.quux* %tmp3) [ "funclet"(token %tmp6) ]
+  unreachable
+}