Skip to content

[compiler-rt] Add infrastructure for testing cpuid builtins #101927

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

boomanaiden154
Copy link
Contributor

This patch adds infrastructure in compiler-rt to test cpuid related builtins (like __builtin_cpu_supports and __builtin_cpu_is) on X86.

This patch adds infrastructure in compiler-rt to test cpuid related
builtins (like __builtin_cpu_supports and __builtin_cpu_is) on X86.
Copy link

github-actions bot commented Aug 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@boomanaiden154
Copy link
Contributor Author

Bump on this when you get a chance @petrhosek. Thanks!

@boomanaiden154
Copy link
Contributor Author

@petrhosek bump on this when you get a chance. Thanks!

@boomanaiden154 boomanaiden154 changed the title Add infrastructure for testing cpuid builtins [compiler-rt] Add infrastructure for testing cpuid builtins Oct 21, 2024
@boomanaiden154
Copy link
Contributor Author

Bump on this when you have a chance @petrhosek.

@phoebewang
Copy link
Contributor

Using OverrideCPUID doesn't look an ideal solution to me. It cannot prevent mistakes from author who wrote both code and test. It also cannot reflect any HW changes or mismatch. I'd like to see run time test from llvm-test-suite. We can leverage SDE to emulate different Intel CPUs, but I don't know how to do with other vendors.

@boomanaiden154
Copy link
Contributor Author

Using OverrideCPUID doesn't look an ideal solution to me. It cannot prevent mistakes from author who wrote both code and test.

Right. It can't catch everything, but it should be an improvement over what we have currently (no testing). I'm also not sure there's anything that is clearly better and works across vendors. My plan for this was ideally to take raw cpuid values from real hardware and then assert that we get the expected features and identify the appropriate CPU. That doesn't make it foolproof, but should be a reasonable enough approach.

It also cannot reflect any HW changes or mismatch.

Can you explicate on the specific scenarios you're describing here?

I'd like to see run time test from llvm-test-suite. We can leverage SDE to emulate different Intel CPUs, but I don't know how to do with other vendors.

I would think using SDE would require adding it as a dependency to llvm-test-suite for running these tests, which I don't think would be trivial, and would significantly limit the total number of machines that run these tests. I'd also like to test AMD CPU values as well.

I think we could write a hook that uses ptrace as I believe the rr folks added some kernel support to trap around cpuid instructions, but that would put us back in the same spot as your first comment, and at that point I think unittests are easier to write and maintain.

@phoebewang
Copy link
Contributor

It also cannot reflect any HW changes or mismatch.

Can you explicate on the specific scenarios you're describing here?

It's unlike to happen. I imagine the HW spec might be updated after it's disclosed and compiler may fail to catch it.

I would think using SDE would require adding it as a dependency to llvm-test-suite for running these tests, which I don't think would be trivial, and would significantly limit the total number of machines that run these tests.

There was a bot run with SDE, but seems removed from llvm-zorg now.

@boomanaiden154
Copy link
Contributor Author

It's unlike to happen. I imagine the HW spec might be updated after it's disclosed and compiler may fail to catch it.

Makes sense. Either way, seems like something that would be hard to detect even with decent testing?

There was a bot run with SDE, but seems removed from llvm-zorg now.

Yeah, this doesn't seem too uncommon for some of the bots with specific configurations, hence my concern about limiting the tests by requiring something like SDE.

Are we able to move forward with this given testing a specific processor's CPU values and asserting microarch/features based on manufacturer documentation should alleviate concerns regarding tests being incorrectly based on the implementation?

@phoebewang
Copy link
Contributor

It's unlike to happen. I imagine the HW spec might be updated after it's disclosed and compiler may fail to catch it.

Makes sense. Either way, seems like something that would be hard to detect even with decent testing?

There was a bot run with SDE, but seems removed from llvm-zorg now.

Yeah, this doesn't seem too uncommon for some of the bots with specific configurations, hence my concern about limiting the tests by requiring something like SDE.

Are we able to move forward with this given testing a specific processor's CPU values and asserting microarch/features based on manufacturer documentation should alleviate concerns regarding tests being incorrectly based on the implementation?

I agree the dependency to extra tool is not great, but the method here doesn't look much value to me. In my opinion, a valuable test should serve one or another of below functions:

  • Cross verification. The representation is llvm-test-suite, hence the SDE proposal;
  • Regression test. The representation is the lit tests, but a CPU model info nearly never regresses. So we shouldn't bother to pursuit this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants