[lld] Ensure NoTrapAfterNoreturn is false for the wasm backend (PR #65876)

[Matt Harding via llvm-commits]

================
@@ -1,33 +1,133 @@
-; RUN: llc < %s -asm-verbose=false -verify-machineinstrs | FileCheck %s
-; RUN: llc < %s -asm-verbose=false -fast-isel -fast-isel-abort=1 -verify-machineinstrs | FileCheck %s
-
-; Test that LLVM unreachable instruction and trap intrinsic are lowered to
-; wasm unreachable
+; RUN: llc < %s -verify-machineinstrs | FileCheck %s
+; RUN: llc < %s -fast-isel -fast-isel-abort=1 -verify-machineinstrs | FileCheck %s
+; RUN: llc < %s -verify-machineinstrs --trap-unreachable | FileCheck %s
+; RUN: llc < %s -fast-isel -fast-isel-abort=1 -verify-machineinstrs --trap-unreachable | FileCheck %s
+; RUN: llc < %s -verify-machineinstrs --trap-unreachable --no-trap-after-noreturn | FileCheck %s
+; RUN: llc < %s -fast-isel -fast-isel-abort=1 -verify-machineinstrs --trap-unreachable --no-trap-after-noreturn | FileCheck %s
 
 target triple = "wasm32-unknown-unknown"
 
-declare void @llvm.trap()
-declare void @llvm.debugtrap()
-declare void @abort()
 
-; CHECK-LABEL: f1:
-; CHECK: call abort{{$}}
-; CHECK: unreachable
-define i32 @f1() {
-  call void @abort()
-  unreachable
-}
+; Test that the LLVM trap and debug trap intrinsics are lowered to
+; wasm unreachable.
+
+declare void @llvm.trap() cold noreturn nounwind
+declare void @llvm.debugtrap() nounwind
 
-; CHECK-LABEL: f2:
-; CHECK: unreachable
-define void @f2() {
+define void @trap_ret_void() {
+; CHECK-LABEL: trap_ret_void:
+; CHECK:         .functype trap_ret_void () -> ()
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    # fallthrough-return
+; CHECK-NEXT:    end_function
   call void @llvm.trap()
   ret void
 }
 
-; CHECK-LABEL: f3:
-; CHECK: unreachable
-define void @f3() {
+define void @dtrap_ret_void() {
+; CHECK-LABEL: dtrap_ret_void:
+; CHECK:         .functype dtrap_ret_void () -> ()
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    # fallthrough-return
+; CHECK-NEXT:    end_function
   call void @llvm.debugtrap()
   ret void
 }
+
+; LLVM trap followed by LLVM unreachable could become exactly one
+; wasm unreachable, but two are emitted currently.
+define void @trap_unreach() {
+; CHECK-LABEL: trap_unreach:
+; CHECK:         .functype trap_unreach () -> ()
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    end_function
+  call void @llvm.trap()
+  unreachable
+}
+
+
+; Test that LLVM unreachable instruction is lowered to wasm unreachable when
+; necessary to fulfill the wasm operand stack requirements.
+
+declare void @ext_func()
+declare i32 @ext_func_i32()
+declare void @ext_never_return() noreturn
+
+; This test emits wasm unreachable to fill in for the missing i32 return value.
+define i32 @missing_ret_unreach() {
+; CHECK-LABEL: missing_ret_unreach:
+; CHECK:         .functype missing_ret_unreach () -> (i32)
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    call ext_func
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    end_function
+  call void @ext_func()
+  unreachable
+}
+
+; This is similar to the above test, but ensures wasm unreachable is emitted
+; even after a noreturn call.
+define i32 @missing_ret_noreturn_unreach() {
+; CHECK-LABEL: missing_ret_noreturn_unreach:
+; CHECK:         .functype missing_ret_noreturn_unreach () -> (i32)
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    call ext_never_return
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    end_function
+  call void @ext_never_return()
+  unreachable
+}
+
+; We could emit no instructions at all for the llvm unreachables in these next
+; three tests, as the signatures match and reaching llvm unreachable is
+; undefined behaviour. But wasm unreachable is emitted for the time being.
+
+define void @void_sig_match_unreach() {
+; CHECK-LABEL: void_sig_match_unreach:
+; CHECK:         .functype void_sig_match_unreach () -> ()
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    call ext_func
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    end_function
+  call void @ext_func()
+  unreachable
+}
+
+define i32 @i32_sig_match_unreach() {
+; CHECK-LABEL: i32_sig_match_unreach:
+; CHECK:         .functype i32_sig_match_unreach () -> (i32)
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    call ext_func_i32
+; CHECK-NEXT:    drop
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    end_function
+  call i32 @ext_func_i32()
+  unreachable
+}
+
+define void @void_sig_match_noreturn_unreach() {
+; CHECK-LABEL: void_sig_match_noreturn_unreach:
+; CHECK:         .functype void_sig_match_noreturn_unreach () -> ()
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    call ext_never_return
+; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    end_function
+  call void @ext_never_return()
+  unreachable
+}
+
+; This function currently doesn't emit unreachable.
----------------
majaha wrote:

>> The last function, @void_sig_match_noreturn_ret(), is an interesting example because the ret void is unreachable code, but doesn't get replaced with a wasm unreachable. It answers a question you might have, adding another data point to when it is and is not emitted.

> Not sure what question of mine you are referring to.

I didn't mean any question of yours in particular. I just meant that it provides information that a reader of the test might find useful.

> No unreachable is explicitly generated in the Wasm backend to handle noreturn functions. We just verbatim translate LLVM IR's unreachable to Wasm unreachable.

I'm aware that that's what's happening. However, this isn't relevant from the perspective of testing. We are testing the entire compiler, end-to-end. Whether correct compilation happens because of the wasm backend or in the common llvm middle-end code isn't relevant to the test, as long as the output is good.

The exact way that the wasm backend handles unreachable code could change (and probably should, as I suggested near the top of this page). The mark of a good test is that it continues to be useful if the code changes, and is uncoupled from the specific inner workings of the current code.

> I think it is sufficient to test this case, where unreachable happens right after a noreturn function, and not all the others that are not relevant. For example, not sure why we need to test the case where there is no LLVM unreachable to begin with. This PR is not related to those cases.

I know not all of the new tests are directly related to the NoTrapAfterNoreturn bug, but they are in the same area, and the old tests were rather scant. The way I see it, more tests and more exploration of the possibility space can't be a bad thing.

`@void_sig_match_noreturn_ret()` is probably the least useful one, and if you really don't like it I don't mind removing it.

https://github.com/llvm/llvm-project/pull/65876