[WebAssembly] remove instruction after builtin trap #90207

HerrCai0907 · 2024-04-26T12:56:08Z

llvm.trap will be convert as unreachable which is terminator. Instruction after terminator will cause validation failed.
This PR introduces a pass to clean instruction after terminator.
Fixes: #68770

llvmbot · 2024-04-26T12:56:37Z

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-webassembly

Author: Congcong Cai (HerrCai0907)

Changes

llvm.trap will be convert as unreachable which is termiantor. But we cannot append any instruction after terminator wihch will casue llir's validation failed.
This PR introduces a pass to clean instruction after terminator.
Fixes: #68770

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

6 Files Affected:

(modified) llvm/lib/Target/WebAssembly/CMakeLists.txt (+1)
(modified) llvm/lib/Target/WebAssembly/WebAssembly.h (+2)
(added) llvm/lib/Target/WebAssembly/WebAssemblyCleanCodeAfterTrap.cpp (+90)
(modified) llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp (+4)
(modified) llvm/test/CodeGen/WebAssembly/unreachable.ll (+9-2)
(modified) llvm/test/MC/WebAssembly/global-ctor-dtor.ll (+6-6)

diff --git a/llvm/lib/Target/WebAssembly/CMakeLists.txt b/llvm/lib/Target/WebAssembly/CMakeLists.txt
index f430be2653b4ee..1e83cbeac50d6d 100644
--- a/llvm/lib/Target/WebAssembly/CMakeLists.txt
+++ b/llvm/lib/Target/WebAssembly/CMakeLists.txt
@@ -19,6 +19,7 @@ add_llvm_target(WebAssemblyCodeGen
   WebAssemblyArgumentMove.cpp
   WebAssemblyAsmPrinter.cpp
   WebAssemblyCFGStackify.cpp
+  WebAssemblyCleanCodeAfterTrap.cpp
   WebAssemblyCFGSort.cpp
   WebAssemblyDebugFixup.cpp
   WebAssemblyDebugValueManager.cpp
diff --git a/llvm/lib/Target/WebAssembly/WebAssembly.h b/llvm/lib/Target/WebAssembly/WebAssembly.h
index 1c40addb6d6f78..7fc8546248f164 100644
--- a/llvm/lib/Target/WebAssembly/WebAssembly.h
+++ b/llvm/lib/Target/WebAssembly/WebAssembly.h
@@ -37,6 +37,7 @@ FunctionPass *createWebAssemblyISelDag(WebAssemblyTargetMachine &TM,
                                        CodeGenOptLevel OptLevel);
 FunctionPass *createWebAssemblyArgumentMove();
 FunctionPass *createWebAssemblySetP2AlignOperands();
+FunctionPass *createWebAssemblyCleanCodeAfterTrap();
 
 // Late passes.
 FunctionPass *createWebAssemblyReplacePhysRegs();
@@ -63,6 +64,7 @@ void initializeOptimizeReturnedPass(PassRegistry &);
 void initializeWebAssemblyRefTypeMem2LocalPass(PassRegistry &);
 void initializeWebAssemblyAddMissingPrototypesPass(PassRegistry &);
 void initializeWebAssemblyArgumentMovePass(PassRegistry &);
+void initializeWebAssemblyCleanCodeAfterTrapPass(PassRegistry &);
 void initializeWebAssemblyCFGSortPass(PassRegistry &);
 void initializeWebAssemblyCFGStackifyPass(PassRegistry &);
 void initializeWebAssemblyDAGToDAGISelPass(PassRegistry &);
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCleanCodeAfterTrap.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCleanCodeAfterTrap.cpp
new file mode 100644
index 00000000000000..426254b2e5f655
--- /dev/null
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCleanCodeAfterTrap.cpp
@@ -0,0 +1,90 @@
+//===-- WebAssemblyCleanCodeAfterTrap.cpp - Argument instruction moving ---------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file moves ARGUMENT instructions after ScheduleDAG scheduling.
+///
+/// Arguments are really live-in registers, however, since we use virtual
+/// registers and LLVM doesn't support live-in virtual registers, we're
+/// currently making do with ARGUMENT instructions which are placed at the top
+/// of the entry block. The trick is to get them to *stay* at the top of the
+/// entry block.
+///
+/// The ARGUMENTS physical register keeps these instructions pinned in place
+/// during liveness-aware CodeGen passes, however one thing which does not
+/// respect this is the ScheduleDAG scheduler. This pass is therefore run
+/// immediately after that.
+///
+/// This is all hopefully a temporary solution until we find a better solution
+/// for describing the live-in nature of arguments.
+///
+//===----------------------------------------------------------------------===//
+
+#include "WebAssembly.h"
+#include "WebAssemblyUtilities.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
+#include "llvm/CodeGen/Passes.h"
+#include "llvm/MC/MCInstrDesc.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/raw_ostream.h"
+using namespace llvm;
+
+#define DEBUG_TYPE "wasm-clean-code-after-trap"
+
+namespace {
+class WebAssemblyCleanCodeAfterTrap final : public MachineFunctionPass {
+public:
+  static char ID; // Pass identification, replacement for typeid
+  WebAssemblyCleanCodeAfterTrap() : MachineFunctionPass(ID) {}
+
+  StringRef getPassName() const override { return "WebAssembly Clean Unreachable Code After Trap"; }
+
+  bool runOnMachineFunction(MachineFunction &MF) override;
+};
+} // end anonymous namespace
+
+char WebAssemblyCleanCodeAfterTrap::ID = 0;
+INITIALIZE_PASS(WebAssemblyCleanCodeAfterTrap, DEBUG_TYPE,
+                "Clean unreachable code after trap instruction", false, false)
+
+FunctionPass *llvm::createWebAssemblyCleanCodeAfterTrap() {
+  return new WebAssemblyCleanCodeAfterTrap();
+}
+
+bool WebAssemblyCleanCodeAfterTrap::runOnMachineFunction(MachineFunction &MF) {
+  LLVM_DEBUG({
+    dbgs() << "********** CleanUnreachableCodeAfterTrap **********\n"
+           << "********** Function: " << MF.getName() << '\n';
+  });
+
+  bool Changed = false;
+
+  for (MachineBasicBlock & BB : MF) {
+    bool HasTerminator = false;
+    llvm::SmallVector<MachineInstr*> RemoveMI{};
+    for (MachineInstr & MI : BB) {
+      if (HasTerminator)
+        RemoveMI.push_back(&MI);
+      if (MI.hasProperty(MCID::Trap) && MI.isTerminator())
+        HasTerminator = true;
+    }
+    if (!RemoveMI.empty()) {
+      Changed = true;
+      LLVM_DEBUG({
+        for (MachineInstr *MI : RemoveMI) {
+          llvm::dbgs() << "* remove ";
+          MI->print(llvm::dbgs());
+        }
+      });
+      for (MachineInstr * MI : RemoveMI)
+        MI->eraseFromParent();
+    }
+  }
+  return Changed;
+}
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
index cdd39eeb6bbbc2..de342e89657367 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
@@ -512,6 +512,10 @@ bool WebAssemblyPassConfig::addInstSelector() {
   // Eliminate range checks and add default targets to br_table instructions.
   addPass(createWebAssemblyFixBrTableDefaults());
 
+  // unreachable is terminator, non-terminator instruction after it is not
+  // allowed.
+  addPass(createWebAssemblyCleanCodeAfterTrap());
+
   return false;
 }
 
diff --git a/llvm/test/CodeGen/WebAssembly/unreachable.ll b/llvm/test/CodeGen/WebAssembly/unreachable.ll
index 5368c2ba5b8dc1..ccac31a9af4a3d 100644
--- a/llvm/test/CodeGen/WebAssembly/unreachable.ll
+++ b/llvm/test/CodeGen/WebAssembly/unreachable.ll
@@ -30,7 +30,6 @@ define void @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
@@ -54,7 +53,6 @@ define void @trap_unreacheable() {
 ; CHECK:         .functype trap_unreacheable () -> ()
 ; CHECK-NEXT:  # %bb.0:
 ; CHECK-NEXT:    unreachable
-; CHECK-NEXT:    unreachable
 ; CHECK-NEXT:    end_function
   call void @llvm.trap()
   unreachable
@@ -94,3 +92,12 @@ define i32 @missing_ret_noreturn_unreachable() {
   call void @ext_never_return()
   unreachable
 }
+
+define i32 @no_crash_for_other_instruction_after_trap(ptr %p, i32 %b) {
+; CHECK-LABEL: no_crash_for_other_instruction_after_trap:
+; CHECK:      unreachable
+; CHECK-NEXT: end_function
+  %a = load i32, ptr %p
+  call void @llvm.trap()
+  ret i32 %a
+}
diff --git a/llvm/test/MC/WebAssembly/global-ctor-dtor.ll b/llvm/test/MC/WebAssembly/global-ctor-dtor.ll
index bc1be793134969..f1ec71da1ebb64 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

github-actions · 2024-04-26T12:59:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

`llvm.trap` will be convert as `unreachable` which is termiantor. But we cannot append any instruction after terminator wihch will casue llir's validation failed. This PR introduces a pass to clean instruction after terminator. Fixes: llvm#68770

sbc100 · 2024-04-26T16:22:53Z

I'm not sure it true that Instruction after terminator will cause validation failure. IIUC the WebAssembly validator has a special mode for instruction that follow unreachable, but its not necessarily a validation failure. @tlively can correct me if I'm wrong.

tlively · 2024-04-26T17:39:02Z

I think it's some LLVM validation that will fail, not WebAssembly validation.

sbc100 · 2024-04-26T17:55:18Z

I think it's some LLVM validation that will fail, not WebAssembly validation.

If that was the case why wouldn't this pass also be needed for other targets?

tlively · 2024-04-26T19:13:22Z

They probably lower @llvm.trap to something that is not considered a terminator. FWIW I'm not really sure why we consider unreachable to be a terminator. It seems like that could go either way.

sbc100 · 2024-04-26T19:17:59Z

They probably lower @llvm.trap to something that is not considered a terminator.

I guess you are right, but that seems odd.. wouldn't that be an archetypal thing to lower to a terminator?

dschuff · 2024-04-26T21:10:30Z

Yeah, it does seem like it could go either way. In fact there is also llvm.debugtrap which is not marked as noreturn, so we can't lower it to actual wasm unreachable, so we have a pseudoinstruction instead:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyInstrControl.td

Lines 113 to 120 in d9fd0dd

    
           // debugtrap explicitly returns despite trapping because it is supposed to just 
        
           // get the attention of the debugger. Unfortunately, because UNREACHABLE is a 
        
           // terminator, lowering debugtrap to UNREACHABLE can create an invalid 
        
           // MachineBasicBlock when there is additional code after it. Lower it to this 
        
           // non-terminator version instead. 
        
           // TODO: Actually execute the debugger statement when running on the Web 
        
           let isTrap = 1 in 
        
           defm DEBUG_UNREACHABLE : NRI<(outs), (ins), [(debugtrap)], "unreachable", 0x00>;

HerrCai0907 · 2024-04-26T22:14:46Z

This is what aarch64 llc do, it also drop the all instruction after llvm.trap in O2.

define void @foo(ptr %p) {
  %a = load i32, ptr %p
  call void @llvm.trap()
  %b = load i32, ptr %p
  ret void
}

foo:                                    // @foo
	.cfi_startproc
// %bb.0:
	brk	#0x1
	ret

HerrCai0907 · 2024-04-27T14:09:19Z

In AArch64, Trap and DebugTrap will be lowered as BRK, which is not terminator. But llc still remove the unreachable instruction during optimization. Maybe we can also do this?

def : Pat<(trap), (BRK 1)>;
def : Pat<(debugtrap), (BRK 0xF000)>;
let isTrap = 1 in {
def BRK   : ExceptionGeneration<0b001, 0b00, "brk",
                                [(int_aarch64_break timm32_0_65535:$imm)]>;
}

joker-eph · 2024-04-27T19:12:42Z

This broke the bot: https://lab.llvm.org/buildbot/#/builders/272

You should have got an email notification about it, didn't you?

This reverts commit ff03f23.

Reverts #90207 LLD Bots are broken.

…90354)" (#90366) `llvm.trap` will be convert as unreachable which is terminator. Instruction after terminator will cause validation failed. This PR introduces a pass to clean instruction after terminator. Fixes: #68770 Reapply: #90207

llvmbot added backend:WebAssembly mc Machine (object) code labels Apr 26, 2024

HerrCai0907 changed the title ~~[WASM] remove instruction after builtin trap~~ [WebAssembly] remove instruction after builtin trap Apr 26, 2024

HerrCai0907 requested review from tlively and sbc100 April 26, 2024 12:57

HerrCai0907 force-pushed the fix/stackify branch from 820e4a8 to 97b0ddc Compare April 26, 2024 14:55

tlively approved these changes Apr 26, 2024

View reviewed changes

HerrCai0907 merged commit ff03f23 into llvm:main Apr 27, 2024
3 of 4 checks passed

HerrCai0907 deleted the fix/stackify branch April 27, 2024 14:11

joker-eph added a commit that referenced this pull request Apr 27, 2024

Revert "[WebAssembly] remove instruction after builtin trap (#90207)"

b95e62b

This reverts commit ff03f23.

joker-eph mentioned this pull request Apr 27, 2024

Revert "[WebAssembly] remove instruction after builtin trap" #90354

Merged

joker-eph added a commit that referenced this pull request Apr 27, 2024

Revert "[WebAssembly] remove instruction after builtin trap" (#90354)

38a2051

Reverts #90207 LLD Bots are broken.

HerrCai0907 mentioned this pull request Apr 28, 2024

Revert "Revert "[WebAssembly] remove instruction after builtin trap" (#90354)" #90366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebAssembly] remove instruction after builtin trap #90207

[WebAssembly] remove instruction after builtin trap #90207

HerrCai0907 commented Apr 26, 2024 •

edited

Loading

llvmbot commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading

sbc100 commented Apr 26, 2024

tlively commented Apr 26, 2024

sbc100 commented Apr 26, 2024

tlively commented Apr 26, 2024

sbc100 commented Apr 26, 2024

dschuff commented Apr 26, 2024 •

edited

Loading

HerrCai0907 commented Apr 26, 2024 •

edited

Loading

HerrCai0907 commented Apr 27, 2024 •

edited

Loading

joker-eph commented Apr 27, 2024

[WebAssembly] remove instruction after builtin trap #90207

[WebAssembly] remove instruction after builtin trap #90207

Conversation

HerrCai0907 commented Apr 26, 2024 • edited Loading

llvmbot commented Apr 26, 2024 • edited Loading

github-actions bot commented Apr 26, 2024 • edited Loading

sbc100 commented Apr 26, 2024

tlively commented Apr 26, 2024

sbc100 commented Apr 26, 2024

tlively commented Apr 26, 2024

sbc100 commented Apr 26, 2024

dschuff commented Apr 26, 2024 • edited Loading

HerrCai0907 commented Apr 26, 2024 • edited Loading

HerrCai0907 commented Apr 27, 2024 • edited Loading

joker-eph commented Apr 27, 2024

HerrCai0907 commented Apr 26, 2024 •

edited

Loading

llvmbot commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading

dschuff commented Apr 26, 2024 •

edited

Loading

HerrCai0907 commented Apr 26, 2024 •

edited

Loading

HerrCai0907 commented Apr 27, 2024 •

edited

Loading