-
Notifications
You must be signed in to change notification settings - Fork 89
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
instructions: GFNI, VAES and VPCLMULQDQ naming inconsistency with x/sys/cpu #364
Comments
|
And everything above also applies to |
Thanks for pointing this out. I could consider a change in However, I tend to agree that we've got this right in In the event of implementing feature checks #168 I don't think having a special-case fixup for those specific ISAs is going to be that bad? Note that @klauspost's |
Ah! Sorry I'm just now grasping what you're saying. It's not simply an issue of applying a naming transform. |
If I recall correctly that's exactly what it does. It will use VEX unless it has to use EVEX.
Lines 786 to 799 in 05ed388
|
Yes, on reflection this is how it would have to work, otherwise it would be impossible to write valid AVX/AVX2 code and ensure that it would not fault on hardware without AVX512. |
Yes, there are pure SSE2 (non+VEX)/AVX2 (VEX) versions of these. There are "duplicates":
I assume the In #146 I was looking for a way to restrict extended registry usage. Example of CPU with GFNI and no AVX
|
I just checked and there doesn't appear to be an existing issue on the golang project for this problem. I've already prototyped it and the fix is straightforward, so I'm considering filing a new issue over there this week. |
With the addition of the new Ice Lake AVX-512 instruction support in Avo, I've encountered a couple of wrinkles in the naming of the required CPUID features for a given function generated by Avo and the golang x/sys/cpu feature detection support.
A bit of history... Up until this point, all Avo CPUID feature names have:
x/sys/cpu
flag.That is, if an Avo generated function says it requires "AVX2", "AVX512F" and "AVX512DQ", those strings can be directly used to generate code to test for the truth of those requrements. (e.g. a
"cpu.X86.Has%s"
template can be used to generate:cpu.X86.HasAVX2 && cpu.X86.HasAVX512F && cpu.X86.HasAVX512DQ
.)So here's the icky new problem. The Intel blessed (and Avo adopted) identifiers for the GFNI and VAES instructions are literally that. And there appears to be a good reason for this, because unlike AVX512 dependent features like AVX512_VPOPCNTDQ or AVX512_BITALG, Intel is signalling here that it intends to ship SSE and/or AVX (VEX) encoded forms of these instructions that can run on hardware without AVX512 support enabled.
In fact it apparently already has, shipping SSE encoded GFNI support on the "Intel Atom processor based on Tremont microarchitecture" that lacks AVX/AVX512.
So what's the wrinkle? Golang's standard library has defined and implemented identifiers that lock these features to AVX512 both in naming and in the detection logic. See: https://cs.opensource.google/go/x/sys/+/refs/tags/v0.4.0:cpu/cpu_x86.go;l=133-134
This is arguably a bug because, for example, that "Intel Atom processor based on Tremont microarchitecture" will have perfectly functional SSE encoded GFNI instructions that the Go assembler will happily emit (*but see comment below), but that can never run because the standard library assumes AVX512F is a requirement.
Backing out of this likely will require a breaking change to the semantics of the
x/sys/cpu
library, namely, eliminating the erroneousX86.HasAVX512GNFI
andX86.HasAVX512VAES
symbols, and replacing them withX86.GNFI
andX86.VAES
and new logic to ensure they are enabled when present in the absence of AVX512 (the old versions could be maintained with current semantics and deprecated). This could take a long time, and I think there's a non-zero chance they'll just punt and say it's good enough, which would be annoying, and forever require special case code to map between the proper naming for these things and the golang implementation.So this isn't really an issue in Avo, so much as an issue for Avo.
The text was updated successfully, but these errors were encountered: