Cranelift: ensure ISA level needed for SIMD is present when SIMD is enabled. #3816

cfallin · 2022-02-16T20:20:54Z

Addresses #3809: when we are asked to create a Cranelift backend with
shared flags that indicate support for SIMD, we should check that the
ISA level needed for our SIMD lowerings is present.

github-actions · 2022-02-16T21:11:29Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

cfallin · 2022-02-16T21:55:55Z

Pushed updates to propagate through the rest (I only cargo check'd cranelift-codegen and wasmtime-cli before original push) and it seems that Engine::default() is actually giving an Engine without the right CPU flags to allow for SIMD; this is somewhat perplexing and I'll look into it further.

abrown

Yeah, this makes sense to me. @cfallin, don't know if I understand your last comment: does your latest commit fix the issue or are more changes required?

cfallin · 2022-02-17T00:03:44Z

Yeah, this makes sense to me. @cfallin, don't know if I understand your last comment: does your latest commit fix the issue or are more changes required?

Yes, it just needed one more tweak (pushed now) -- namely to set SSE3, SSSE3, SSE4.1 on by default for a Flags that is constructed manually (vs. from cranelift_native); otherwise a wasmtime compile fails because SIMD is on by default, and the CLI machinery sets an explicit target which establishes default feature flags.

…nabled. Addresses bytecodealliance#3809: when we are asked to create a Cranelift backend with shared flags that indicate support for SIMD, we should check that the ISA level needed for our SIMD lowerings is present.

bjorn3 · 2022-02-17T10:51:51Z

cranelift/codegen/meta/src/isa/x86.rs

@@ -17,19 +17,22 @@ fn define_settings(shared: &SettingGroup) -> SettingGroup {
        "has_sse3",
        "Has support for SSE3.",
        "SSE3: CPUID.01H:ECX.SSE3[bit 0]",
-        false,
+        // Needed for default `enable_simd` setting.
+        true,


This means explicitly using the baseline preset will now incorrectly include these flags, right?

Will include those flags, yes; "incorrectly" is a matter of interpretation, as we effectively need them at the moment to support our default enable_simd setting. Otherwise a default config will not work with default Wasmtime settings.

If I enable the baseline preset and not enable_simd, then these features shouldn't be enabled as it won't run on a cpu only implementing the x86_64 baseline. Maybe add them as dependency of enable_simd or enable them in wasmtime?

We could do it that way as well; happy to take a PR if you want to refactor in that direction. From my point of view, this should be relatively temporary anyway, as we should eventually (#3810) be able to do SIMD without any optional extensions.

Fuzzing over the weekend found that `i64x2` comparison operators require `pcmpgtq` which is an SSE 4.2 instruction. Along the lines of bytecodealliance#3816 this commit unconditionally enables and requires SSE 4.2 for compilation and fuzzing. It will no longer be possible to create a compiler for x86_64 with simd enabled if SSE 4.2 is disabled.

* Enable SSE 4.2 unconditionally Fuzzing over the weekend found that `i64x2` comparison operators require `pcmpgtq` which is an SSE 4.2 instruction. Along the lines of #3816 this commit unconditionally enables and requires SSE 4.2 for compilation and fuzzing. It will no longer be possible to create a compiler for x86_64 with simd enabled if SSE 4.2 is disabled. * Update comment

…nabled. (bytecodealliance#3816) Addresses bytecodealliance#3809: when we are asked to create a Cranelift backend with shared flags that indicate support for SIMD, we should check that the ISA level needed for our SIMD lowerings is present.

* Enable SSE 4.2 unconditionally Fuzzing over the weekend found that `i64x2` comparison operators require `pcmpgtq` which is an SSE 4.2 instruction. Along the lines of bytecodealliance#3816 this commit unconditionally enables and requires SSE 4.2 for compilation and fuzzing. It will no longer be possible to create a compiler for x86_64 with simd enabled if SSE 4.2 is disabled. * Update comment

In bytecodealliance#4224 we saw that an SSE2-only x86-64 system somehow was still detecting SSE3/SSSE3/SSE4.1/SSE4.2. It turns out that we enabled these in the baseline `Flags` in bytecodealliance#3816, because without that, a ton of other things break: default flags no longer produce a compiler backend that works with default Wasmtime settings. However the logic to set them when detected (via `CPUID`-using feature-test macros) only does an "if detected then set bit" step per feature; the bits are never *cleared*. This PR fixes that.

…4231) In #4224 we saw that an SSE2-only x86-64 system somehow was still detecting SSE3/SSSE3/SSE4.1/SSE4.2. It turns out that we enabled these in the baseline `Flags` in #3816, because without that, a ton of other things break: default flags no longer produce a compiler backend that works with default Wasmtime settings. However the logic to set them when detected (via `CPUID`-using feature-test macros) only does an "if detected then set bit" step per feature; the bits are never *cleared*. This PR fixes that.

…ytecodealliance#4231) In bytecodealliance#4224 we saw that an SSE2-only x86-64 system somehow was still detecting SSE3/SSSE3/SSE4.1/SSE4.2. It turns out that we enabled these in the baseline `Flags` in bytecodealliance#3816, because without that, a ton of other things break: default flags no longer produce a compiler backend that works with default Wasmtime settings. However the logic to set them when detected (via `CPUID`-using feature-test macros) only does an "if detected then set bit" step per feature; the bits are never *cleared*. This PR fixes that.

cfallin requested review from alexcrichton and abrown February 16, 2022 20:20

alexcrichton approved these changes Feb 16, 2022

View reviewed changes

cfallin force-pushed the simd-require-sse41 branch from a218fb6 to 10db9a6 Compare February 16, 2022 20:40

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen wasmtime:api Related to the API of the `wasmtime` crate itself labels Feb 16, 2022

cfallin force-pushed the simd-require-sse41 branch 2 times, most recently from d392b51 to 007eb4d Compare February 16, 2022 21:54

abrown approved these changes Feb 16, 2022

View reviewed changes

cfallin force-pushed the simd-require-sse41 branch from 007eb4d to 20b6fb7 Compare February 17, 2022 00:01

cfallin force-pushed the simd-require-sse41 branch 2 times, most recently from 0efa03d to 7b47520 Compare February 17, 2022 00:14

cfallin force-pushed the simd-require-sse41 branch from 7b47520 to 7719a61 Compare February 17, 2022 00:48

github-actions bot added the cranelift:meta Everything related to the meta-language. label Feb 17, 2022

cfallin merged commit 1c014d1 into bytecodealliance:main Feb 17, 2022

cfallin deleted the simd-require-sse41 branch February 17, 2022 01:29

bjorn3 reviewed Feb 17, 2022

View reviewed changes

alexcrichton mentioned this pull request Feb 22, 2022

Enable SSE 4.2 unconditionally #3833

Merged

cfallin mentioned this pull request Jun 6, 2022

cannot test if target-specific flag "has_sse42" is available at runtime #4224

Closed

cfallin mentioned this pull request Jun 6, 2022

cranelift-native flags detection: fix flags on SSE2-only systems. #4231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cranelift: ensure ISA level needed for SIMD is present when SIMD is enabled. #3816

Cranelift: ensure ISA level needed for SIMD is present when SIMD is enabled. #3816

cfallin commented Feb 16, 2022

github-actions bot commented Feb 16, 2022

cfallin commented Feb 16, 2022

abrown left a comment

cfallin commented Feb 17, 2022

bjorn3 Feb 17, 2022

cfallin Feb 17, 2022

bjorn3 Feb 18, 2022

cfallin Feb 23, 2022

Cranelift: ensure ISA level needed for SIMD is present when SIMD is enabled. #3816

Cranelift: ensure ISA level needed for SIMD is present when SIMD is enabled. #3816

Conversation

cfallin commented Feb 16, 2022

github-actions bot commented Feb 16, 2022

Subscribe to Label Action

cfallin commented Feb 16, 2022

abrown left a comment

Choose a reason for hiding this comment

cfallin commented Feb 17, 2022

bjorn3 Feb 17, 2022

Choose a reason for hiding this comment

cfallin Feb 17, 2022

Choose a reason for hiding this comment

bjorn3 Feb 18, 2022

Choose a reason for hiding this comment

cfallin Feb 23, 2022

Choose a reason for hiding this comment