Move from an "asm" flag to a "no-asm" feature flag

[josephlr]

Right now there's no way to get the assembly related improvements when building this crate with -Zbuild-std, as the asm feature flag is not enabled by libstd. This is also preventing performance improvements from getting into the pre-built libstd (see rust-lang/rust#39078).

The solution to this is to have a "no-asm" feature. That way, by default, we will get the optimized version. This is reasonable as the default backend (rustc) supports inline assembly.

CC: @bjorn3 @alexcrichton

Signed-off-by: Joe Richey joerichey@google.com

[bjorn3]

Could you also make a PR to disable it by default in libstd and only enable it when libstd has a certain feature flag enabled? It is fine to make this feature flag enabled by default, for as long as libtest disables it by default.

[josephlr]

Could you also make a PR to disable it by default in libstd and only enable it when libstd has a certain feature flag enabled? It is fine to make this feature flag enabled by default, for as long as libtest disables it by default.

libtest having it on by default is actually the main thing I wanted to achieve with this PR (as that's what determines what flags get set by -Zbuild-std). We could add yet another feature flag for users to pass via -Zbuild-std-features, but that seems unergonomic, the asm feature shouldn't be disabled by default for rustc builds.

EDIT: Would it be possible for cg_clif to disable the "asm" feature when building this crate, and leave it to default on for rustc?

[bjorn3]

cg_clif can't remove cfg features. What I want to achieve is that simply passing --no-default-features/default-features = false is enough to disable asm. Maybe have compiler-builtins, libstd and libtest use default-features = false and have a feature that is enabled by default enabling asm.

# compiler-builtins
[features]
default = ["asm"]
asm = []

# std
[dependencies]
compiler-builtins = { version = "...", default-features = false, features = ["compiler-builtins"] }

[features]
default = ["asm"]
asm = ["compiler-builtins/asm"]

# test
[dependencies]
std = { path = "../std", default-features = false }

[features]
default = ["asm"]
asm = ["std/asm"]

[alexcrichton]

I think it'd be best to add a feature to libstd to enable the asm feature, and that way -Zbuild-std can opt-in as necessary and rustbuild can be tweaked to pass it where necessary.

[josephlr]

@bjorn3 @alexcrichton would having a clif feature for this crate work better? It would make it clear why the feature existed, and could also be used to disable other things Cranelift doesn't support (like stack probes). This also makes the Cranelift workarounds opt-in. So -Zbuild-std can use everything normally, and all cg_clif has to do is enable the clif feature on this crate.

This also means that as cg_clif supports more of Rust, we can disable these workarounds without changing the external feature set.

[bjorn3]

What about naming it no_asm instead? That makes it clear that it isn't just for cg_clif, as other backends may also not support inline asm.

[alexcrichton]

I don't really mind how this works myself, I think it just needs to solve the constraint of "it should be able to compile this crate in libstd without asm!". However that works I don't really mind myself.

If this is enabled by default (as in this PR), is there an example of how to disable this feature as part of building libstd?

[josephlr]

What about naming it no_asm instead? That makes it clear that it isn't just for cg_clif, as other backends may also not support inline asm.

That's a good idea. I've updated this PR and the description to do this. The "asm" feature is gone and the "no-asm" feature is added in it's place.

I don't really mind how this works myself, I think it just needs to solve the constraint of "it should be able to compile this crate in libstd without asm!". However that works I don't really mind myself.

Ya, @bjorn3's idea to just have a "no-asm" feature works a lot better than my original idea. Having default features for libstd and its dependencies is a hassle, and we should avoid it if possible. Now, by default, things just work for rustc.

If this is enabled by default (as in this PR), is there an example of how to disable this feature as part of building libstd?

Now the question would be how to enable "no-asm" when using -Zbuild-std. This would require adding a (non-default) "no-asm" feature flag to core/alloc/std/test, which forwards to this feature. This feature would also disable any inline-assembly used within libcore.

[alexcrichton]

I think that the -Zbuild-std-features flag should work for enabling this, right?

[josephlr]

I think that the -Zbuild-std-features flag should work for enabling this, right?

Correct, once the no-asm feature is properly forwarded into libtest.

Note that libcore and libstd currently use inline asm, so they will also need to gate those implementations with #[cfg(not(feature = "no-asm"))] for cg_clif to compile the standard library.

[alexcrichton]

Ok sounds good to me! Is this ready to go or would you like some more time?

[josephlr]

This is ready to be merged. @bjorn3 should I open a PR to rust-lang/rust to add the no-asm feature to libcore/liballoc/libstd/libtest/etc... or do you want to do that?

[bjorn3]

I can do it, but if you want to do it that is fine with me too.

[alexcrichton]

👍

[stlankes]

@josephlr @alexcrichton @bjorn3 I introduced the asm feature in alloc and std (rust-lang/rust#78472). Maybe we should remove the asm feature and use the latest compiler builtins in these crates.