Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WebAssembly] Disable reference types in generic CPU #90792

Merged
merged 1 commit into from
May 1, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented May 1, 2024

#80923 newly enabled multivalue and reference-types in the generic CPU.
But enabling reference-types ended up breaking up Wasm's Chromium CI (https://chromium-review.googlesource.com/c/emscripten-releases/+/5500231) because the way the table index is encoded is different from MVP (u32) vs. reference-types (LEB), which caused different encodings for call_indirect.

And Chromium CI's and Emscripten's minimum required node version is v16, which does not yet support reference-types, which does not recognize that table index encoding. reference-types is first supported in node v17.2.

We knew the current minimum required node for Emscripten (v16) did not support reference-types, but thought it was fine because unless you explicitly use __funcref or __externref things would be fine, and if you want to use them explicitly, you would have a newer node. But it turned out it also affected the encoding of call_indirect.

While we are discussing the potential solutions, I will disable reference-types to unblock the rolls.

 llvm#80923 newly enabled multivalue and reference-types in the generic CPU.
But enabling reference-types ended up breaking up Wasm's Chromium CI
(https://chromium-review.googlesource.com/c/emscripten-releases/+/5500231)
because the way the table index is encoded different from MVP (u32) vs.
reference-types (LEB), which caused different encodings for
`call_indirect`.

And Chromium CI's and Emscripten's minimum required node version is v16,
which does not yet support reference-types, which does not recognize
that table index encoding. reference-types is first supported in node
v17.2.

We knew the current minimum required node for Emscripten (v16) did not
support reference-types, but thought it was fine because unless you
explicitly use `__funcref` or `__externref` things would be fine, and if
you want to use them explicitly, you would have a newer node. But it
turned out it also affected the encoding of `call_indirect`.

While we are discussing the potential solutions, I will disable
reference-types to unblock the rolls.
@aheejin aheejin requested review from sbc100 and dschuff May 1, 2024 22:52
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:WebAssembly clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 1, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-clang

Author: Heejin Ahn (aheejin)

Changes

#80923 newly enabled multivalue and reference-types in the generic CPU.
But enabling reference-types ended up breaking up Wasm's Chromium CI (https://chromium-review.googlesource.com/c/emscripten-releases/+/5500231) because the way the table index is encoded different from MVP (u32) vs. reference-types (LEB), which caused different encodings for call_indirect.

And Chromium CI's and Emscripten's minimum required node version is v16, which does not yet support reference-types, which does not recognize that table index encoding. reference-types is first supported in node v17.2.

We knew the current minimum required node for Emscripten (v16) did not support reference-types, but thought it was fine because unless you explicitly use __funcref or __externref things would be fine, and if you want to use them explicitly, you would have a newer node. But it turned out it also affected the encoding of call_indirect.

While we are discussing the potential solutions, I will disable reference-types to unblock the rolls.


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+4-4)
  • (modified) clang/lib/Basic/Targets/WebAssembly.cpp (+1-1)
  • (modified) clang/test/Preprocessor/wasm-target-features.c (+1-1)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2c5308fbcb319a..0e3f7cf89ca885 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -742,10 +742,10 @@ AIX Support
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^
 
-The -mcpu=generic configuration now enables multivalue and reference-types.These
-proposals are standardized and available in all major engines. Enabling
-multivalue here only enables the language feature but does not turn on the
-multivalue ABI (this enables non-ABI uses of multivalue, like exnref).
+The -mcpu=generic configuration now enables multivalue feature, which is
+standardized and available in all major engines. Enabling multivalue here only
+enables the language feature but does not turn on the multivalue ABI (this
+enables non-ABI uses of multivalue, like exnref).
 
 AVR Support
 ^^^^^^^^^^^
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index 1f0418b21c1f86..a6d820e108088f 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -153,7 +153,6 @@ bool WebAssemblyTargetInfo::initFeatureMap(
   auto addGenericFeatures = [&]() {
     Features["multivalue"] = true;
     Features["mutable-globals"] = true;
-    Features["reference-types"] = true;
     Features["sign-ext"] = true;
   };
   auto addBleedingEdgeFeatures = [&]() {
@@ -162,6 +161,7 @@ bool WebAssemblyTargetInfo::initFeatureMap(
     Features["bulk-memory"] = true;
     Features["multimemory"] = true;
     Features["nontrapping-fptoint"] = true;
+    Features["reference-types"] = true;
     Features["tail-call"] = true;
     Features["half-precision"] = true;
     setSIMDLevel(Features, SIMD128, true);
diff --git a/clang/test/Preprocessor/wasm-target-features.c b/clang/test/Preprocessor/wasm-target-features.c
index 72ecc60a6e7898..5a4f85461d5aa2 100644
--- a/clang/test/Preprocessor/wasm-target-features.c
+++ b/clang/test/Preprocessor/wasm-target-features.c
@@ -164,7 +164,6 @@
 //
 // GENERIC-INCLUDE-DAG: #define __wasm_multivalue__ 1{{$}}
 // GENERIC-INCLUDE-DAG: #define __wasm_mutable_globals__ 1{{$}}
-// GENERIC-INCLUDE-DAG: #define __wasm_reference_types__ 1{{$}}
 // GENERIC-INCLUDE-DAG: #define __wasm_sign_ext__ 1{{$}}
 //
 // RUN: %clang -E -dM %s -o - 2>&1 \
@@ -181,6 +180,7 @@
 // GENERIC-NOT: #define __wasm_half_precision__ 1{{$}}
 // GENERIC-NOT: #define __wasm_multimemory__ 1{{$}}
 // GENERIC-NOT: #define __wasm_nontrapping_fptoint__ 1{{$}}
+// GENERIC-NOT: #define __wasm_reference_types__ 1{{$}}
 // GENERIC-NOT: #define __wasm_relaxed_simd__ 1{{$}}
 // GENERIC-NOT: #define __wasm_simd128__ 1{{$}}
 // GENERIC-NOT: #define __wasm_tail_call__ 1{{$}}

@aheejin
Copy link
Member Author

aheejin commented May 1, 2024

Will land this given that this is necessary to unblock the rolls and the full CI will take more than a full day.

@aheejin aheejin merged commit 8c64a30 into llvm:main May 1, 2024
8 of 9 checks passed
@aheejin aheejin deleted the disable_reftype branch May 1, 2024 23:51
aheejin added a commit to aheejin/emscripten that referenced this pull request May 8, 2024
I submitted emscripten-core#21853 before landing
llvm/llvm-project#80923, so the previous version
(3.1.59) was released before the LLVM change actually landed. And then
we decided to disable reference-types temporarily
(llvm/llvm-project#90792) while the node version
is being resolved
(emscripten-core/emsdk#1173 (comment)).

This moves the entry to the current release and only mentions multivalue
is the newly enabled feature.
aheejin added a commit to emscripten-core/emscripten that referenced this pull request May 8, 2024
I submitted #21853 before landing
llvm/llvm-project#80923, so the previous version
(3.1.59) was released before the LLVM change actually landed. And then
we decided to disable reference-types temporarily
(llvm/llvm-project#90792) while the node version
is being resolved
(emscripten-core/emsdk#1173 (comment)).

This moves the entry to the current release and only mentions multivalue
is the newly enabled feature.
aheejin added a commit to aheejin/llvm-project that referenced this pull request May 24, 2024
Now that we are about to upgrade emsdk's default node to v18.20.3
(emscripten-core/emsdk#1387), we can re-enable
reference-types by default again. This effectively reverts llvm#90792.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:WebAssembly clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants