[WebAssembly] Fix a bug in removing unnecessary branches

[aheejin]

Summary

One of the things removeUnnecessaryInstrs() in CFGStackify does is to
remove an unnecessary unconditinal branch before an EH pad. When there
is an unconditional branch right before a catch instruction and it
branches to the end of end_try marker, we don't need the branch,
because it there is no exception, the control flow transfers to
that point anyway.

bb0:
  try
    ...
    br bb2      <- Not necessary
bb1:
  catch
    ...
bb2:
  end

This applies when we have a conditional branch followed by an
unconditional one, in which case we should only remove the unconditional
branch. For example:

bb0:
  try
    ...
    br_if someplace_else
    br bb2                 <- Not necessary
bb1:
  catch
    ...
bb2:
  end

But TargetInstrInfo::removeBranch we used removed all existing
branches when there are multiple ones. This patch fixes it by only
deleting the last (= unconditional) branch manually.

Also fixes some preds comments in the test file.

[dschuff]

oh also "WebbAssembly" in the CL title should be "WebAssembly"

llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp

for (auto &MBB : MF) {
     if (!MBB.isEHPad())
       continue;
 
     MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
     SmallVector<MachineOperand, 4> Cond;
     MachineBasicBlock *EHPadLayoutPred = MBB.getPrevNode();
     MachineBasicBlock *Cont = BeginToEnd[EHPadToTry[&MBB]]->getParent();
     bool Analyzable = !TII.analyzeBranch(*EHPadLayoutPred, TBB, FBB, Cond);
+    // This condition means either
+    // 1. This BB ends with a single unconditional branch whose destinaion is
+    //    Cont.
+    // 2. This BB ends with a conditional branch followed by an unconditional
+    //    branch, and the unconditional branch's destination is Cont.
+    // In both cases, we want to remove the last (= unconditional) branch.
     if (Analyzable && ((Cond.empty() && TBB && TBB == Cont) ||
-                       (!Cond.empty() && FBB && FBB == Cont)))
-      TII.removeBranch(*EHPadLayoutPred);
+                       (!Cond.empty() && FBB && FBB == Cont))) {
+      bool ErasedUncondBr = false;
+      for (auto I = EHPadLayoutPred->end(), E = EHPadLayoutPred->begin();
+           I != E; --I) {
+        auto PrevI = std::prev(I);
+        if (PrevI->isTerminator()) {
+          assert(PrevI->getOpcode() == WebAssembly::BR);
+          PrevI->eraseFromParent();
+          ErasedUncondBr = true;
+          break;
+        }
+      }
+      assert(ErasedUncondBr && "Unconditional branch not erased!");
+    }
   }
 
   // When there are block / end_block markers that overlap with try / end_try
   // markers, and the block and try markers' return types are the same, the
   // block /end_block markers are not necessary, because try / end_try markers
   // also can serve as boundaries for branches.
   // block         <- Not necessary
   //   try

llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll

 ; REQUIRES: asserts
 ; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling | FileCheck %s
+; RUN: llc < %s -disable-wasm-fallthrough-return-opt -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling
 ; RUN: llc < %s -O0 -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -verify-machineinstrs -exception-model=wasm -mattr=+exception-handling | FileCheck %s --check-prefix=NOOPT
 ; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling -wasm-disable-ehpad-sort | FileCheck %s --check-prefix=NOSORT
 ; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling -wasm-disable-ehpad-sort -stats 2>&1 | FileCheck %s --check-prefix=NOSORT-STAT
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
 @_ZTIi = external constant i8*

 }
 
 ; We don't need to call placeBlockMarker after fixUnwindMismatches unless the
 ; destination is the appendix BB at the very end. This should not crash.
 define void @test16(i32* %p, i32 %a, i32 %b) personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
 entry:
   br label %loop
 
-loop:
+loop:                                             ; preds = %try.cont, %entry
   invoke void @foo()
           to label %bb0 unwind label %catch.dispatch0
 
-bb0:
+bb0:                                              ; preds = %loop
   %cmp = icmp ne i32 %a, %b
   br i1 %cmp, label %bb1, label %last
 
 bb1:                                              ; preds = %bb0
   invoke void @bar()
           to label %try.cont unwind label %catch.dispatch1
 
 catch.dispatch0:                                  ; preds = %loop
catch.dispatch1:                                  ; preds = %bb1
   %4 = catchswitch within none [label %catch.start1] unwind to caller
 
 catch.start1:                                     ; preds = %catch.dispatch1
   %5 = catchpad within %4 [i8* null]
   %6 = call i8* @llvm.wasm.get.exception(token %5)
   %7 = call i32 @llvm.wasm.get.ehselector(token %5)
   catchret from %5 to label %try.cont
 
-try.cont:                                         ; preds = %catch.start, %loop
+try.cont:                                         ; preds = %catch.start1, %catch.start0, %bb1
   br label %loop
 
-last:
+last:                                             ; preds = %bb0
+  ret void
+}
+
+; Tests if CFGStackify's removeUnnecessaryInstrs() removes unnecessary branches
+; correctly.
+; CHECK-LABEL: test17
+define void @test17(i32 %n) personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+entry:
+  invoke void @foo()
+          to label %for.body unwind label %catch.dispatch
+
+for.body:                                         ; preds = %for.end, %entry
+  %i = phi i32 [ %inc, %for.end ], [ 0, %entry ]
+  invoke void @foo()
+          to label %for.end unwind label %catch.dispatch
+
+; Before going to CFGStackify, this BB will have a conditional branch followed
+; by an unconditional branch. CFGStackify should remove only the unconditional
+; one.
+for.end:                                          ; preds = %for.body
+  %inc = add nuw nsw i32 %i, 1
+  %exitcond = icmp eq i32 %inc, %n
+  br i1 %exitcond, label %try.cont, label %for.body
+; CHECK: br_if
+; CHECK-NOT: br
+; CHECK: end_loop
+; CHECK: catch
+
+catch.dispatch:                                   ; preds = %for.body, %entry
+  %0 = catchswitch within none [label %catch.start] unwind to caller
+
+catch.start:                                      ; preds = %catch.dispatch
+  %1 = catchpad within %0 [i8* null]
+  %2 = call i8* @llvm.wasm.get.exception(token %1)
+  %3 = call i32 @llvm.wasm.get.ehselector(token %1)
+  %4 = call i8* @__cxa_begin_catch(i8* %2) #2 [ "funclet"(token %1) ]
+  call void @__cxa_end_catch() [ "funclet"(token %1) ]
+  catchret from %1 to label %try.cont
+
+try.cont:                                         ; preds = %catch.start, %for.end
   ret void
 }
 
 ; Check if the unwind destination mismatch stats are correct
 ; NOSORT-STAT: 16 wasm-cfg-stackify    - Number of EH pad unwind mismatches found
 
 declare void @foo()
 declare void @bar()