Ensure NoTrapAfterNoreturn is false for the wasm backend

[majaha]

In the WebAssembly back end, the TrapUnreachable option is currently load-bearing for correctness, inserting wasm unreachable instructions where needed to create valid wasm. There is another option, NoTrapAfterNoreturn, that removes some of those traps and causes incorrect wasm to be emitted.

The first commit adds a command line flag for NoTrapAfterNoreturn so that it can be tested, and adds new tests (and fixes some typos).
The second commit turns off NoTrapAfterNoreturn for the Wasm backend.
The third commit adds a new peephole optimisation, to remove some common cases of unnecessary instructions around unreachable. Properly modelling how unreachable can be a sink and a source for operands would be better, but this will do for now.

[majaha]

As an aside, here's a thought that occurred to me while reading through the code: It would be nice if LLVM IR's unreachable was instruction-selected to an ISD::UNDEFINED_BEHAVIOUR pseudo-instruction (when TrapUnreachable is off). Physical ISA backends would lower it to nothing, but wasm could lower it to wasm unreachable or nothing, depending on what was needed to fulfil the stack requirement. I didn't feel brave enough to attempt that though :)

[llvmbot]

@llvm/pr-subscribers-lld-wasm

Changes

In the WebAssembly back end, the TrapUnreachable option is currently load-bearing for correctness, inserting wasm unreachable instructions where needed to create valid wasm. There is another option, NoTrapAfterNoreturn, that removes some of those traps and causes incorrect wasm to be emitted.

The first commit adds a command line flag for NoTrapAfterNoreturn so that it can be tested, and adds new tests (and fixes some typos).
The second commit turns off NoTrapAfterNoreturn for the Wasm backend.
The third commit adds a new peephole optimisation, to remove some common cases of unnecessary instructions around unreachable. Properly modelling how unreachable can be a sink and a source for operands would be better, but this will do for now.

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

10 Files Affected:

• (modified) lld/test/wasm/init-fini.ll (+1-1)
• (modified) llvm/include/llvm/CodeGen/MachineFunction.h (+1-1)
• (modified) llvm/include/llvm/CodeGen/MachineInstr.h (+1-1)
• (modified) llvm/lib/CodeGen/LLVMTargetMachine.cpp (+7)
• (modified) llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp (+1-1)
• (modified) llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp (+11)
• (modified) llvm/lib/Target/WebAssembly/WebAssemblyPeephole.cpp (+51)
• (modified) llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp (+1)
• (modified) llvm/test/CodeGen/WebAssembly/unreachable.ll (+116-21)
• (modified) llvm/test/MC/WebAssembly/global-ctor-dtor.ll (+6-6)

diff --git a/lld/test/wasm/init-fini.ll b/lld/test/wasm/init-fini.ll
index 14385f042efb7c2..35286bbcccc2f54 100644
--- a/lld/test/wasm/init-fini.ll
+++ b/lld/test/wasm/init-fini.ll
@@ -77,7 +77,7 @@ entry:
 ; CHECK-NEXT:         Body:            10031004100A100F1012100F10141003100C100F10161001100E0B
 ; CHECK:            - Index:           22
 ; CHECK-NEXT:         Locals:
-; CHECK-NEXT:         Body:            02404186808080004100418088808000108780808000450D0000000B0B
+; CHECK-NEXT:         Body:            02404186808080004100418088808000108780808000450D00000B0B
 ; CHECK-NEXT:   - Type:            CUSTOM
 ; CHECK-NEXT:     Name:            name
 ; CHECK-NEXT:     FunctionNames:
diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h
index 6c2da626ea54b4d..8f1651c2958e591 100644
--- a/llvm/include/llvm/CodeGen/MachineFunction.h
+++ b/llvm/include/llvm/CodeGen/MachineFunction.h
@@ -266,7 +266,7 @@ class LLVM_EXTERNAL_VISIBILITY MachineFunction {
   // RegInfo - Information about each register in use in the function.
   MachineRegisterInfo *RegInfo;
 
-  // Used to keep track of target-specific per-machine function information for
+  // Used to keep track of target-specific per-machine-function information for
   // the target implementation.
   MachineFunctionInfo *MFInfo;
 
diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index 03fb15f77c65cbb..8367f999fcc76d3 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1276,7 +1276,7 @@ class MachineInstr
   /// eraseFromBundle() to erase individual bundled instructions.
   void eraseFromParent();
 
-  /// Unlink 'this' form its basic block and delete it.
+  /// Unlink 'this' from its basic block and delete it.
   ///
   /// If the instruction is part of a bundle, the other instructions in the
   /// bundle remain bundled.
diff --git a/llvm/lib/CodeGen/LLVMTargetMachine.cpp b/llvm/lib/CodeGen/LLVMTargetMachine.cpp
index d02ec1db1165d41..aadc3709b85bfb7 100644
--- a/llvm/lib/CodeGen/LLVMTargetMachine.cpp
+++ b/llvm/lib/CodeGen/LLVMTargetMachine.cpp
@@ -37,6 +37,11 @@ static cl::opt
     EnableTrapUnreachable("trap-unreachable", cl::Hidden,
                           cl::desc("Enable generating trap for unreachable"));
 
+static cl::opt
+    EnableNoTrapAfterNoreturn("no-trap-after-noreturn", cl::Hidden,
+                              cl::desc("Do not emit a trap instruction for 'unreachable' IR instructions "
+                              "after noreturn calls, even if --trap-unreachable is set."));
+
 void LLVMTargetMachine::initAsmInfo() {
   MRI.reset(TheTarget.createMCRegInfo(getTargetTriple().str()));
   assert(MRI && "Unable to create reg info");
@@ -95,6 +100,8 @@ LLVMTargetMachine::LLVMTargetMachine(const Target &T,
 
   if (EnableTrapUnreachable)
     this->Options.TrapUnreachable = true;
+  if (EnableNoTrapAfterNoreturn)
+    this->Options.NoTrapAfterNoreturn = true;
 }
 
 TargetTransformInfo
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
index 131e99c66fa2e5a..d8cbddf74545da6 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
@@ -667,7 +667,7 @@ void WebAssemblyCFGStackify::removeUnnecessaryInstrs(MachineFunction &MF) {
 
   // 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.
+  // if there is no exception, the control flow transfers to that point anyway.
   // bb0:
   //   try
   //     ...
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
index 4a75bab6b95ddcd..eb7d5b2cfd2a4b3 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
@@ -122,9 +122,20 @@ bool WebAssemblyDebugFixup::runOnMachineFunction(MachineFunction &MF) {
           // it will be culled later.
         }
       } else {
+        
+        // WebAssembly Peephole optimisation can remove instructions around wasm unreachable.
+        // This is valid for wasm, as unreachable is operand stack polymorphic. But this is not modeled
+        // in llvm at the moment, and so the stack may not seem to pop all that it pushes.
+        // Clear the stack so we don't violate the assert(Stack.empty()) later on.
+        if (MI.getOpcode() == WebAssembly::UNREACHABLE) {
+          Stack.clear();
+          break;
+        }
+
         // Track stack depth.
         for (MachineOperand &MO : reverse(MI.explicit_uses())) {
           if (MO.isReg() && MFI.isVRegStackified(MO.getReg())) {
+            assert(Stack.size() != 0 && "WebAssemblyDebugFixup: Pop: Operand stack empty!");
             auto Prev = Stack.back();
             Stack.pop_back();
             assert(Prev.Reg == MO.getReg() &&
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyPeephole.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyPeephole.cpp
index 6e2d566d9b48630..a573f0d86436e58 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyPeephole.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyPeephole.cpp
@@ -20,6 +20,7 @@
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
+#include 
 using namespace llvm;
 
 #define DEBUG_TYPE "wasm-peephole"
@@ -109,6 +110,53 @@ static bool maybeRewriteToFallthrough(MachineInstr &MI, MachineBasicBlock &MBB,
   return true;
 }
 
+static bool eraseDeadCodeAroundUnreachable(MachineInstr &UnreachbleMI, MachineBasicBlock &MBB) {
+  SmallVector ToDelete;
+
+  // Because wasm unreachable is stack polymorphic and unconditionally ends control,
+  // all instructions after it can be removed until the end of this block.
+  // We remove the common case of double unreachable.
+  auto ForwardsIterator = UnreachbleMI.getIterator();
+  for (ForwardsIterator++; !ForwardsIterator.isEnd(); ForwardsIterator++) {
+    MachineInstr& MI = *ForwardsIterator;
+    if (MI.getOpcode() == WebAssembly::UNREACHABLE) {
+      ToDelete.push_back(&MI);
+    } else {
+      break;
+    }
+  }
+
+  [&]() {
+    // For the same reasons as above, previous instructions that only affect
+    // local function state can be removed (e.g. local.set, drop, various reads).
+    // We remove the common case of "drop unreachable".
+    auto BackwardsIterator = UnreachbleMI.getReverseIterator();
+    for (BackwardsIterator++; !BackwardsIterator.isEnd(); BackwardsIterator++) {
+      MachineInstr& MI = *BackwardsIterator;
+      switch(MI.getOpcode()) {
+      case WebAssembly::DROP_I32:
+      case WebAssembly::DROP_I64:
+      case WebAssembly::DROP_F32:
+      case WebAssembly::DROP_F64:
+      case WebAssembly::DROP_EXTERNREF:
+      case WebAssembly::DROP_FUNCREF:
+      case WebAssembly::DROP_V128:
+        ToDelete.push_back(&MI);
+        continue;
+      default:
+        return;
+      }
+    }
+  }();
+
+  bool Changed = false;
+  for (MachineInstr* MI : ToDelete) {
+    MI->eraseFromParent();
+    Changed = true;
+  }
+  return Changed;
+}
+
 bool WebAssemblyPeephole::runOnMachineFunction(MachineFunction &MF) {
   LLVM_DEBUG({
     dbgs() << "********** Peephole **********\n"
@@ -159,6 +207,9 @@ bool WebAssemblyPeephole::runOnMachineFunction(MachineFunction &MF) {
       case WebAssembly::RETURN:
         Changed |= maybeRewriteToFallthrough(MI, MBB, MF, MFI, MRI, TII);
         break;
+      case WebAssembly::UNREACHABLE:
+        Changed |= eraseDeadCodeAroundUnreachable(MI, MBB);
+        break;
       }
 
   return Changed;
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
index f8a4b95a95515e4..184910ae68f15e6 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
@@ -127,6 +127,7 @@ WebAssemblyTargetMachine::WebAssemblyTargetMachine(
   // LLVM 'unreachable' to ISD::TRAP and then lower that to WebAssembly's
   // 'unreachable' instructions which is meant for that case.
   this->Options.TrapUnreachable = true;
+  this->Options.NoTrapAfterNoreturn = false;
 
   // WebAssembly treats each function as an independent unit. Force
   // -ffunction-sections, effectively, so that we can emit them independently.
diff --git a/llvm/test/CodeGen/WebAssembly/unreachable.ll b/llvm/test/CodeGen/WebAssembly/unreachable.ll
index ad1c90090ac58bf..38b8f543dffd972 100644
--- a/llvm/test/CodeGen/WebAssembly/unreachable.ll
+++ b/llvm/test/CodeGen/WebAssembly/unreachable.ll
@@ -1,33 +1,128 @@
-; 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
 }
+
+; Test that LLVM trap followed by LLVM unreachable becomes exactly one wasm unreachable.
+define void @trap_unreach() {
+; CHECK-LABEL: trap_unreach:
+; CHECK:         .functype trap_unreach () -> ()
+; CHECK-NEXT:  # %bb.0:
+; 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:    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.
+define void @void_sig_match_noreturn_ret() {
+; CHECK-LABEL: void_sig_match_noreturn_ret:
+; CHECK:         .functype void_sig_match_noreturn_ret () -> ()
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    call ext_never_return
+; CHECK-NEXT:    # fallthrough-return
+; CHECK-NEXT:    end_function
+  call void @ext_never_return()
+  ret void
+}
diff --git a/llvm/test/MC/WebAssembly/global-ctor-dtor.ll b/llvm/test/MC/WebAssembly/global-ctor-dtor.ll
index bc1be7931349697..f1ec71da1ebb641 100644
--- a/llvm/test/MC/WebAssembly/global-ctor-dtor.ll
+++ b/llvm/test/MC/WebAssembly/global-ctor-dtor.ll
@@ -80,29 +80,29 @@ declare void @func3()
 ; CHECK-NEXT:         Offset:          0x1D
 ; CHECK-NEXT:       - Type:            R_WASM_FUNCTION_INDEX_LEB
 ; CHECK-NEXT:         Index:           6
-; CHECK-NEXT:         Offset:          0x2C
+; CHECK-NEXT:         Offset:          0x2B
 ; CHECK-NEXT:       - Type:            R_WASM_TABLE_INDEX_SLEB
 ; CHECK-NEXT:         Index:           5
-; CHECK-NEXT:         Offset:          0x37
+; CHECK-NEXT:         Offset:          0x36
 ; CHECK-NEXT:       - Type:            R_WASM_MEMORY_ADDR_SLEB
 ; CHECK-NEXT:         Index:           3
-; CHECK-NEXT:         Offset:          0x3F
+; CHECK-NEXT:         Offset:          0x3E
 ; CHECK-NEXT:       - Type:            R_WASM_FUNCTION_INDEX_LEB
 ; CHECK-NEXT:         Index:           4
-; CHECK-NEXT:         Offset:          0x45
+; CHECK-NEXT:         Offset:          0x44
 ; CHECK-NEXT:     Functions:
 ; CHECK-NEXT:       - Index:           5
 ; CHECK-NEXT:         Locals:
 ; CHECK-NEXT:         Body:            1080808080000B
 ; CHECK-NEXT:       - Index:           6
 ; CHECK-NEXT:         Locals:
-; CHECK-NEXT:         Body:            02404181808080004100418080808000108180808000450D0000000B0B
+; CHECK-NEXT:         Body:            02404181808080004100418080808000108180808000450D00000B0B
 ; CHECK-NEXT:       - Index:           7
 ; CHECK-NEXT:         Locals:
 ; CHECK-NEXT:         Body:            1082808080000B
 ; CHECK-NEXT:       - Index:           8
 ; CHECK-NEXT:         Locals:
-; CHECK-NEXT:         Body:            02404182808080004100418080808000108180808000450D0000000B0B
+; CHECK-NEXT:         Body:            02404182808080004100418080808000108180808000450D00000B0B
 ; CHECK-NEXT:   - Type:            DATA
 ; CHECK-NEXT:     Segments:
 ; CHECK-NEXT:       - SectionOffset:   6

[aheejin]

Haven't read the code yet, but I wonder, why can't we turn NoTrapAfterNoreturn on? Do we need unreachable after noreturn calls?

[aheejin]

Also please run clang-format

[majaha]

Do we need unreachable after noreturn calls?

We do for most cases, because the body of a wasm function has to pass wasm validation. This means that the body has to have the same "type" as the function signature, even if the end of the function is never reached. Wasm unreachable is a special kind of instruction that can have any "type", and so it can be used as an easy way to fulfil the operand stack typing requirements. Essentially we're saying "Nothing can get past this point, so it doesn't matter that this functions body doesn't match its signature."

This is something that native-style architectures don't require, which is kinda what I was getting at with my earlier comment about an UB instruction, to better support these kinds of virtual ISAs.

[majaha]

Hmm, I'm not sure how I could create another pull request that is stacked on top of this one for the peephole optimisation. Searching around in the LLVM discord, it seems like that's a bit of a pain point at the moment.

[aheejin]

Hmm, I'm not sure how I could create another pull request that is stacked on top of this one for the peephole optimisation. Searching around in the LLVM discord, it seems like that's a bit of a pain point at the moment.

I don't think the stacked PR feature exists in Github, which was a nice feature in Phabricator. You can either upload the next PR now that includes both PR's changes and rebase after this lands, or you can wait until this lands first and then create a new PR. Oh, I have seen my colleague using this plugin (https://app.graphite.dev/) for stacked PRs, but I haven't used it myself and don't know how to use it.

[majaha]

Here's the new pull request for the peephole optimisation: #66062

[majaha]

Are there any more thoughts, opinions, or questions about this PR?

[nikic]

This looks generally fine to me, but I think for the new -no-trap-after-noreturn option we should introduce that in a separate PR that actually tests the behavior in a case where it does something. It's weird to only test the option in a place where it doesn't actually do anything.

[majaha]

Do you mean that there should be a test that checks that the --no-trap-after-noreturn command line option actually enables the behaviour it's supposed to?

Would that be a unit test, or an llvm-lit style test? Where might that test live?

After a quick search the closest thing I could find for --trap-unreachable is this: https://github.com/llvm/llvm-project/blob/116f7a2dcb86a7a8812a60fb7101f90329dada19/llvm/test/CodeGen/ARM/trap-unreachable.ll
which tests the effect on the ARM backend. There's also one for the Hexagon backend. Is this the kind of test you had in mind?

[aheejin]

Sorry for the long delay; I was OOO for a while.

Sorry again; I'm still having a hard time understanding what this PR is trying to achieve. See inline comments.

[majaha]

@aheejin I suggest looking at each of the first three commits individually. I carefully crafted and stacked them so that the test passes after each commit. That way, the changes to the test declaratively describe the effect of each commit.
You can click on each commit in turn near the top of this page. Also, when looking at changes, there's a little pulldown near the top right that says Changes from * commits that you can use to flick through the different commits.

I probably should have mentioned that earlier. It seems like GitHub doesn't surface this stacked-commit style information as well as it could. I was following nikic's guide for the old pull request system, where I assume this style was better supported.

I'll give a quick overview of what each of the first three commits do:

1. Add no-trap-after-noreturn flag and wasm tests
   This commit doesn't actually contain any real changes. It simply adds some new tests in unreachable.ll that describe the current state of affairs, and adds the command line option --no-trap-after-noreturn to support those tests. This option was previously usable from C++ projects (clang, rustc etc.), but not from the command line, making it untestable (and thus buggy!).
   In particular, the function @missing_ret_noreturn_unreach() produces invalid Wasm code when NoTrapAfterNoreturn is turned on: it's missing a necessary unreachable.

2. Ensure NoTrapAfterNoreturn is false for wasm
   This is the bug fix commit. I did the simplest possible thing to fix the bug: copy the behaviour for TargetOptions.TrapUnreachable and overwrite the value of TargetOptions.NoTrapAfterNoreturn with false*. Note that TargetOptions is part of the public API and that these options can be set to anything by llvm's embedders (i.e. clang, rustc).
   This is a bit of a hack, but a hack that already existed.
   As you can see in the tests, this makes all combinations of the two command line options output the same code, and fixes the invalid output. But there's still those pesky double unreachables, which leads to the next commit:

3. Add peephole optimisation
   This commit adds a simple peephole optimisation that removes unnecessary drops and unreachables near other unreachables. It would be nice if the wasm backend was able to model these properly so as not to add them in the first place, but this fixes the most obvious cases well enough for now. This commit has been reverted and moved to a different PR.

* Incidently, is this already buggy? We are overwriting our user's TargetOptions, and so if they re-used that object for a different backend they might be surprised by it changing on them. Should we be copying the whole TargetOptions instead?

[majaha]

@nikic I've split off the --no-trap-after-noreturn flag change into a separate PR: #67051
Is this the kind of change you had in mind?

[aheejin]

There's no difference between "doing nothing with respect to it" and "silently rejecting", it's the same thing. The wasm backend understands neither --xcoff-traceback-table, --trap-unreachable, or --no-trap-after-noreturn (the fact that --no-trap-after-noreturn affects the wasm output despite being conceptually unsupported is the bug). None of those are supported by the wasm backend, and they are all equally silently ignored.

I don't think they are the same thing. As I said above, --xcoff-traceback-table doesn't apply to Wasm because Wasm does not use xcoff. But --trap-unreachable or --no-trap-after-noreturn are applicable to wasm and we override them. I don't understand the analogy.

I have been kind of confused by your characterization of this as "bugfix", given that this does not change anything functionality-wise.
...So I'm not sure if I agree that this PR is a bugfix. I consider this as an improvement adding a failsafe mechanism.

It does change things functionality wise. Look at the tests, the output has changed with this pull request from invalid to valid. If that's not a bug fix, I don't know what is.

But --no-trap-after-noreturn didn't exist before, so there was no way to specify that from the command line. You created it, originally in this PR, and then the split-off PR in #67051. If this is a bugfix, it sounds like you are fixing a bug of your own making.

The reason I said this might count as an improvement was, in a hypothetical scenario when the default value for NoTrapAfterNoreturn changes, or someone else (if you didn't create that option) wants to add the same command line option, this overriding can guard us from those changes. But I think these scenarios are basically hypothetical and unlikely. So while I'm not opposed to these changes, I am confused by the motivation of this PR (together with #67051) and then more about the claim this is a bugfix.

What's your distinction criteria for "real contradiction" and "hints or suggestions"?

A real contradiction would be asking for two incompatible target options, like -exception-model=wasm -enable-emscripten-cxx-exceptions. TrapUnreachable is a much more wishy-washy request, it's frequently ignored across the codebase, and using it doesn't even guarantee that unreachable will produce a trap.

This is a case where the unreachable was optimized away or transformed to something else in opt, even before reaching the backend, so I don't think this counts as "we don't guarantee thatunreachable will produce a trap". We (the Wasm backend) doesn't even see the unreachable. (Of course, it is entirely possible that we optimize out some code blocks containing unreachable and don't end up producing a trap corresponding to that within our Wasm backend. Anyway what I'm saying is optimizing away unreachable doesn't conflict the command-line option.)

Also I'm having a hard time understanding your criteria between wishy-washy request and real contradiction still. To my understanding in both cases they are not compatible.

To put it plainly: the --trap-unreachable and --no-trap-after-noreturn options are not supported on the wasm backend, and they are silently ignored just like every other unsupported option. This is consistent with the behaviour both in the wasm backend, and across the codebase: 1, 2, 3.

Yeah I get that we haven't been warning about a similar case (--trap-unreachable) before. But I think they are more of what ended up happen, and not the firm intention not to warn for conflicted requests.

For example, that --trap-unreachable command line option was added much later (6d9f8c9) than when we set it to false (ffa143c). When first we set it to false in 2015, that option didn't exist so we had nothing to warn against. And someone who added that option in 2018 didn't go through all the code to check if there were any conflicting assignments.

[sbc100]

But --no-trap-after-noreturn didn't exist before, so there was no way to specify that from the command line. You created it, originally in this PR, and then the split-off PR in #67051. If this is a bugfix, it sounds like you are fixing a bug of your own making.

I think perhaps the misunderstanding here is that NoTrapAfterNoreturn was usable/settable by compilers such as rust via the LLVM API, even though --no-trap-after-noreturn didn't exist in llvm itself?

So adding this option allows us to test something that wasn't testable via the command line, but was exposed via the API? Is that right?

[aheejin]

But --no-trap-after-noreturn didn't exist before, so there was no way to specify that from the command line. You created it, originally in this PR, and then the split-off PR in #67051. If this is a bugfix, it sounds like you are fixing a bug of your own making.

I think perhaps the misunderstanding here is that NoTrapAfterNoreturn was usable/settable by compilers such as rust via the LLVM API, even though --no-trap-after-noreturn didn't exist in llvm itself?

So adding this option allows us to test something that wasn't testable via the command line, but was exposed via the API? Is that right?

Ah, so it was already possible to set NoTrapAfterNoreturn=0 via some API. I couldn't find one in the LLVM codebase that allowed it but I guess I don't know about Rust API part. Thanks for point that out, and sorry If I missed that. @majaha

[majaha]

Yeah you've got it, that's the main point. It can be set via the C++ API, and when I tried doing so in the Rust compiler I found it broke the WebAssembly backend. Sorry if I didn't make that clear enough earlier, I'm glad we're on the same page now.

[aheejin]

Yeah you've got it, that's the main point. It can be set via the C++ API, and when I tried doing so in the Rust compiler I found it broke the WebAssembly backend. Sorry if I didn't make that clear enough earlier, I'm glad we're on the same page now.

The other points I tried to say in #65876 (comment) stay the same though.

[majaha]

Yeah I get that we haven't been warning about a similar case (--trap-unreachable) before. But I think they are more of what ended up happen, and not the firm intention not to warn for conflicted requests.

For example, that --trap-unreachable command line option was added much later (6d9f8c9) than when we set it to false (ffa143c). When first we set it to false in 2015, that option didn't exist so we had nothing to warn against. And someone who added that option in 2018 didn't go through all the code to check if there were any conflicting assignments.

You might be right, but my feeling is that the behaviour of --trap-unreachable and --no-trap-after-noreturn should be consistent across all backends, and probably with other options too. I think that's a big enough change to warrant discussion and testing separately.

This change is consistent with the current behaviour and fixes a real issue, so I think it makes sense for this change to go in first, before we start to think about changing any existing behaviour.

[aheejin]

Yeah I get that we haven't been warning about a similar case (--trap-unreachable) before. But I think they are more of what ended up happen, and not the firm intention not to warn for conflicted requests.
For example, that --trap-unreachable command line option was added much later (6d9f8c9) than when we set it to false (ffa143c). When first we set it to false in 2015, that option didn't exist so we had nothing to warn against. And someone who added that option in 2018 didn't go through all the code to check if there were any conflicting assignments.

You might be right, but my feeling is that the behaviour of --trap-unreachable and --no-trap-after-noreturn should be consistent across all backends, and probably with other options too. I think that's a big enough change to warrant discussion and testing separately.

This change is consistent with the current behaviour and fixes a real issue, so I think it makes sense for this change to go in first, before we start to think about changing any existing behaviour.

OK fair enough. Sorry that this PR is taking so long time. I guess we can land this after we fix some tests / add some comments. I still think the test file needs more comments; I would like that the readers of the test file find it easy enough to figure out what the purpose of the tests in that file is, without referring to git blames and PR discussions.

[majaha]

That's a good point, I'll add some more explanatory comments.

[aheejin]

No newline at the end

[aheejin]

Thanks for your patience! Will merge this.