cpufetch does not accept pull requests, see the contributing guidelines for details

Thanks for the patch. Overall, looks good. I've added a few comments. Also, could you provide some screenshot to show how this looks on such old Intel/AMD CPUs? This will also be useful to better answer your questions:

1. Do you have a different way to better structure the list of inferred CPU names

Not right now, but because it's size is increasing I will probably rewrite it into a more structured way (similar to CHECK_UARCH but for CPU names). Feel free to do this if you want.

2. Do we need a backup way to determine cpu speed?

It would be good to have one if needed, but I would need to see some evidences. Do you have any CPU for which you cannot find the CPU frequency?

3. Should we hide SSE, Peak Performance, etc. line items if unknown?

Peak Performance no, for SSE maybe. Do you have any CPU in such scenario?

Before the last set of merges to master, I saw missing clock speeds a lot more often. Now I see empty SSE:

Before the last set of merges to master, I saw missing clock speeds a lot more often. Now I see empty SSE:

Regarding the frequency, I have implemented a new functionality recently that estimates the maximum frequency. That's why you have noticed this change in the recent commits.

Regarding SSE, I've pushed a fix (cb186a2), just rebase and confirm that it works as expected.

There is something important I forgot to mention in the last review.

Setting different strings for the same uarch, like in:

  CHECK_UARCH(arch, 0,  4,  0,  1, NA, "i80486DX-50",       UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  2, NA, "i80486SX",          UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  3, NA, "i80486DX2",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  4, NA, "i80486SL",          UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  5, NA, "i80486SX2",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  7, NA, "i80486DX2WB",       UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  8, NA, "i80486DX4",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  9, NA, "i80486DX4WB",       UARCH_I486,            UNK)

This is incorrect! All strings for a given uarch must be the same (in this case it should be something like i486 for all UARCH_I486, as this is the name of the microarchitecture). I understand that you want to differentiate between CPUs here, but this would be the CPU name, not the microarchitecture. The right thing to do is:

1. Save the CPUID value in struct cpuInfo.
2. Use these values in infer_cpu_name_from_uarch to determine the exact CPU name depending on those values (rather than incorrectly doing so in get_uarch_from_cpuid_intel).

You can implement this yourself, or just leave it like this and I'll implement it after merging your PR.

Another (optional) enhancement to your PR is to add support for detecting MMX.

As you showed, your CPU does not support SSE but I'll probably support MMX. You could take the AVX or SSE detection as a reference to implement this. After that, in your case it would show that MMX is supported for your CPU (assuming it does support it) and also the peak performance would be higher.

There is something important I forgot to mention in the last review.

Setting different strings for the same uarch, like in:

  CHECK_UARCH(arch, 0,  4,  0,  1, NA, "i80486DX-50",       UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  2, NA, "i80486SX",          UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  3, NA, "i80486DX2",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  4, NA, "i80486SL",          UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  5, NA, "i80486SX2",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  7, NA, "i80486DX2WB",       UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  8, NA, "i80486DX4",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  9, NA, "i80486DX4WB",       UARCH_I486,            UNK)

This is incorrect! All strings for a given uarch must be the same (in this case it should be something like i486 for all UARCH_I486, as this is the name of the microarchitecture). I understand that you want to differentiate between CPUs here, but this would be the CPU name, not the microarchitecture. The right thing to do is:

1. Save the CPUID value in `struct cpuInfo`.

2. Use these values in `infer_cpu_name_from_uarch` to determine the exact CPU name depending on those values (rather than incorrectly doing so in `get_uarch_from_cpuid_intel`).

You can implement this yourself, or just leave it like this and I'll implement it after merging your PR.

That does make sense.

So, there are three different things conflated in the current mapping

1. Microarchitecture name
2. More specific model name (486DX, 486SX, etc)
3. Core codename when different from the microarchitecture. name (Klamath, Deschutes, etc)

To keep the implementation purely for uarch, this is my thought:

1. Add a new function that takes cpuid values and uarch to potentially get a more detailed model name.
   For the current line CHECK_UARCH(arch, 0, 4, 0, 0, NA, "i80486DX", UARCH_I486, UNK), this new function would return 'DX', so the full string might be "Intel 486 DX".
2. Add an attribute for the CPU core codename, maybe "Codename" or "Core name".

Another (optional) enhancement to your PR is to add support for detecting MMX.

As you showed, your CPU does not support SSE but I'll probably support MMX. You could take the AVX or SSE detection as a reference to implement this. After that, in your case it would show that MMX is supported for your CPU (assuming it does support it) and also the peak performance would be higher.

Adding MMX detection does seem like a good idea. (Although the Pentium Pro does predate MMX). I think that should wait for a different PR.

Also, I did see that the SSE fix worked.

There is something important I forgot to mention in the last review.
Setting different strings for the same uarch, like in:

  CHECK_UARCH(arch, 0,  4,  0,  1, NA, "i80486DX-50",       UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  2, NA, "i80486SX",          UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  3, NA, "i80486DX2",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  4, NA, "i80486SL",          UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  5, NA, "i80486SX2",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  7, NA, "i80486DX2WB",       UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  8, NA, "i80486DX4",         UARCH_I486,            UNK)
  CHECK_UARCH(arch, 0,  4,  0,  9, NA, "i80486DX4WB",       UARCH_I486,            UNK)

This is incorrect! All strings for a given uarch must be the same (in this case it should be something like i486 for all UARCH_I486, as this is the name of the microarchitecture). I understand that you want to differentiate between CPUs here, but this would be the CPU name, not the microarchitecture. The right thing to do is:

1. Save the CPUID value in `struct cpuInfo`.

2. Use these values in `infer_cpu_name_from_uarch` to determine the exact CPU name depending on those values (rather than incorrectly doing so in `get_uarch_from_cpuid_intel`).

You can implement this yourself, or just leave it like this and I'll implement it after merging your PR.

That does make sense.

So, there are three different things conflated in the current mapping

1. Microarchitecture name
2. More specific model name (486DX, 486SX, etc)
3. Core codename when different from the microarchitecture. name (Klamath, Deschutes, etc)

To keep the implementation purely for uarch, this is my thought:

1. Add a new function that takes cpuid values and uarch to potentially get a more detailed model name.
   For the current line CHECK_UARCH(arch, 0, 4, 0, 0, NA, "i80486DX", UARCH_I486, UNK), this new function would return 'DX', so the full string might be "Intel 486 DX".

If prefer not to use the uarch name directly for the CPU name. What I mean is that I would rather return "Intel 486 DX" instead of "DX" and then append that to the "Intel 486". Anyway, I will implement this after your PR lands.

2. Add an attribute for the CPU core codename, maybe "Codename" or "Core name".

This is actually a very good point. I want to implement this since 2021! (see #120) but not much time to spend on this.

Another (optional) enhancement to your PR is to add support for detecting MMX.
As you showed, your CPU does not support SSE but I'll probably support MMX. You could take the AVX or SSE detection as a reference to implement this. After that, in your case it would show that MMX is supported for your CPU (assuming it does support it) and also the peak performance would be higher.

Adding MMX detection does seem like a good idea. (Although the Pentium Pro does predate MMX). I think that should wait for a different PR.

Agreed, can be good for a different PR if you have a good testcase (a CPU that supports MMX but not newer instructions like SSE). Right now this PR looks good to me. If you can confirm that this works for your hardware I'll merge into master soon 👍

For the CPU identification, everything looks good to me.

The only minor issue (which we can figure out later) is that getting a clock speed for these old CPUs is tricky:

• For 486 chips, there's no timestamp counter, nor a speed reference from the linux kernel.
• It looks like /proc/cpuinfo could be a good fallback. It might even be more accurate than the current fallback, which always seems about 20MHz slow on the Pentium Pro system

486 Socket

Pentium Pro

AMD K5

Okay, I'm ready to merge. Just wanted to confirm if you are happy with this?

Sorry for the big delay. Sometimes it's dificult to find time for this. About the frequency, I don't think there is much to do. I'm afraid that without perf_event_open the accuracy would be poor. We could rely on /proc/cpuinfo however, to read the frequency, that's a good point. I'll schedule some time to do some test to see if this method is reliable 👍

Yeah, I'm good to merge. Just please update the email address to [email protected]

Merged! Thank you for your contribution and looking forward for the next 👍