Enabling features by default in LLVM

[tlively]

I know we discussed this a while ago, but I forget what exactly we came up with @aheejin @dschuff @sbc100 @sunfishcode. IIRC, it wasn't too different from enabling new features once the following two conditions are satisfied:

• The feature has been in phase 4 for six months.
• All major browsers have shipped it.

Am I remembering that correctly? (I couldn't find this discussion in our notes). Should there be a time delay on the second condition as well? I'm thinking about this because multivalue and mutable globals might be eligible to be turned on by default. If so, it would be nice to get that in before LLVM 12 is cut.

[sbc100]

Thanks for reviving this issue. I seem to remember it was something I meant to do a while back.

Aside from the issue of what to do when in LLVM, did we agree that binaryen and emscripten should be in sync with LLVM (i.e. was there any reason not to have them all be in sync?)

[tlively]

I don't remember having discussed what to do with other tools, but IMO having them match LLVM seems reasonable.

[aardappel]

Same with WABT. You don't really want these tools to error on every LLVM generated binary because --enable-* was omitted.

[tlively]

For Binaryen this generally wouldn't be an issue because it reads the target features section by default. I assume WABT is more frequently used with binaries that have had their target features section stripped, though.

[sbc100]

Yeah I think it makes sense for all tools to stay in sync. They should all probably strive for an --mvp mode that can be set on the command line for folks that don't like progress.

[AlexEne]

Is this criteria decided? As someone who works in non-web browser wasm32-wasi scenarios, making this tied to browser release cycle makes it different from all other rustc targets and at a pace where it's not ideal for anything other than web.

The feature has been in phase 4 for six months. All major browsers have shipped it.

What's the mechanism to decide this as doing things in this way hurts places like IOT, or others where HOST programs cannot be updated due to various reasons at the same pace. This essentially locks application writers that target wasm to run on those host programs to a particular rust version (as WASM and WASI evolve). In the case of wasi, this may be alleviated by having stdlibs independent of rustc releases (not possible today tho).

E.g. if we could use the latest rust with a stdlib from 3 versions ago, then the addition of new wasi capabilities for wasm32-wasi targets won't really affect this type of use-cases.

This also goes towards a more philosophical discussion of what does it mean to have targets in stable rust and what guarantees those build targets are going to be held against but this is probably not the right forum to discuss it I think.

[sbc100]

Presumably rust, like any other toolchain, will need some way to disable certainly features at compile time if they hope to target older/legacy runtimes? If not, then its output would only be use-able on platforms that support the same set of defaults (i.e. no legacy targets).

Another option for toolchains (other than allowing feature selection on the command line) is to use a tool like binaryen to remove the use of certain features, post link. This would allow the compiler itself to be less configurable, and delay feature selection until a late phase. (Obviously this only works for features that can be polyfilled or re-written away)

[AlexEne]

I like the disable feature for defaults that break backwards compatibility. E.g. Threads won't break anything as now for example rust panics whenever you try to instantiate a thread, so having it panic because it can't find some functionality after threads are moved into the default feature set doesn't really qualify as breaking.

But in the case they are breaking, we could add flags to disable those once they move into the default set of features. The case i'm describing isn't the majority so adding some flags to remove breaking stuff makes sense

[sbc100]

As well is individual features tools can/should probably provide an --mvp flag of some find to remove all non-mvp features. I believe binaryen, wabt and llvm already support this.

[sunfishcode]

The proposed wording at the top of this issue talks about browsers, which doesn't address major non-Web engines. And even among browsers, there are different versions. And the idea of documenting a 6-month waiting period feels too rigid. Some features have taken engines longer to implement. Others were implemented before reaching stage 4.

Even if we wrote words to pin these things down more, for the foreseeable future I expect we'd still end up adding an informal decision-making on top for each feature anyway. So I've now added a comment to this proposed LLVM patch which just says:

+// This includes features that have achieved phase 4 of the standards process,
+// and that are expected to work for most users in the current time, with
+// consideration given to available support in relevant engines and tools, and
+// the importance of the features.

[kripken]

I agree some amount of informal reasoning is necessary.

But I do hope we can do that in a cross-ecosystem way, that is, avoid differences between toolchains. In some sense maybe it's ok if say Emscripten is web-focused and makes choices based on browsers, and WASI-libc is server-focused and chooses based on server VMs, and maybe those choices end up different sometimes. But if it would be possible to make a single set of decisions across the entire ecosystem I think that simplicity would have benefits.

[sunfishcode]

@kripken Do you have a concern about the wording of the comment above, or about the selection of proposed features in the patch: bulk-memory, mutable-globals, nontrapping-fptoint, and sign-ext?

[kripken]

The wording seems ok to me. About the selection, I'd be happiest if we knew we were also going to enable those precise features in Emscripten shortly. I'm not sure of current plans there (@sbc100 @dschuff ?).

But I guess my larger suggestion is, it might be nice if we decided at the level of the entire ecosystem on the set of features rather than doing so in separate toolchains. I don't feel terribly strongly, though - I think that would have benefits, but minor, so if it slows progress maybe it's not worth it.

[dschuff]

In terms of switching emscripten defaults, we need to decide when we're ready to break users on old devices with the default build, and finish getting the tools and/or documentation in place to help developers target them.

For the first question, the long pole on the web is nontrapping and bulk memory, shipped in Safari/iOS 15, which Apple says is on about 90% of phones and 80% of tablets released in the last 4 years. If we don't like those numbers, we could just enable mutable-gobals and signext as they have been out for longer. Of those I might guess that nontrapping-fptoint would be the most valuable because of the extra blocks we have to insert to implement LLVM IR's version of FP to int conversion.

Then we need to decide whether we want to expand the use of the -s MIN_SAFARI_VERSION etc flags at compile time, or lower features away at link time with Binaryen as mentioned above, or just have a doc that says "if you want to target Safari 15, use the following feature flags".

Regarding the ecosystem I think it would be nice to all move together too, and if we can get get this first step coordinated we'd at least have done the easy first step. It wouldn't necessarily surprise me if there were some SDK in the future that went their own way; that was basically the sentiment expressed above that people might not want to wait on Safari to implement something to enable a feature in some completely unrelated embedding. But if we coordinate LLVM itself, at lease we're applying a nudge in that direction.

[sunfishcode]

I've now updated D125729 to remove bulk-memory and nontrapping-fptoint. Does anyone have any objections to the patch now?

[kripken]

sgtm. It does seem safer to not enable features by default that won't work on almost 20% of a major platform like iPhone, since it says only 82% of all iPhones are at 15. (I don't see a reason to focus on just those released in the last 4 years @dschuff ?)

Also just 72% of iPads, which is a smaller market, but still that's almost 30%.

I guess iOS ties major Safari updates to OS updates? Especially unfortunate as apps can't ship their own browser engines.

[fgmccabe]

Technically, on ios & mac, safari updates are not always connected to os upgrades.

[kripken]

@fgmccabe Major versions, or security updates? (I did some searching and can't find a clear answer, but I'm not an Apple user so maybe I don't know the right terms.)

[fgmccabe]

Definitely both. It's a mostly transparent process on ios: apps are 'just updated' every so often, whenever the developer releases a new version. This appears similar to how chrome is updated in the background too.

[sbc100]

@fgmccabe, that seems to contradict the information that apple supplies on how to update safari: https://support.apple.com/en-us/HT204416: "If a Safari update is available, you can get it by following the steps to update or upgrade macOS, iOS, or iPadOS." I'm not an iOS user so don't have first hand information, but are you sure? Where are you getting your information about this from?

[fgmccabe]

Well, I have seen those message too. I have also experienced it being upgraded without (esp on ios). But, my experience is stricly anecdotal.

[jryans]

On macOS, it's definitely possible (at least in recent times) to update just Safari alone (though not always to the latest Safari version if your version of macOS is old enough that support has been dropped). For example, if I'm on macOS major version N - 1 (where N is the latest), sometimes the latest Safari is available as a standalone update. The Safari version history on Wikipedia shows that each release is available for multiple macOS versions:

On iOS, as far as I've seen over the last ~5 years at least, Safari is always tied to the overall iOS system version. I have iOS app auto-updates disabled and review each app updates manually before installing them. I do not recall seeing a Safari standalone update on iOS. Once again, the Safari version history on Wikipedia supports this by showing each Safari is connected to an iOS version:

So to summarise, AFAIK only macOS allows installing Safari separately from the overall OS version. In recent times, only the previous 1 or 2 macOS versions have been supported by the latest version of Safari.

@fgmccabe, that seems to contradict the information that apple supplies on how to update safari: support.apple.com/en-us/HT204416: "If a Safari update is available, you can get it by following the steps to update or upgrade macOS, iOS, or iPadOS."

That wording doesn't really help with this particular question because (at least on macOS where I have seen standalone Safari updates before), they are delivered through the same settings UI flow for upgrading the overall system version. You have to manually choose to install only the Safari update and not the latest macOS (they appear as separate line items that can each be unchecked).

[sunfishcode]

As an update here, D125729 updating LLVM to enable mutable-globals and signext has now landed.

[fornwall]

multivalue and mutable globals might be eligible to be turned on by default. If so, it would be nice to get that in before LLVM 12 is cut

While this didn't happen, what about enabling multi-value by default in LLVM 18 or 19 (can look into a PR if this might be a good idea)? Considering the guideline that was added in D125729 about what to enable by default, implementation support as well as the utility value (see for instance this comment):

// This includes features that have achieved phase 4 of the standards process, // and that are expected to work for most users in the current time, with // consideration given to available support in relevant engines and tools, and // the importance of the features.

[kripken]

Regarding multivalue specifically, it would be good to verify that enabling it does not cause any regressions. My specific concern is that wasm-opt's handling of multivalue is not yet optimal (though it has improved).

[fornwall]

Regarding multivalue specifically, it would be good to verify that enabling it does not cause any regressions. My specific concern is that wasm-opt's handling of multivalue is not yet optimal (though it has https://github.com/WebAssembly/binaryen/pull/5937).

Good point! I'll experiment a bit with wasm-opt and multivalue to see code size and benchmark differences. Would be good if anyone has input or experience with the readiness of the webassembly ecosystem in general, regarding potentially enabling multivalue by default in LLVM next year.

For rust https://github.com/rust-lang/rust/issues/73755 is (finally!) resolved, but wasm-bindgen support remains: https://github.com/rustwasm/wasm-bindgen/issues/3552.

[alexcrichton]

I might recommend opening a dedicated issue for enabling multi-value for discussion there since this issue itself isn't necessarily about that specifically. Multi-value is tricky because if it changes the default ABI then it's a breaking change, unlike many other features in LLVM which aren't breaking like sign-extension-ops which only enables a few extra instructions to get emitted. If the intention is to not change the default ABI, however, then compatibility shouldn't be necessary since using multi-value in the ABI would be opt-in.

[aheejin]

I happened to have an offline discussion w/ some people here today about enabling more features including multivalue by default in LLVM. Syncing Binaryen sounds good too, if people can always fall back to --mvp or equivalent.

One thing @tlively pointed out was enabling multivalue feature by default in clang does NOT automatically cause LLVM to use the multivalue ABI and thus change the code quality. Enabling multivalue feature in the features section is separate from opting to use the multivalue ABI, which is currently controlled by a separate option and not very much used or tested. Given that it is not gonna cause the changes some people here were concerned about, I don't see much downsides in enabling some more features that have been standardized (= reached Phase 5) for years at this point.

I think we can probably enable these by default at this point:

• multivalue
• nontrapping-fptoint

How do people feel about these? reference-types seems harmless. Does enabling bulk-memory change something in generated code by default?

• reference-types
• bulk-memory

[tlively]

Enabling bulk-memory does change how memset and memcpy calls are lowered, but that should be fine. It may change how small on-stack memory copies are lowered as well, but I don't remember the details there.

[aheejin]

I tried enabling four more features (multivalue, nontrapping-fptoint, reference-types, and bulk-memory) in https://github.com/llvm/llvm-project/pull/80923. Comments will be appreciated!

[CryZe]

@aheejin I may have missed something, but why was this done specifically for clang and not LLVM? https://github.com/llvm/llvm-project/blob/e62c2146aa9a195c219b3585eb36c6987857c1bb/llvm/lib/Target/WebAssembly/WebAssembly.td#L106-L113

By having done this in the clang frontend only, Rust is now missing out on these features.

[aheejin]

@CryZe

@aheejin I may have missed something, but why was this done specifically for clang and not LLVM? https://github.com/llvm/llvm-project/blob/e62c2146aa9a195c219b3585eb36c6987857c1bb/llvm/lib/Target/WebAssembly/WebAssembly.td#L106-L113

By having done this in the clang frontend only, Rust is now missing out on these features.

Thanks for the catch, and sorry for the late reply. I updated them in https://github.com/llvm/llvm-project/pull/96584.

[aheejin]

I tried enabling four more features (multivalue, nontrapping-fptoint, reference-types, and bulk-memory) in llvm/llvm-project#80923. Comments will be appreciated!

We ended up enabling two of the features (multivalue and reference-types) first. (https://github.com/llvm/llvm-project/pull/80923, https://github.com/llvm/llvm-project/pull/90792, https://github.com/llvm/llvm-project/pull/93261, and https://github.com/llvm/llvm-project/pull/96584).

Enabling the other two (nontrapping-fptoint and bulk-memory) requires solving some issues first, and I hope we can enable them not too far in the future as well.

[glandium]

I tried enabling four more features (multivalue, nontrapping-fptoint, reference-types, and bulk-memory) in llvm/llvm-project#80923. Comments will be appreciated!

We ended up enabling two of the features (multivalue and reference-types) first. (llvm/llvm-project#80923, llvm/llvm-project#90792, llvm/llvm-project#93261, and llvm/llvm-project#96584).

Enabling the other two (nontrapping-fptoint and bulk-memory) requires solving some issues first, and I hope we can enable them not too far in the future as well.

Building wasi-sdk/wasi-libc with current clang trunk (with reference-types enabled) leads to crt1-command.o containing an undefined reference to indirect_function_table that lld doesn't like. I don't know whether wasi-libc should be changed to force-disable reference types, or if there's a bug in lld (because there are undefined references to indirect_function_table in other objects too, but lld only fails when it's crt1-command.o). I can provide a reproducer if necessary.

[aheejin]

@glandium I'd appreciate the reproducer.

[glandium]

@aheejin here you go: repro.zip

Unpack and run wasm-ld @response.txt.

[aheejin]

@aheejin here you go: repro.zip

Unpack and run wasm-ld @response.txt.

The command in response.txt has --import-table, and builds/worker/fetches/sysroot-wasm32-wasi/lib/wasm32-wasi/crt1-command.o imports __indirect_function_table:

 (import "env" "__indirect_function_table" (table $timport$0 0 funcref))

llvm-nm lists the symbol as undefined (as expected, because it is to be imported):

$ llvm-nm builds/worker/fetches/sysroot-wasm32-wasi/lib/wasm32-wasi/crt1-command.o
         U __indirect_function_table
         ...

It looks this causes an error, but I'm not very familiar with why this works in this way. @sbc100 Any ideas?

[sbc100]

Looks like that could be a bug in wasm-ld yes.

@glandium to be clear its not the building of wasi-sdk that fails here but you usage of the SDK once its built, right?

Out of interest, is there some reason why you need to build with --import-memory and --import-table?

[glandium]

@glandium to be clear its not the building of wasi-sdk that fails here but you usage of the SDK once its built, right?

Yes

Out of interest, is there some reason why you need to build with --import-memory and --import-table?

The wasm is translated to C afterwards via wasm2c for consumption by rlbox (https://github.com/PLSysSec/rlbox_wasm2c_sandbox). I'm not sure exactly why these flags are used, specifically, but @shravanrn should know.

[shravanrn]

@sbc100 RLBox uses both import-memory and import-table because of the following high level constraints

• the wasm host/embedder has specific alignment and size requirements on memory allocated for a wasm instance's linear memory, hence the memory is created by the host and imported by the module

• the wasm host/embedder needs to dynamically make some of it's functions callable by the wasm instance. This can be done by inserting these functions as entries to the wasm instance's indirect function table. For this, we make the host responsible for creating (and updating) the table and simply import the table from the wasm instance

I can offer more concrete/specific examples if needed.

[glandium]

@aheejin here you go: repro.zip Unpack and run wasm-ld @response.txt.

The command in response.txt has --import-table, and builds/worker/fetches/sysroot-wasm32-wasi/lib/wasm32-wasi/crt1-command.o imports __indirect_function_table:

 (import "env" "__indirect_function_table" (table $timport$0 0 funcref))

llvm-nm lists the symbol as undefined (as expected, because it is to be imported):

$ llvm-nm builds/worker/fetches/sysroot-wasm32-wasi/lib/wasm32-wasi/crt1-command.o
         U __indirect_function_table
         ...

It looks this causes an error, but I'm not very familiar with why this works in this way. @sbc100 Any ideas?

Note that the symbol is also similarly undefined in most if not all the other files in the archive, but that doesn't cause a problem. Only the one in crt1-command.o does. As in, if you get rid of the one in crt1-command.o somehow, then it links fine.

[sbc100]

Note that the symbol is also similarly undefined in most if not all the other files in the archive, but that doesn't cause a problem. Only the one in crt1-command.o does. As in, if you get rid of the one in crt1-command.o somehow, then it links fine.

Indeed, this looks like a wasm-ld a bug. I will look into it.

[sbc100]

It looks like there are actually two bugs here. The first one was mis-reporting the source of the undefined symbols references. They are actually coming from __stdio_exit.o. I'm fixes that first issue in https://github.com/llvm/llvm-project/pull/97444.

[sunfishcode]

The original issue here is resolved in D125729. "generic" will evolve over time, as features are implemented in engines.

https://github.com/WebAssembly/tool-conventions/issues/233 is a discussion about the specific feature of reference-types being enabled by default in LLVM 19, and the change to call_indirect encoding.

People interested in feature sets may be interested in https://github.com/WebAssembly/tool-conventions/pull/235, which proposes Trail1, a new table target "CPU" configuration for LLVM.

Also relevant here is that I've now submitted https://github.com/llvm/llvm-project/pull/112049 to LLVM to enable nontrapping-fptoint and bulk-memory. They were planned to be enabled here back in February but were reverted for what appear to be unrelated issues, so I'm now proposing to re-add them.

And finally here, @sbc100, you mentioned here that there were two bugs. It looks like those were fixed in https://github.com/llvm/llvm-project/pull/97444 and https://github.com/llvm/llvm-project/pull/97451. If that's true, then I propose we can close this issue now.

[sbc100]

Yup, it looks like both issue were fixed.

[sunfishcode]

Thanks!