[WebAssembly] Enable multivalue and reference-types in generic CPU config #80923

aheejin · 2024-02-07T02:05:13Z

This enables multivalue and reference-types in -mcpu=generic configuration. These proposals have been standardized and supported in all major browsers for several years at this point: https://github.com/WebAssembly/proposals/blob/main/finished-proposals.md

This enables nontrapping-fptoint, multivlaue, reference-types, and bulk-memory in `-mcpu=generic` configuration. These proposals have been standardized and supported in all major browsers for several years at this point: https://github.com/WebAssembly/proposals/blob/main/finished-proposals.md

llvmbot · 2024-02-07T02:05:44Z

@llvm/pr-subscribers-clang

Author: Heejin Ahn (aheejin)

Changes

This enables nontrapping-fptoint, multivlaue, reference-types, and bulk-memory in -mcpu=generic configuration. These proposals have been standardized and supported in all major browsers for several years at this point: https://github.com/WebAssembly/proposals/blob/main/finished-proposals.md

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+4)
(modified) clang/lib/Basic/Targets/WebAssembly.cpp (+12-6)
(modified) clang/test/Preprocessor/wasm-target-features.c (+4-4)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 802c44b6c86080..5a07dcca106876 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -259,6 +259,10 @@ AIX Support
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^
 
+The -mcpu=generic configuration now enables nontrapping-fptoint, multivalue,
+reference-types, and bulk-memory.These proposals are standardized and available
+in all major engines.
+
 AVR Support
 ^^^^^^^^^^^
 
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index f1c925d90cb649..38fe4013090f40 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -147,19 +147,25 @@ void WebAssemblyTargetInfo::setFeatureEnabled(llvm::StringMap<bool> &Features,
 bool WebAssemblyTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
-  if (CPU == "bleeding-edge") {
-    Features["nontrapping-fptoint"] = true;
+  auto addGenericFeatures = [&]() {
     Features["sign-ext"] = true;
+    Features["mutable-globals"] = true;
+    Features["nontrapping-fptoint"] = true;
     Features["bulk-memory"] = true;
+    Features["reference-types"] = true;
+    Features["multivalue"] = true;
+  };
+  auto addBleedingEdgeFeatures = [&]() {
     Features["atomics"] = true;
-    Features["mutable-globals"] = true;
     Features["tail-call"] = true;
-    Features["reference-types"] = true;
     Features["multimemory"] = true;
     setSIMDLevel(Features, SIMD128, true);
+  };
+  if (CPU == "bleeding-edge") {
+    addGenericFeatures();
+    addBleedingEdgeFeatures();
   } else if (CPU == "generic") {
-    Features["sign-ext"] = true;
-    Features["mutable-globals"] = true;
+    addGenericFeatures();
   }
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
diff --git a/clang/test/Preprocessor/wasm-target-features.c b/clang/test/Preprocessor/wasm-target-features.c
index eccd432aa8eee6..5834e6d183bc9c 100644
--- a/clang/test/Preprocessor/wasm-target-features.c
+++ b/clang/test/Preprocessor/wasm-target-features.c
@@ -155,15 +155,15 @@
 //
 // GENERIC-DAG:#define __wasm_sign_ext__ 1{{$}}
 // GENERIC-DAG:#define __wasm_mutable_globals__ 1{{$}}
-// GENERIC-NOT:#define __wasm_nontrapping_fptoint__ 1{{$}}
-// GENERIC-NOT:#define __wasm_bulk_memory__ 1{{$}}
+// GENERIC-DAG:#define __wasm_nontrapping_fptoint__ 1{{$}}
+// GENERIC-DAG:#define __wasm_bulk_memory__ 1{{$}}
+// GENERIC-DAG:#define __wasm_multivalue__ 1{{$}}
+// GENERIC-DAG:#define __wasm_reference_types__ 1{{$}}
 // GENERIC-NOT:#define __wasm_simd128__ 1{{$}}
 // GENERIC-NOT:#define __wasm_atomics__ 1{{$}}
 // GENERIC-NOT:#define __wasm_tail_call__ 1{{$}}
 // GENERIC-NOT:#define __wasm_multimemory__ 1{{$}}
 // GENERIC-NOT:#define __wasm_exception_handling__ 1{{$}}
-// GENERIC-NOT:#define __wasm_multivalue__ 1{{$}}
-// GENERIC-NOT:#define __wasm_reference_types__ 1{{$}}
 // GENERIC-NOT:#define __wasm_extended_const__ 1{{$}}
 
 // RUN: %clang -E -dM %s -o - 2>&1 \

llvmbot · 2024-02-07T02:05:44Z

@llvm/pr-subscribers-backend-webassembly

Author: Heejin Ahn (aheejin)

Changes

This enables nontrapping-fptoint, multivlaue, reference-types, and bulk-memory in -mcpu=generic configuration. These proposals have been standardized and supported in all major browsers for several years at this point: https://github.com/WebAssembly/proposals/blob/main/finished-proposals.md

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+4)
(modified) clang/lib/Basic/Targets/WebAssembly.cpp (+12-6)
(modified) clang/test/Preprocessor/wasm-target-features.c (+4-4)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 802c44b6c86080..5a07dcca106876 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -259,6 +259,10 @@ AIX Support
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^
 
+The -mcpu=generic configuration now enables nontrapping-fptoint, multivalue,
+reference-types, and bulk-memory.These proposals are standardized and available
+in all major engines.
+
 AVR Support
 ^^^^^^^^^^^
 
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index f1c925d90cb649..38fe4013090f40 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -147,19 +147,25 @@ void WebAssemblyTargetInfo::setFeatureEnabled(llvm::StringMap<bool> &Features,
 bool WebAssemblyTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
-  if (CPU == "bleeding-edge") {
-    Features["nontrapping-fptoint"] = true;
+  auto addGenericFeatures = [&]() {
     Features["sign-ext"] = true;
+    Features["mutable-globals"] = true;
+    Features["nontrapping-fptoint"] = true;
     Features["bulk-memory"] = true;
+    Features["reference-types"] = true;
+    Features["multivalue"] = true;
+  };
+  auto addBleedingEdgeFeatures = [&]() {
     Features["atomics"] = true;
-    Features["mutable-globals"] = true;
     Features["tail-call"] = true;
-    Features["reference-types"] = true;
     Features["multimemory"] = true;
     setSIMDLevel(Features, SIMD128, true);
+  };
+  if (CPU == "bleeding-edge") {
+    addGenericFeatures();
+    addBleedingEdgeFeatures();
   } else if (CPU == "generic") {
-    Features["sign-ext"] = true;
-    Features["mutable-globals"] = true;
+    addGenericFeatures();
   }
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
diff --git a/clang/test/Preprocessor/wasm-target-features.c b/clang/test/Preprocessor/wasm-target-features.c
index eccd432aa8eee6..5834e6d183bc9c 100644
--- a/clang/test/Preprocessor/wasm-target-features.c
+++ b/clang/test/Preprocessor/wasm-target-features.c
@@ -155,15 +155,15 @@
 //
 // GENERIC-DAG:#define __wasm_sign_ext__ 1{{$}}
 // GENERIC-DAG:#define __wasm_mutable_globals__ 1{{$}}
-// GENERIC-NOT:#define __wasm_nontrapping_fptoint__ 1{{$}}
-// GENERIC-NOT:#define __wasm_bulk_memory__ 1{{$}}
+// GENERIC-DAG:#define __wasm_nontrapping_fptoint__ 1{{$}}
+// GENERIC-DAG:#define __wasm_bulk_memory__ 1{{$}}
+// GENERIC-DAG:#define __wasm_multivalue__ 1{{$}}
+// GENERIC-DAG:#define __wasm_reference_types__ 1{{$}}
 // GENERIC-NOT:#define __wasm_simd128__ 1{{$}}
 // GENERIC-NOT:#define __wasm_atomics__ 1{{$}}
 // GENERIC-NOT:#define __wasm_tail_call__ 1{{$}}
 // GENERIC-NOT:#define __wasm_multimemory__ 1{{$}}
 // GENERIC-NOT:#define __wasm_exception_handling__ 1{{$}}
-// GENERIC-NOT:#define __wasm_multivalue__ 1{{$}}
-// GENERIC-NOT:#define __wasm_reference_types__ 1{{$}}
 // GENERIC-NOT:#define __wasm_extended_const__ 1{{$}}
 
 // RUN: %clang -E -dM %s -o - 2>&1 \

sbc100 · 2024-02-07T02:31:29Z

@kripken

aheejin · 2024-02-07T03:28:56Z

Tried running Emscripten tests, and some of them fail with reference-types or bulk-memory. I haven't investigated the causes yet, but it could be better to start with the other two (nontrapping-fptoint and multivalue).

sbc100 · 2024-02-07T03:36:17Z

Tried running Emscripten tests, and some of them fail with reference-types or bulk-memory. I haven't investigated the causes yet, but it could be better to start with the other two (nontrapping-fptoint and multivalue).

No need to block these on emscripten failures.. I'm sure we can find and fix those. We should make this decision based on current browser stats and release dates I think,

sunfishcode · 2024-02-07T20:48:09Z

clang/docs/ReleaseNotes.rst

@@ -259,6 +259,10 @@ AIX Support
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^

+The -mcpu=generic configuration now enables nontrapping-fptoint, multivalue,
+reference-types, and bulk-memory.These proposals are standardized and available
+in all major engines.


Could you also mention that enabling multivalue here just means enabling the language feauture, and not changing the ABI?

We plan to enable multivalue in the features section soon (llvm#80923) for other reasons, such as the feature having been standardized for many years and other features being developed (e.g. EH) depending on it. This is separate from enabling Clang experimental multivalue ABI (`-Xclang -target-abi -Xclang experimental-mv`), but it turned out we generate some multivalue code in the backend as well if it is enabled in the features section. Given that our backend multivalue generation still has not been much used nor tested, and enabling the feature in the features section can be a separate decision from how much multialue (including none) we decide to generate for now, I'd like to temporarily disable the actual generation of multivalue in our backend. To do that, this adds an internal flag `-wasm-emit-multivalue` that defaults to false. All our existing multivalue tests can use this to test multivalue code. This flag can be removed later when we are confident the multivalue generation is well tested.

We plan to enable multivalue in the features section soon (#80923) for other reasons, such as the feature having been standardized for many years and other features being developed (e.g. EH) depending on it. This is separate from enabling Clang experimental multivalue ABI (`-Xclang -target-abi -Xclang experimental-mv`), but it turned out we generate some multivalue code in the backend as well if it is enabled in the features section. Given that our backend multivalue generation still has not been much used nor tested, and enabling the feature in the features section can be a separate decision from how much multialue (including none) we decide to generate for now, I'd like to temporarily disable the actual generation of multivalue in our backend. To do that, this adds an internal flag `-wasm-emit-multivalue` that defaults to false. All our existing multivalue tests can use this to test multivalue code. This flag can be removed later when we are confident the multivalue generation is well tested.

aheejin · 2024-04-23T11:02:53Z

This first tried to enable four features (reference-types, multivalue, bulk-memory, and nontrapping-fptoint), but I think now we can enable the two first: reference-types and multivalue. These two were actually the first motivation I started this (these are necessary for the new EH spec adopted in Oct 2023). I believe I ran all emscripten tests with these two enabled and I don't think more tests fail than the current status quo. (We have some tests failing even now in the modes that are not regularly checked by CI, but enabling these two doesn't change that.)

Usually, when we enable new features, we provide corresponding lowering passes in Binaryen to support legacy users. But I don't think that's necessary or even possible for these two. You can lower neither all multivalue-returning functions away (especially when they are imported or exported), nor reference types away. But I think it's fine, given that these two features are not really generated unless you opt into it, for example, by explicitly using __funcref or __externref. And after #88492, multivalue code is not gonna be generated unless you explicitly use a multivalue ABI like -Xclang -target-abi -Xclang experimental-mv or use the new EH instructions (try_table vaiants) in LLVM, which have not been implemented yet. So even if we enable these two features, the legacy users wouldn't be suddenly getting reference types or multivalue in their code.

I'll modify this PR to enable the two features (multivalue and reference-types) first. That way I think we can land this soon. WDYT?

aheejin · 2024-04-23T13:32:05Z

cc @kripken too (You didn't show up in the reviewers list, so I couldn't add you)

kripken

Enabling multivalue and reference-types first sgtm.

clang/docs/ReleaseNotes.rst

sbc100 · 2024-04-23T15:59:18Z

After this lands does that mean that all object files will have multi-value in the features section? Or only object files that actually use mutli-value?

dschuff · 2024-04-23T17:28:17Z

I think "generic" is the default CPU so object files will have it enabled by default. You can still specify "mvp" as the CPU as before to avoid it.

tlively · 2024-04-23T18:59:44Z

clang/lib/Basic/Targets/WebAssembly.cpp

+  };
+  auto addBleedingEdgeFeatures = [&]() {
+    addGenericFeatures();
+    Features["nontrapping-fptoint"] = true;


Looks like this was enabled in Safari in September 2021. Should we enable this by default as well?

Oh, I see, if we think we would need a Binaryen lowering pass first, then proceeding without this SGTM.

I'm a bit sad not to see this added, since this is the one that can actually improve codegen. If we know we want to enable to this, is there any downside to doing two different rounds (i.e. are there advantages to waiting on this until we have the lowering pass in place?)

MV and reftypes are required to use exnref, that's the thing that's on the critical path here. So yes, we definitely want to land these two without blocking on nontrapping-fptoint.

Relatedly (to nontrapping-fptoint, but unrelatedly to this PR), I prototyped a Binaryen pass to lower away LLVM's use of nontrapping-fptoint* which is what would allow us to improve codegen. I think it works, but I discovered that some of emscripten's tests fail when we enable nontrapping fptoint, and I haven't yet looked at why that's the case (either a bug in emscripten's tests, or a bug in V8's implementation of nontrapping fptoint). We'd need to also fix that to unblock enabling this.

** (This isn't exactly the same as a full lowering of nontrapping-fptoint as spec'ed, because it takes advantage of knowledge of the semantics of LLVM's fptosi intrinsics)

dschuff

(also LGTM FTR)

Co-authored-by: Alon Zakai <alonzakai@gmail.com>

llvm/llvm-project#80923 enabled multivalue and reference-types by default. Even though this is an LLVM change, given that we release emsdk with all LLVM/Binaryen/Emscripten versions, it could be worth mentioning in the ChangeLog.

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.

#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.

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.

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.

llvmbot added clang Clang issues not falling into any other category backend:WebAssembly clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 7, 2024

aheejin mentioned this pull request Feb 7, 2024

Enabling features by default in LLVM WebAssembly/tool-conventions#158

Closed

Merge branch 'main' into enable_features

e501363

sunfishcode reviewed Feb 7, 2024

View reviewed changes

This was referenced Feb 13, 2024

[WebAssembly] Demote PHIs in catchswitch BB only #81570

Merged

[WebAssembly] Problems of forcing mem2reg in reference-types #81575

Closed

aheejin mentioned this pull request Feb 23, 2024

[WebAssembly] Disable multivalue emission temporarily #82714

Merged

aheejin added 3 commits February 23, 2024 00:31

Mention that ABI is not turned on

01ac8bd

Merge branch 'main' into enable_features

7119137

Merge branch 'main' into enable_features

17e81e3

aheejin added 2 commits April 23, 2024 11:43

Merge branch 'main' into enable_features

26caed5

Enable only multivalue and reference types

de6aa6a

aheejin changed the title ~~[WebAssembly] Add more features to generic CPU config~~ [WebAssembly] Enable multivalue and reference-types in generic CPU config Apr 23, 2024

aheejin requested review from sbc100, dschuff and tlively April 23, 2024 13:30

Enable multivalue and reference-types in bleeding-edge

91df749

kripken reviewed Apr 23, 2024

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

tlively approved these changes Apr 23, 2024

View reviewed changes

dschuff approved these changes Apr 23, 2024

View reviewed changes

aheejin and others added 2 commits April 24, 2024 14:35

Update clang/docs/ReleaseNotes.rst

f4aee60

Co-authored-by: Alon Zakai <alonzakai@gmail.com>

Merge branch 'main' into enable_features

af79d4e

aheejin merged commit 5bbf1ea into llvm:main Apr 29, 2024
5 checks passed

aheejin deleted the enable_features branch April 29, 2024 21:23

aheejin mentioned this pull request Apr 29, 2024

Note newly enabled features in LLVM in ChangeLog emscripten-core/emscripten#21853

Merged

kleisauke mentioned this pull request Apr 30, 2024

Ensure compatibility with older versions of Node.js emscripten-core/emscripten#21863

Merged

aheejin mentioned this pull request May 1, 2024

[WebAssembly] Disable reference types in generic CPU #90792

Merged

aheejin mentioned this pull request May 8, 2024

Fix ChangeLog related to new default LLVM features emscripten-core/emscripten#21912

Merged

bjorn3 mentioned this pull request Jun 25, 2024

Align with the "C" ABI rustwasm/wasm-bindgen#3454

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebAssembly] Enable multivalue and reference-types in generic CPU config #80923

[WebAssembly] Enable multivalue and reference-types in generic CPU config #80923

aheejin commented Feb 7, 2024 •

edited

Loading

llvmbot commented Feb 7, 2024

llvmbot commented Feb 7, 2024

sbc100 commented Feb 7, 2024

aheejin commented Feb 7, 2024 •

edited

Loading

sbc100 commented Feb 7, 2024

sunfishcode Feb 7, 2024

aheejin Feb 23, 2024

aheejin commented Apr 23, 2024 •

edited

Loading

aheejin commented Apr 23, 2024 •

edited

Loading

kripken left a comment

sbc100 commented Apr 23, 2024

dschuff commented Apr 23, 2024

tlively Apr 23, 2024

tlively Apr 23, 2024

sbc100 Apr 23, 2024

dschuff Apr 23, 2024 •

edited

Loading

dschuff left a comment

[WebAssembly] Enable multivalue and reference-types in generic CPU config #80923

[WebAssembly] Enable multivalue and reference-types in generic CPU config #80923

Conversation

aheejin commented Feb 7, 2024 • edited Loading

llvmbot commented Feb 7, 2024

llvmbot commented Feb 7, 2024

sbc100 commented Feb 7, 2024

aheejin commented Feb 7, 2024 • edited Loading

sbc100 commented Feb 7, 2024

sunfishcode Feb 7, 2024

Choose a reason for hiding this comment

aheejin Feb 23, 2024

Choose a reason for hiding this comment

aheejin commented Apr 23, 2024 • edited Loading

aheejin commented Apr 23, 2024 • edited Loading

kripken left a comment

Choose a reason for hiding this comment

sbc100 commented Apr 23, 2024

dschuff commented Apr 23, 2024

tlively Apr 23, 2024

Choose a reason for hiding this comment

tlively Apr 23, 2024

Choose a reason for hiding this comment

sbc100 Apr 23, 2024

Choose a reason for hiding this comment

dschuff Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

dschuff left a comment

Choose a reason for hiding this comment

aheejin commented Feb 7, 2024 •

edited

Loading

aheejin commented Feb 7, 2024 •

edited

Loading

aheejin commented Apr 23, 2024 •

edited

Loading

aheejin commented Apr 23, 2024 •

edited

Loading

dschuff Apr 23, 2024 •

edited

Loading