Change block and loop signature fixing

[jgravelle-google]

Summary

We add block signatures to blocks at the ends of functions in order to
satisfy the constraints around unreachable. The way we do so breaks down
with more complex cfgs, particularly those emitted by repeated calls to
setjmp/longjmp.
Change the fixup from adding a type signature, to adding an unreachable
instruction.

[jgravelle-google]

Abandoning this.
The underlying issue was in s2wasm's modeling of loops with type signatures (https://github.com/WebAssembly/binaryen/pull/810).
I still like this conceptually, but the difference is minor enough that the prudent thing is probably to leave it as-is.

lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp

 }
 
 /// In normal assembly languages, when the end of a function is unreachable,
 /// because the function ends in an infinite loop or a noreturn call or similar,
 /// it isn't necessary to worry about the function return type at the end of
 /// the function, because it's never reached. However, in WebAssembly, blocks
 /// that end at the function end need to have a return type signature that
 /// matches the function signature, even though it's unreachable. This function
-/// checks for such cases and fixes up the signatures.
+/// checks for such cases and inserts an explicit unreachable.
 static void FixEndsAtEndOfFunction(
     MachineFunction &MF,
     const WebAssemblyFunctionInfo &MFI,
-    DenseMap<const MachineInstr *, MachineInstr *> &BlockTops,
-    DenseMap<const MachineInstr *, MachineInstr *> &LoopTops) {
+    const WebAssemblyInstrInfo &TII) {
   assert(MFI.getResults().size() <= 1);
 
   if (MFI.getResults().empty())
     return;
 
   WebAssembly::ExprType retType;
   switch (MFI.getResults().front().SimpleTy) {
   case MVT::i32: retType = WebAssembly::I32; break;
   case MVT::i64: retType = WebAssembly::I64; break;
   case MVT::f32: retType = WebAssembly::F32; break;
   case MVT::f64: retType = WebAssembly::F64; break;
   case MVT::v16i8: retType = WebAssembly::I8x16; break;
   case MVT::v8i16: retType = WebAssembly::I16x8; break;
   case MVT::v4i32: retType = WebAssembly::I32x4; break;
   case MVT::v2i64: retType = WebAssembly::I64x2; break;
   case MVT::v4f32: retType = WebAssembly::F32x4; break;
   case MVT::v2f64: retType = WebAssembly::F64x2; break;
   default: llvm_unreachable("unexpected return type");
   }
 
   for (MachineBasicBlock &MBB : reverse(MF)) {
     for (MachineInstr &MI : reverse(MBB)) {
       if (MI.isPosition() || MI.isDebugValue())
         continue;
-      if (MI.getOpcode() == WebAssembly::END_BLOCK) {
-        BlockTops[&MI]->getOperand(0).setImm(int32_t(retType));
-        continue;
-      }
-      if (MI.getOpcode() == WebAssembly::END_LOOP) {
-        LoopTops[&MI]->getOperand(0).setImm(int32_t(retType));
-        continue;
+
+      if (MI.getOpcode() == WebAssembly::END_BLOCK ||
+          MI.getOpcode() == WebAssembly::END_LOOP) {
+        const MCInstrDesc &Desc = TII.get(WebAssembly::UNREACHABLE);
+        BuildMI(&MBB, MI.getDebugLoc(), Desc);
       }
-      // Something other than an `end`. We're done.
+
+      // Only need to consider the first non-debug machineinstr
       return;
     }
   }
 }
 
 /// Insert LOOP and BLOCK markers at appropriate places.
 static void PlaceMarkers(MachineFunction &MF, const MachineLoopInfo &MLI,
                          const WebAssemblyInstrInfo &TII,
for (auto &MI : reverse(MBB)) {
         break;
       }
     }
   }
   assert(Stack.empty() && "Control flow should be balanced");
 
   // Fix up block/loop signatures at the end of the function to conform to
   // WebAssembly's rules.
-  FixEndsAtEndOfFunction(MF, MFI, BlockTops, LoopTops);
+  FixEndsAtEndOfFunction(MF, MFI, TII);
 }
 
 bool WebAssemblyCFGStackify::runOnMachineFunction(MachineFunction &MF) {
   DEBUG(dbgs() << "********** CFG Stackifying **********\n"
                   "********** Function: "
                << MF.getName() << '\n');
 
   const auto &MLI = getAnalysis<MachineLoopInfo>();

test/CodeGen/WebAssembly/cfg-stackify.ll

 entry:
   store volatile i32 0, i32* %p
   ret i32 0
 }
 
 ; CHECK-LABEL: minimal_loop:
 ; CHECK-NOT: br
 ; CHECK: .LBB7_1:
-; CHECK: loop i32
+; CHECK: loop
 ; CHECK: i32.store 0($0), $pop{{[0-9]+}}{{$}}
 ; CHECK: br 0{{$}}
 ; CHECK: .LBB7_2:
+; CHECK: unreachable{{$}}
 ; OPT-LABEL: minimal_loop:
 ; OPT-NOT: br
 ; OPT: .LBB7_1:
-; OPT: loop i32
+; OPT: loop
 ; OPT: i32.store 0($0), $pop{{[0-9]+}}{{$}}
 ; OPT: br 0{{$}}
 ; OPT: .LBB7_2:
+; OPT: unreachable{{$}}
 define i32 @minimal_loop(i32* %p) {
 entry:
   store volatile i32 0, i32* %p
   br label %loop
 loop:
   store volatile i32 1, i32* %p
   br label %loop
 }
ft:
   br label %exit
 exit:
   store volatile i32 4, i32* %p
   ret i32 0
 }
 
 ; CHECK-LABEL: doublediamond_in_a_loop:
 ; CHECK: .LBB11_1:
-; CHECK: loop i32{{$}}
+; CHECK: loop   {{$}}
 ; CHECK: block   {{$}}
 ; CHECK: br_if           0, $0{{$}}
 ; CHECK: br              1{{$}}
 ; CHECK: .LBB11_3:
 ; CHECK: end_block{{$}}
 ; CHECK: block   {{$}}
 ; CHECK: br_if           0, $1{{$}}
 ; CHECK: br              1{{$}}
 ; CHECK: .LBB11_5:
 ; CHECK: br              0{{$}}
 ; CHECK: .LBB11_6:
 ; CHECK-NEXT: end_loop{{$}}
+; CHECK-NEXT: unreachable{{$}}
 ; OPT-LABEL: doublediamond_in_a_loop:
 ; OPT:      .LBB11_1:
-; OPT:      loop i32{{$}}
+; OPT:      loop   {{$}}
 ; OPT:      block   {{$}}
 ; OPT:      br_if           0, {{[^,]+}}{{$}}
 ; OPT:      block   {{$}}
 ; OPT:      br_if           0, {{[^,]+}}{{$}}
 ; OPT:      br              2{{$}}
 ; OPT-NEXT: .LBB11_4:
 ; OPT-NEXT: end_block{{$}}
 ; OPT:      br              1{{$}}
 ; OPT:      .LBB11_5:
 ; OPT-NEXT: end_block{{$}}
 ; OPT:      br              0{{$}}
 ; OPT:      .LBB11_6:
 ; OPT-NEXT: end_loop{{$}}
+; OPT-NEXT: unreachable{{$}}
 define i32 @doublediamond_in_a_loop(i32 %a, i32 %b, i32* %p) {
 entry:
   br label %header
 header:
   %c = icmp eq i32 %a, 0
   %d = icmp eq i32 %b, 0
   store volatile i32 0, i32* %p
   br i1 %c, label %true, label %false
u1:
   store volatile i32 5, i32* null
   unreachable
 }
 
 ; Test an interesting case using nested loops and switches.
 
 ; CHECK-LABEL: test8:
 ; CHECK:       .LBB17_1:
-; CHECK-NEXT:  loop i32{{$}}
+; CHECK-NEXT:  loop   {{$}}
 ; CHECK-NEXT:  i32.const $push{{[^,]+}}, 0{{$}}
 ; CHECK-NEXT:  br_if    0, {{[^,]+}}{{$}}
 ; CHECK-NEXT:  br       0{{$}}
 ; CHECK-NEXT:  .LBB17_2:
 ; CHECK-NEXT:  end_loop{{$}}
+; CHECK-NEXT:  unreachable{{$}}
 ; OPT-LABEL: test8:
 ; OPT:       .LBB17_1:
-; OPT-NEXT:  loop i32{{$}}
+; OPT-NEXT:  loop   {{$}}
 ; OPT-NEXT:  i32.const $push{{[^,]+}}, 0{{$}}
 ; OPT-NEXT:  br_if    0, {{[^,]+}}{{$}}
 ; OPT-NEXT:  br       0{{$}}
 ; OPT-NEXT:  .LBB17_2:
 ; OPT-NEXT:  end_loop{{$}}
+; OPT-NEXT:  unreachable{{$}}
 define i32 @test8() {
 bb:
   br label %bb1
 
 bb1:
   br i1 undef, label %bb2, label %bb3
 
 bb2:

 
 bb14:
   call void @test15_callee1()
   ret void
 
 bb15:
   ret void
 }
+
+; Test that nested loops and blocks typecheck correctly.
+
+; CHECK-LABEL: test16
+; CHECK:      loop
+; CHECK:      loop
+; CHECK-NEXT: block
+; CHECK:      end_block{{$}}
+; CHECK:      end_loop{{$}}
+; CHECK-NEXT: end_loop{{$}}
+; CHECK-NEXT: unreachable{{$}}
+; CHECK-NEXT: .endfunc{{$}}
+; OPT-LABEL: test16
+; OPT:      loop
+; OPT:      loop
+; OPT-NEXT: block
+; OPT:      end_block{{$}}
+; OPT:      end_loop{{$}}
+; OPT-NEXT: end_loop{{$}}
+; OPT-NEXT: unreachable{{$}}
+; OPT-NEXT: .endfunc{{$}}
[e-mail] = global i32 0
+define hidden i32 @test16() {
+entry:
+  br label %L1
+
+L1:
+  %x = load i32, i32* @test16global.ptr
+  %y = add i32 %x, 1
+  %cmp = icmp ne i32 %x, 2
+  br i1 %cmp, label %L2, label %L3
+
+L2:
+  store i32 %y, i32* @test16global.ptr
+  br i1 %cmp, label %L1, label %L3
+
+L3:
+  %z = add i32 %y, 3
+  store i32 %z, i32* @test16global.ptr
+  br label %L2
+}