search

klauspost / cpuid

CPU feature identification for Go
MIT License
1.04k stars 128 forks source link

Move x86 CPU features from Flags to a FlagSet #59

Closed mythi closed 3 years ago

mythi commented 4 years ago

This change deprecates CPU.Features due to a limitation of only 64 feature bits available when using Flags type (uint64).

The new method introduces a FlagSet with flag names as map keys. The set values are empty structs which, in Go, consume zero bytes.

The proposal is to start adding post-Icelake new features to FlagSet.

The new model makes it possible to combine Arm Flags too (flagNames and flagNamesArm are no longer needed)

TODO: more testing and discussion

/cc @askervin

askervin commented 4 years ago

Nice, @mythi! This would definitely help me adding new Sapphire Rapids feature flags, as they won't really go under AMXFeatures or any other single feature group.

So I think suggested giving up the public AMXFeatures field in the API for more future-proof solution (given that AMXFeatures haven't been released and there is no hardware out there yet) would be a good move. Then we wouldn't leave behind extra legacy to be deprecated.

The only thing I'm a bit worried is the use of the same flag name string without getting an error in case of mistyping it. What do you think if the FlagSet would be map[string]bool and initialized to false for all known flags? Then we could have runtime errors from functions inSet() and addSet() (new function, used instead of explicit flagset["..."] = struct{}{}) verifying correctness of the flag strings. Alternatively a new set of constants could make this a build-time failure, which would be even more desirable.

mythi commented 4 years ago

go test -tags=noasm ./... is failing and I'll fix it but in the mean time, how does v2 look like?

klauspost commented 4 years ago

Good. This will be the base of v2. When I get the time I will attempt to finish it up and release it.

mythi commented 3 years ago

Good. This will be the base of v2. When I get the time I will attempt to finish it up and release it.

Anything we could help with the release? @askervin created #60 on top of the commits in this PR so this needs to go in first.

askervin commented 3 years ago

Looks good, works fine!

marquiz commented 3 years ago

Looks good to me 👍

klauspost commented 3 years ago

@mythi If you are happy, I will merge it so we can get moving on a v2 release.

mythi commented 3 years ago

@klauspost I'm happy and it's good to go. I made some last minute changes this morning because the CI wasn't happy but it's happy now too ;-) @askervin and myself have been testing the latest changes and all work OK.