[InstCombine] Don't slice up PHIs when pred BB has catchswitch

[aheejin]

Summary

If an integer PHI has an illegal type (according to the data layout) and
it is only used by trunc or trunc(lshr) operations, we split the PHI
into various instructions in its predecessors:
https://github.com/llvm/llvm-project/blob/6d1543a16797fa07eecea7e542df5b42422fc721/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp#L1536-L1543

So this can produce code like the following:
Before:

pred:
  ...

bb:
  %p = phi i8 [ %somevalue, %pred ], ...
  ...
  %tobool = trunc i8 %p to i1
  use %tobool
  ...

In this code, %p has an illegal integer type, i8, and its only used
in a trunc instruction later. In this case this pass puts extraction
code in its predecessors:

After:

pred:
  ...
  %t = and i8 %somevalue, 1
  %extract = icmp ne i8 %t, 0

bb:
  %p.new = phi i1 [ %extract, %pred ], ...
  use %p.new instead of %tobool

But this doesn't work if pred is a catchswitch BB because it cannot
have any non-PHI instructions. This CL ensures we bail out in that case.

Fixes https://github.com/llvm/llvm-project/issues/55803.

[aheejin]

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

62

This test is a little more complicated than I hoped, but I couldn't reduce it further. Removing any BB from here breaks the complex condition necessary to exactly trigger this bug.

[dschuff]

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

60

95

So, does the IR verifier check the property that catchswitches can't have any non-PHIs? would that be sufficient to check this?
Not that I'm suggesting you change this, it's a simple condition and I don't really expect it to be brittle. Mostly I'm just wondering how this escaped and what made the resulting error not obvious.

[aheejin]

Address comments

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

95

I don't think the IR verifier runs after every pass unless you pass some option, so this pass produced an invalid IR, and that caused the crash in the inliner. But it seems opt at least checks the validity at the end, so if you run only -instcombine and if it produces an invalid IR, it will crash.

[nikic]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

1128

The correct way to check this is whether getFirstInsertionPt() is end().

[aheejin]

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

1128

Done in D127810.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

for (auto Incoming : zip(PN->blocks(), PN->incoming_values())) {
         continue;
 
       // If we have a phi, and if it's directly in the predecessor, then we have
       // a critical edge where we need to put the truncate.  Since we can't
       // split the edge in instcombine, we have to bail out.
       return nullptr;
     }
 
+    // If the incoming value is a PHI node before a catchswitch, we cannot
+    // extract the value within that BB because we cannot insert any non-PHI
+    // instructions in the BB.
+    for (auto *Pred : PN->blocks())
+      if (isa<CatchSwitchInst>(Pred->getFirstNonPHI()))
+        return nullptr;
+
     for (User *U : PN->users()) {
       Instruction *UserI = cast<Instruction>(U);
 
       // If the user is a PHI, inspect its uses recursively.
       if (PHINode *UserPN = dyn_cast<PHINode>(UserI)) {
         if (PHIsInspected.insert(UserPN).second)
           PHIsToSlice.push_back(UserPN);
         continue;

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

 ; RUN: opt < %s -passes=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 @baz()
 declare i32 @__gxx_wasm_personality_v0(...)
+; Function Attrs: noreturn
+declare void @llvm.wasm.rethrow() #0
 
-define void @test() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+; Test that a PHI in catchswitch BB are excluded from combining into a non-PHI
+; instruction.
+define void @test0() 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

 bb6:                                              ; preds = %bb5
   unreachable
 
 bb7:                                              ; preds = %bb5, %bb4
   %tmp6 = cleanuppad within none []
   call void @bar(%struct.quux* %tmp3) [ "funclet"(token %tmp6) ]
   unreachable
 }
+
+; Test that slicing-up of illegal integer type PHI does not happen in catchswitch
+; BBs, which can't have any non-PHI instruction before the catchswitch.
+define void @test1() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+entry:
+  invoke void @foo()
+          to label %invoke.cont unwind label %catch.dispatch1
+
+invoke.cont:                                      ; preds = %entry
+  %call = invoke i32 @baz()
+          to label %invoke.cont1 unwind label %catch.dispatch
+
+invoke.cont1:                                     ; preds = %invoke.cont
+  %tobool = icmp ne i32 %call, 0
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:                                          ; preds = %invoke.cont1
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %invoke.cont1
+  %ap.0 = phi i8 [ 1, %if.then ], [ 0, %invoke.cont1 ]
+  invoke void @foo()
+          to label %invoke.cont2 unwind label %catch.dispatch
+
+invoke.cont2:                                     ; preds = %if.end
+  br label %try.cont
+
+catch.dispatch:                                   ; preds = %if.end, %invoke.cont
+  ; %ap.2 in catch.dispatch1 BB has an illegal integer type (i8) in the data
+  ; layout, and it is only used by trunc or trunc(lshr) operations. In this case
+  ; InstCombine will split this PHI in its predecessors, which include this
+  ; catch.dispatch BB. This splitting involves creating non-PHI instructions,
+  ; such as 'and' or 'icmp' in this BB, which is not valid for a catchswitch BB.
+  ; So if one of sliced-up PHI's predecessor is a catchswitch block, we don't
+  ; optimize that case and bail out. This BB should be preserved intact after
+  ; InstCombine and the pass shouldn't produce invalid code.
+  ; CHECK: catch.dispatch:
+  ; CHECK-NEXT: phi
+  ; CHECK-NEXT: catchswitch
+  %ap.1 = phi i8 [ %ap.0, %if.end ], [ 0, %invoke.cont ]
+  %tmp0 = catchswitch within none [label %catch.start] unwind label %catch.dispatch1
+
+catch.start:                                      ; preds = %catch.dispatch
+  %tmp1 = catchpad within %tmp0 [i8* null]
+  br i1 0, label %catch, label %rethrow
+
+catch:                                            ; preds = %catch.start
+  catchret from %tmp1 to label %try.cont
+
+rethrow:                                          ; preds = %catch.start
+  invoke void @llvm.wasm.rethrow() #0 [ "funclet"(token %tmp1) ]
+          to label %unreachable unwind label %catch.dispatch1
+
+catch.dispatch1:                                  ; preds = %rethrow, %catch.dispatch, %entry
+  %ap.2 = phi i8 [ %ap.1, %catch.dispatch ], [ %ap.1, %rethrow ], [ 0, %entry ]
+  %tmp2 = catchswitch within none [label %catch.start1] unwind to caller
+
+catch.start1:                                     ; preds = %catch.dispatch1
+  %tmp3 = catchpad within %tmp2 [i8* null]
+  %tobool1 = trunc i8 %ap.2 to i1
+  br i1 %tobool1, label %if.then1, label %if.end1
+
+if.then1:                                         ; preds = %catch.start1
+  br label %if.end1
+
+if.end1:                                          ; preds = %if.then1, %catch.start1
+  catchret from %tmp3 to label %try.cont
+
+try.cont:                                         ; preds = %if.end1, %catch, %invoke.cont2
+  ret void
+
+unreachable:                                      ; preds = %rethrow
+  unreachable
+}
+
+attributes #0 = { noreturn }