Skip to content

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Jul 22, 2025

PR #147486 broke the sanitizer and expensive-checks buildbot.

These captures were needed when toWasmValType emitted a diagnostic but are no longer needed since we changed it to an assertion failure. This removes the unneeded captures and should fix the sanitizer-buildbot.

I also fixed the codegen in the wasm64 target: table.get requires an i32 but in wasm64 the function pointer is an i64. We need an additional i32.wrap_i64 to convert it. I also added -verify-machineinstrs to the tests so that the test suite validates this fix.

Finally, I noticed that #150201 uses a feature of the intrinsic that is not covered by the tests, namely ptr arguments. So I added one additional test case to ensure that it works properly.

cc @dschuff

@llvmbot
Copy link
Member

llvmbot commented Jul 22, 2025

@llvm/pr-subscribers-backend-webassembly

Author: Hood Chatham (hoodmane)

Changes

PR #147486 broke the sanitizer buildbot. These captures were needed when toWasmValType emitted a diagnostic but are no longer needed since we changed it to an assertion failure.
This removes the unneeded captures and should fix the sanitizer-buildbot.

cc @dschuff


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

1 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp (+1-1)
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
index a7991319be8c7..dfc5a6d78756d 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
@@ -123,7 +123,7 @@ static SDValue getTagSymNode(int Tag, SelectionDAG *DAG) {
 static APInt encodeFunctionSignature(SelectionDAG *DAG, SDLoc &DL,
                                      SmallVector<MVT, 4> &Returns,
                                      SmallVector<MVT, 4> &Params) {
-  auto toWasmValType = [&DAG, &DL](MVT VT) {
+  auto toWasmValType = [](MVT VT) {
     if (VT == MVT::i32) {
       return wasm::ValType::I32;
     }

PR llvm#147486 broke the sanitizer buildbot. These captures were needed
when toWasmValType emitted a diagnostic but are no longer needed
since we changed it to an assertion failure.
This removes the unneeded captures and should fix the sanitizer-buildbot.
@hoodmane hoodmane force-pushed the toWasmValType-no-captures branch from 0e1a04f to 08aa70a Compare July 22, 2025 22:10
@hoodmane hoodmane changed the title Remove unnecessary captures in toWasmValType [WebAssembly,llvm] Remove unnecessary captures in toWasmValType Jul 22, 2025
@dschuff
Copy link
Member

dschuff commented Jul 22, 2025

I'll wait for the tests to run and then merge.

@hoodmane hoodmane changed the title [WebAssembly,llvm] Remove unnecessary captures in toWasmValType [WebAssembly,llvm] Fix buildbot problems with llvm.wasm.ref.test.func Jul 23, 2025
@hoodmane hoodmane requested a review from dschuff July 23, 2025 15:25
@dschuff dschuff merged commit e3b79af into llvm:main Jul 23, 2025
9 checks passed
@hoodmane hoodmane deleted the toWasmValType-no-captures branch July 23, 2025 17:34
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025
…llvm#150116)

PR llvm#147486 broke the sanitizer and expensive-checks buildbot. 

These captures were needed when toWasmValType emitted a diagnostic but
are no longer needed since we changed it to an assertion failure. This
removes the unneeded captures and should fix the sanitizer-buildbot.

I also fixed the codegen in the wasm64 target: table.get requires an i32
but in wasm64 the function pointer is an i64. We need an additional
`i32.wrap_i64` to convert it. I also added `-verify-machineinstrs` to
the tests so that the test suite validates this fix.

Finally, I noticed that llvm#150201 uses a feature of the intrinsic that is
not covered by the tests, namely `ptr` arguments. So I added one
additional test case to ensure that it works properly.

cc @dschuff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants