anrieff / libcpuid

a small C library for x86 CPU detection and feature extraction
Other
451 stars 106 forks source link

Update CPUs database #118

Closed TheTumultuousUnicornOfDarkness closed 5 years ago

TheTumultuousUnicornOfDarkness commented 5 years ago

Hello,

Some new processors, both on AMD and Intel side. No new test are available, but no regression in existing tests.

:question: According to WikiChip, EPYC CPUs (codename Naples) are not (yet) recognized. Wouldn't it better to detect Zen CPUs by using model_bits field instead of model_code field, like Intel's Core CPUs? I'm not sure about _amd_model_codes_t purpose. It can introduce mistakes (like ee32a4a7357083e7e9e47bd3ef52937f5d984453).

anrieff commented 5 years ago

Hi @X0rg , again thanks for your excellent contributions. Re model_bits vs model_code: the code is really a last-means resort to pick up some specific detail from the brand string in order to differentiate two very similar CPUs that share the same family/model/stepping and where an useful model bit to differentiate them is not available (or too specific, e.g. applies for this family only).

I think the example that prompted adding the model code were the "Xeon (Wolfdale)" and "Xeon (Harpertown)". The model numbers in this case are very specific to Core-based Xeons and aren't useful to be encoded as bits. The number of bits has to be kept low too, you can't have more than 64 of them.

As you can see model codes aren't used anywhere else in the Intel table, as the model code is really some sort of a last-resort hack, and should generally be avoided.

I'm thinking about something that could be done for the AMD table, and possibly for the Intel table too. How about instead of an integer model_code field in the match_entry_t, we have a const char*, so the model codes are mostly "", or in the case of one of the Ryzens, use "2500" instead of _2500? The model code string may be matched against the brand string from raw_data_t via match_pattern. The AMD table can be converted trivially, the Intel will be more complicated as the get_model_code() is quite convoluted, but it's likely doable and the models are well covered with tests.

What do you think about it?

I'm otherwise accepting the PR, but we can discuss the suggestion here.

TheTumultuousUnicornOfDarkness commented 5 years ago

Ok, I understand. So we must leave model_code untouched for Intel table.

My question is more about AMD table, and particularly Ryzen section. Look:

{ 15, -1, -1, 23,   17,   4,    -1,    -1, NC, 0, _2800, "Ryzen 7 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,   4,    -1,    -1, NC, 0, _2700, "Ryzen 7 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,   4,    -1,    -1, NC, 0, _2600, "Ryzen 5 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,   4,    -1,    -1, NC, 0, _2500, "Ryzen 5 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,   4,    -1,    -1, NC, 0, _2400, "Ryzen 5 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,   4,    -1,    -1, NC, 0,     0, "Ryzen 3 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,   2,    -1,    -1, NC, 0, _2200, "Ryzen 3 (Raven Ridge)" };

That's one line per model (too much duplicate for me).

In my opinion, it can be done easier, like this (similar to Intel table):

{ 15, -1, -1, 23,   17,  -1,    -1,    -1, NC, RYZEN_|_7, 0, "Ryzen 7 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,  -1,    -1,    -1, NC, RYZEN_|_5, 0, "Ryzen 5 (Raven Ridge)" };
{ 15, -1, -1, 23,   17,  -1,    -1,    -1, NC, RYZEN_|_3, 0, "Ryzen 3 (Raven Ridge)" };

It uses model_bits instead of model_code, and there is not need to set ncores.

It's to simplify how Ryzen CPUs are detected, just like you did with Intel Core ix CPUs (e.g. CORE_|_I_|_7). No need to change match_entry_t at all. :wink:

Otherwise, the char name[32] field is not coherent: Sometimes it's Model (Architecture):

Other times, Architecture (Model):

Or just Architecture:

Of course, it's a detail. Just a remark.

anrieff commented 5 years ago

In my opinion, it can be done easier, like this (similar to Intel table):

of course, that is exactly what the bits are meant for. Go for it :)