[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049

sunfishcode · 2024-10-11T21:28:10Z

We were prepared to enable these features back in February, but they got pulled for what appear to be unrelated reasons. So let's have another try at enabling them!

Another motivation here is that it'd be convenient for the Trail1 proposal if "trail1" is a ~~superset~~subset of "generic". Edit: except for extended-const though.

llvmbot · 2024-10-11T21:28:45Z

@llvm/pr-subscribers-lld-wasm
@llvm/pr-subscribers-lld
@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-webassembly

Author: Dan Gohman (sunfishcode)

Changes

We were prepared to enable these features back in February, but they got pulled for what appear to be unrelated reasons. So let's have another try at enabling them!

Another motivation here is that it'd be convenient for the Trail1 proposal if "trail1" is a superset of "generic".

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+9)
(modified) clang/lib/Basic/Targets/WebAssembly.cpp (+2)
(modified) llvm/docs/ReleaseNotes.md (+9)
(modified) llvm/lib/Target/WebAssembly/WebAssembly.td (+2-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e74dd1a5fb32da..aec48e32dc85a8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -627,6 +627,15 @@ NetBSD Support
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^
 
+The default target CPU, "generic", now enables the `-mnontrapping-fptoint`
+and `-mbulk-memory` flags, which correspond to the [Bulk Memory Operations]
+and [Non-trapping float-to-int Conversions] language features, which are
+[widely implemented in engines].
+
+[Bulk Memory Operations]: https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md
+[Non-trapping float-to-int Conversions]: https://github.com/WebAssembly/spec/blob/master/proposals/nontrapping-float-to-int-conversion/Overview.md
+[widely implemented in engines]: https://webassembly.org/features/
+
 AVR Support
 ^^^^^^^^^^^
 
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index 5ac9421663adea..1ac8f14f8cb175 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -151,8 +151,10 @@ bool WebAssemblyTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
   auto addGenericFeatures = [&]() {
+    Features["bulk-memory"] = true;
     Features["multivalue"] = true;
     Features["mutable-globals"] = true;
+    Features["nontrapping-fptoint"] = true;
     Features["reference-types"] = true;
     Features["sign-ext"] = true;
   };
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index dcdd7a25c7fbee..1d4197f25f4422 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -164,6 +164,15 @@ Changes to the RISC-V Backend
 Changes to the WebAssembly Backend
 ----------------------------------
 
+The default target CPU, "generic", now enables the `-mnontrapping-fptoint`
+and `-mbulk-memory` flags, which correspond to the [Bulk Memory Operations]
+and [Non-trapping float-to-int Conversions] language features, which are
+[widely implemented in engines].
+
+[Bulk Memory Operations]: https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md
+[Non-trapping float-to-int Conversions]: https://github.com/WebAssembly/spec/blob/master/proposals/nontrapping-float-to-int-conversion/Overview.md
+[widely implemented in engines]: https://webassembly.org/features/
+
 Changes to the Windows Target
 -----------------------------
 
diff --git a/llvm/lib/Target/WebAssembly/WebAssembly.td b/llvm/lib/Target/WebAssembly/WebAssembly.td
index c632d4a74355d8..1e22707db23e91 100644
--- a/llvm/lib/Target/WebAssembly/WebAssembly.td
+++ b/llvm/lib/Target/WebAssembly/WebAssembly.td
@@ -110,7 +110,8 @@ def : ProcessorModel<"mvp", NoSchedModel, []>;
 // consideration given to available support in relevant engines and tools, and
 // the importance of the features.
 def : ProcessorModel<"generic", NoSchedModel,
-                      [FeatureMultivalue, FeatureMutableGlobals,
+                      [FeatureBulkMemory, FeatureMultivalue,
+                       FeatureMutableGlobals, FeatureNontrappingFPToInt,
                        FeatureReferenceTypes, FeatureSignExt]>;
 
 // Latest and greatest experimental version of WebAssembly. Bugs included!

llvmbot · 2024-10-11T21:28:46Z

@llvm/pr-subscribers-clang

Author: Dan Gohman (sunfishcode)

Changes

We were prepared to enable these features back in February, but they got pulled for what appear to be unrelated reasons. So let's have another try at enabling them!

Another motivation here is that it'd be convenient for the Trail1 proposal if "trail1" is a superset of "generic".

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+9)
(modified) clang/lib/Basic/Targets/WebAssembly.cpp (+2)
(modified) llvm/docs/ReleaseNotes.md (+9)
(modified) llvm/lib/Target/WebAssembly/WebAssembly.td (+2-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e74dd1a5fb32da..aec48e32dc85a8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -627,6 +627,15 @@ NetBSD Support
 WebAssembly Support
 ^^^^^^^^^^^^^^^^^^^
 
+The default target CPU, "generic", now enables the `-mnontrapping-fptoint`
+and `-mbulk-memory` flags, which correspond to the [Bulk Memory Operations]
+and [Non-trapping float-to-int Conversions] language features, which are
+[widely implemented in engines].
+
+[Bulk Memory Operations]: https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md
+[Non-trapping float-to-int Conversions]: https://github.com/WebAssembly/spec/blob/master/proposals/nontrapping-float-to-int-conversion/Overview.md
+[widely implemented in engines]: https://webassembly.org/features/
+
 AVR Support
 ^^^^^^^^^^^
 
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index 5ac9421663adea..1ac8f14f8cb175 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -151,8 +151,10 @@ bool WebAssemblyTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
   auto addGenericFeatures = [&]() {
+    Features["bulk-memory"] = true;
     Features["multivalue"] = true;
     Features["mutable-globals"] = true;
+    Features["nontrapping-fptoint"] = true;
     Features["reference-types"] = true;
     Features["sign-ext"] = true;
   };
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index dcdd7a25c7fbee..1d4197f25f4422 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -164,6 +164,15 @@ Changes to the RISC-V Backend
 Changes to the WebAssembly Backend
 ----------------------------------
 
+The default target CPU, "generic", now enables the `-mnontrapping-fptoint`
+and `-mbulk-memory` flags, which correspond to the [Bulk Memory Operations]
+and [Non-trapping float-to-int Conversions] language features, which are
+[widely implemented in engines].
+
+[Bulk Memory Operations]: https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md
+[Non-trapping float-to-int Conversions]: https://github.com/WebAssembly/spec/blob/master/proposals/nontrapping-float-to-int-conversion/Overview.md
+[widely implemented in engines]: https://webassembly.org/features/
+
 Changes to the Windows Target
 -----------------------------
 
diff --git a/llvm/lib/Target/WebAssembly/WebAssembly.td b/llvm/lib/Target/WebAssembly/WebAssembly.td
index c632d4a74355d8..1e22707db23e91 100644
--- a/llvm/lib/Target/WebAssembly/WebAssembly.td
+++ b/llvm/lib/Target/WebAssembly/WebAssembly.td
@@ -110,7 +110,8 @@ def : ProcessorModel<"mvp", NoSchedModel, []>;
 // consideration given to available support in relevant engines and tools, and
 // the importance of the features.
 def : ProcessorModel<"generic", NoSchedModel,
-                      [FeatureMultivalue, FeatureMutableGlobals,
+                      [FeatureBulkMemory, FeatureMultivalue,
+                       FeatureMutableGlobals, FeatureNontrappingFPToInt,
                        FeatureReferenceTypes, FeatureSignExt]>;
 
 // Latest and greatest experimental version of WebAssembly. Bugs included!

lld/test/wasm/lto/libcall-archive.ll

clang/lib/Basic/Targets/WebAssembly.cpp

clang/test/Preprocessor/wasm-target-features.c

llvm/test/CodeGen/WebAssembly/cfg-stackify-eh-legacy.ll

aheejin · 2024-10-16T00:08:28Z

lld/test/wasm/custom-section-name.ll

@@ -1,4 +1,4 @@
-; RUN: llc -filetype=obj %s -o %t.o
+; RUN: llc -filetype=obj -mattr=-bulk-memory %s -o %t.o


Why do we need to disable bulk memory here and in many other files?

In lld/test/wasm/custom-section-name.ll, it's because bulk-memory makes the target_features section bigger and thus changes some of the section offsets and it was easier to just disable bulk-memory than to figure out the new section offsets. I can update the section offsets if you prefer though. See my comment below.

In lld/test/wasm/data-segments.ll, it's because the test is written to test both bulk-memory and non-bulk-memory, and to keep it doing that, it's now necessary to explicitly disable bulk-memory for the non-bulk-memory case.

In lld/test/wasm/lto/stub-library-libcall.s it's because the test is testing the use of a memcpy stub function, and with bulk-memory it gets inlined.

llvm/test/MC/WebAssembly/extern-functype-intrinsic.ll is similarly testing memset calls.

llvm/test/MC/WebAssembly/libcall.ll is similarly testing memcpy calls.

Thanks for the explanation!

I've now investigated custom-section-name.ll more.

With bulk-memory, we get a __wasm_init_memory function:

(module $custom-section-name.bss.wasm (type (;0;) (func)) (import "env" "memory" (memory (;0;) 2)) (func $__wasm_call_ctors (;0;) (type 0)) (func $__wasm_init_memory (;1;) (type 0) i32.const 1036 i32.const 0 i32.const 4 memory.fill ) (global $__stack_pointer (;0;) (mut i32) i32.const 66576) (start $__wasm_init_memory) (data $WowZero! (;0;) (i32.const 1024) "\00\00\00\00") (data $MyAwesomeSection (;1;) (i32.const 1028) "*\00\00\00") (data $AnotherGreatSection (;2;) (i32.const 1032) "\07\00\00\00") (@custom "target_features" (after data) "\06+\0bbulk-memory+\16call-indirect-overlong+\0amultivalue+\0fmutable-globals+\0freference-types+\08sign-ext") )

Without bulk-memory we get a .bss section:

(module $custom-section-name.bss.wasm (type (;0;) (func)) (import "env" "memory" (memory (;0;) 2)) (func $__wasm_call_ctors (;0;) (type 0)) (global $__stack_pointer (;0;) (mut i32) i32.const 66576) (data $WowZero! (;0;) (i32.const 1024) "\00\00\00\00") (data $MyAwesomeSection (;1;) (i32.const 1028) "*\00\00\00") (data $AnotherGreatSection (;2;) (i32.const 1032) "\07\00\00\00") (data $.bss (;3;) (i32.const 1036) "\00\00\00\00") (@custom "target_features" (after data) "\05+\16call-indirect-overlong+\0amultivalue+\0fmutable-globals+\0freference-types+\08sign-ext") )

The test is intended to test the use of the .bss section, and we only get a .bss section in this test if we disable bulk-memory.

Hm, interesting, so we are using passive segments anytime bulk memory is enabled. That makes sense since this was all meant for threads, but is this actually an improvement in the non-threads case? i.e. maybe we don't want the extra explicit initialization code if we can help it?

edit: i just checked this out but I'll leave it here FTR: it looks like after linking we actually don't use passive segments unless threads is also enabled. so this seems like not a problem.

aheejin

This LGTM, but I guess we need to wait until Emscripten disables them by default, because there are some failing Emscripten tests and Binaryen doesn't yet have lowering passes for these. I don't think we need to block this on all these; we would just need to disable these two feature in Emscripten by default in the meantime.

nikic · 2024-10-16T08:37:50Z

I'd like #63755 to be fixed before this lands. Rust will likely crash and burn otherwise.

sunfishcode · 2024-10-16T15:09:32Z

I've now added a patch which fixes #63755.

github-actions · 2024-10-16T15:11:28Z

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

aheejin · 2024-10-16T20:31:45Z

I've now added a patch which fixes #63755.

Can you split this patch into a separate PR?

sunfishcode · 2024-10-16T21:05:56Z

Can you split this patch into a separate PR?

Sure, I've now created #112617 with just the that patch.

We were prepared to enable these features [back in February], but they got pulled for what appear to be unrelated reasons. So let's have another try at enabling them! Another motivation here is that it'd be convenient for the [Trail1 proposal] if "trail1" is a superset of "generic". [back in February]: WebAssembly/tool-conventions#158 (comment) [Trail1 proposal]: llvm#112035

When clang starts enabling features by default that Emscripten's minimum browser versions want disabled, Emscripten will need to explicitly disable them. This PR does that; once clang rolls, we can update the default versions and tests in a single commit if needed. See also llvm/llvm-project#112049

…bled. Fix disabling "bulk-memory" via function attributes. - Fix FixFunctionBitcasts to add attributes to the generated wrapper functions so that they don't pick up default target features. - Fix `coalesceFeatures` to only consider defined functions. - Fix `coalesceFeatures` to combine the union starting from the empty set, rather than starting from the target machine set, so that if some feature is explicitly disabled in all functions, it's disabled in the result. This fixes the wasm/lto/libcall-archive.ll test once bulk-memory is enabled, as that test depends on being able to disable bulk-memory via function attributes.

dschuff

I think Emscripten is ready for this now, thanks for working on this!

sunfishcode · 2024-10-25T20:51:44Z

Thanks!

…lvm#112049) We were prepared to enable these features [back in February], but they got pulled for what appear to be unrelated reasons. So let's have another try at enabling them! Another motivation here is that it'd be convenient for the [Lime1 proposal] if "lime1" is close to a subset of "generic" (missing only for extended-const). [back in February]: WebAssembly/tool-conventions#158 (comment) [Lime1 proposal]: llvm#112035

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

sunfishcode mentioned this pull request Oct 11, 2024

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

Closed

llvmbot added the mc Machine (object) code label Oct 11, 2024

sunfishcode force-pushed the sunfishcode/features branch from 00f5c6f to bfa9b85 Compare October 14, 2024 10:39

llvmbot added lld lld:wasm labels Oct 14, 2024

sunfishcode force-pushed the sunfishcode/features branch from bfa9b85 to 4283395 Compare October 14, 2024 11:13

sbc100 requested a review from dschuff October 14, 2024 22:20

sbc100 reviewed Oct 14, 2024

View reviewed changes

lld/test/wasm/lto/libcall-archive.ll Outdated Show resolved Hide resolved

aheejin reviewed Oct 16, 2024

View reviewed changes

dschuff mentioned this pull request Oct 16, 2024

Explicitly set bulk memory and nontrapping-fptoint clang flags emscripten-core/emscripten#22751

Merged

aheejin approved these changes Oct 16, 2024

View reviewed changes

sunfishcode added 7 commits October 24, 2024 14:15

Update tests.

8a655be

Update tests.

229888f

Update tests.

d757a4d

Move -bulk-memory flags from wasm-ld flags to a function attributes.

6f4d40e

Delete redundant features.

d3e79ec

Sort the feature macros alphabetically.

04b0211

sunfishcode force-pushed the sunfishcode/features branch from 23fec58 to 04b0211 Compare October 24, 2024 21:51

sunfishcode added 3 commits October 25, 2024 06:13

code formatting

a119df5

formatting

548564b

dschuff approved these changes Oct 25, 2024

View reviewed changes

sunfishcode merged commit 1bc2cd9 into llvm:main Oct 25, 2024
9 checks passed

sunfishcode deleted the sunfishcode/features branch October 25, 2024 20:52

aheejin added a commit to aheejin/llvm-project that referenced this pull request Nov 5, 2024

Add -mattr=+-bulk-memory to tests to match llvm#112049

289e75c

CryZe mentioned this pull request Nov 5, 2024

rust 1.82.0 wasm32-unknown-unknown triggers issues with wasm-bindgen >= 0.2.94 rust-lang/rust#132620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049

[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049

sunfishcode commented Oct 11, 2024 •

edited

Loading

llvmbot commented Oct 11, 2024 •

edited

Loading

llvmbot commented Oct 11, 2024

aheejin Oct 16, 2024

sunfishcode Oct 16, 2024 •

edited

Loading

aheejin Oct 16, 2024

sunfishcode Oct 16, 2024

dschuff Oct 16, 2024

aheejin left a comment

nikic commented Oct 16, 2024 •

edited

Loading

sunfishcode commented Oct 16, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

aheejin commented Oct 16, 2024

sunfishcode commented Oct 16, 2024

dschuff left a comment

sunfishcode commented Oct 25, 2024

		@@ -1,4 +1,4 @@
		; RUN: llc -filetype=obj %s -o %t.o
		; RUN: llc -filetype=obj -mattr=-bulk-memory %s -o %t.o

[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049

[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049

Conversation

sunfishcode commented Oct 11, 2024 • edited Loading

llvmbot commented Oct 11, 2024 • edited Loading

llvmbot commented Oct 11, 2024

aheejin Oct 16, 2024

Choose a reason for hiding this comment

sunfishcode Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

aheejin Oct 16, 2024

Choose a reason for hiding this comment

sunfishcode Oct 16, 2024

Choose a reason for hiding this comment

dschuff Oct 16, 2024

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

nikic commented Oct 16, 2024 • edited Loading

sunfishcode commented Oct 16, 2024

github-actions bot commented Oct 16, 2024 • edited Loading

aheejin commented Oct 16, 2024

sunfishcode commented Oct 16, 2024

dschuff left a comment

Choose a reason for hiding this comment

sunfishcode commented Oct 25, 2024

sunfishcode commented Oct 11, 2024 •

edited

Loading

llvmbot commented Oct 11, 2024 •

edited

Loading

sunfishcode Oct 16, 2024 •

edited

Loading

nikic commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading