Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono] fix SIMD instructions' availability #34319

Merged
merged 3 commits into from
Apr 1, 2020

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 31, 2020

Fixes: #34304

@tannergooding @davidwrighton
Now it fully matches clang/LLVM behavior, e.g. even -mattr=+avx2 (or -mavx2 in clang) doesn't enable pcmul and aes. However, -mattr=+sse4.2 (-msse4.2) enables popcnt (see https://godbolt.org/z/RWAp_D). The intel manual says

image

So is there a CPU with AVX set but without popcnt? The instruction is pretty useful (it is used in BCL) users might forget to define it when they use mattr=avx (or mattr=avx2).

However in C++ world programmers use named groups e.g. mcpu=haswell rather than listing these sets via attributes.

@tannergooding
Copy link
Member

So is there a CPU with AVX set but without popcnt?

I would expect that this wouldn't happen in practice, but I think it is still appropriate to model the underlying checks accurately so we aren't stuck with a breaking change in the future if that ever changes.

For Intel, POPCNT was introduced as part of SSE4.2 (2008, Nehalem), but it is a separate CPUID check because AMD had already introduced POPCNT and LZCNT independently as part of ABM (2007, Barcelona). Intel didn't introduce LZCNT until much later alongside AVX2/BMI1/BMI2/FMA3 (2013, Haswell).

MONO_CPU_X86_FULL_SSEAVX_COMBINED = MONO_CPU_X86_FMA_COMBINED | MONO_CPU_X86_AVX2,
MONO_CPU_X86_AVX2_COMBINED = MONO_CPU_X86_AVX_COMBINED | MONO_CPU_X86_AVX2,
MONO_CPU_X86_FMA_COMBINED = MONO_CPU_X86_AVX_COMBINED | MONO_CPU_X86_FMA,
MONO_CPU_X86_FULL_SSEAVX_COMBINED = MONO_CPU_X86_FMA_COMBINED | MONO_CPU_X86_AVX2 | MONO_CPU_X86_PCLMUL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is a bit confusing, I'm not quite sure based on the name along, what is meant to be enabled by it.

Copy link
Member Author

@EgorBo EgorBo Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tannergooding it's only used internally to be able to disable SSE/AVX like that:
mattr=-sse3 => disable sse3, also ssse3, sse4.1, etc (but don't touch lzcnt, bmi)

see here: https://github.com/dotnet/runtime/blob/master/src/mono/mono/mini/aot-compiler.c#L8109-L8111

it also matches llvm behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it allows to do things like this mcpu=native,mattr=-avx

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and to match the hierarchy specified by the Intel/AMD architecture manuals.

@EgorBo
Copy link
Member Author

EgorBo commented Apr 1, 2020

The failing job is known and unrelated

@EgorBo EgorBo merged commit 0ff46ab into dotnet:master Apr 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review MONO_CPU_X86_SSSE3_COMBINED
5 participants