Add proper CPUID eax checking #3026

isuruf · 2024-12-17T15:34:32Z

This makes it possible to use the sse42, avx2 code paths on AMD processors

Description

Add a comprehensive description of proposed changes

Adds proper CPUID extension support checking which was previously done by guarding with a check for Intel CPUs. This allows AMD processors to use the sse42, avx2 code paths instead of the default sse2.

List associated issue number(s) if exist(s): #6 (for example)

Documentation PR (if needed): #1340 (for example)

Benchmarks PR (if needed): IntelPython/scikit-learn_bench#155 (for example)

PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

I have reviewed my changes thoroughly before submitting this pull request.
I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
I have added a respective label(s) to PR if I have a permission for that.
I have resolved any merge conflicts that might occur with the base branch.

Testing

I have run it locally and tested the changes extensively.
All CI jobs are green or I have provided justification why they aren't.
I have extended testing suite if new functionality was introduced in this PR.

Performance

I have measured performance for affected algorithms using scikit-learn_bench and provided at least summary table with measured data, if performance change is expected.
I have provided justification why performance has changed or why changes are not expected.
I have provided justification why quality metrics have changed or why changes are not expected.
I have extended benchmarking suite and provided corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

icfaust

Initial review.

icfaust · 2024-12-19T06:25:18Z

cpp/daal/src/services/compiler/generic/env_detect_features.cpp

@@ -71,6 +71,19 @@ void run_cpuid(uint32_t eax, uint32_t ecx, uint32_t * abcd)
    #endif
 }

+uint32_t __daal_internal_get_max_extension_support()


This function is not necessary, it can be folded in below.

This follows the pattern of __daal_internal_is_intel_cpu and daal_check_is_intel_cpu where the former is the one that does the work and the latter is there to cache the value in a static variable to avoid running the former multiple times.

icfaust · 2024-12-19T06:25:29Z

cpp/daal/src/services/compiler/generic/env_detect_features.cpp

@@ -71,6 +71,19 @@ void run_cpuid(uint32_t eax, uint32_t ecx, uint32_t * abcd)
    #endif
 }

+uint32_t __daal_internal_get_max_extension_support()
+{
+    uint32_t abcd[4];


clearer naming is necessary.

The naming used is abcd throughout this file. This variable stores the values of the eax, ebx, ecx, edx registers just like the other functions. Any ideas on what to rename all of them to?

cpp/daal/src/services/compiler/generic/env_detect_features.cpp

icfaust · 2024-12-19T06:26:46Z

cpp/daal/src/services/compiler/generic/env_detect_features.cpp

@@ -193,7 +211,7 @@ static int check_sse42_features()

 DAAL_EXPORT bool __daal_serv_cpu_extensions_available()
 {
-    return daal_check_is_intel_cpu();
+    return 1;


First, if a bool, it would be best to return a bool. Secondly, if this is a no-op, then the function should be removed.

I removed the function and its usages.

isuruf · 2024-12-19T09:22:44Z

Thanks for the review @icfaust

isuruf · 2025-01-06T03:53:26Z

@icfaust what are the next steps for this PR?

icfaust · 2025-01-06T13:56:06Z

@isuruf I think addressing uxlfoundation/scikit-learn-intelex#1000 may take priority, though I will defer to code owners on this. I think testability is key, and this PR may cover up an underlying issue.

isuruf · 2025-01-07T06:29:08Z

I think testability is key, and this PR may cover up an underlying issue.

Yeah, it will cover up an underlying issue in sse2 code path, but those are really old hardware and it seems like the only way to make that bug surface is to use/emulate really old hardware or use AMD hardware. I don't think it is fair to keep AMD hardware throttled to sse2 just to have a way to reproduce this bug.

isuruf · 2025-01-13T09:08:02Z

Ping on this

napetrov · 2025-01-29T22:19:31Z

/intelci: run

napetrov · 2025-01-31T17:10:42Z

I think testability is key, and this PR may cover up an underlying issue.

Yeah, it will cover up an underlying issue in sse2 code path, but those are really old hardware and it seems like the only way to make that bug surface is to use/emulate really old hardware or use AMD hardware. I don't think it is fair to keep AMD hardware throttled to sse2 just to have a way to reproduce this bug.

I think those can be separated. One is changing detection of instructions which is ok. There are several reasons why this wasn't touched from the beginning - there is no AMD specific validation or maintainers for it with responsibility over those. It's simple with this change and i wouldn't expect issues there, but there are other things that are enabled specifically on Intel just to reduce potential problems.

On sse2 problem - i think it might be not specific to sse2 as there is no specialization there and this is what get compiled by default. though it was not reported for other platforms.

cpp/daal/src/services/service_defines.h

napetrov · 2025-02-06T20:57:47Z

/intelci: run

napetrov · 2025-02-26T19:30:20Z

/intelci: run

Vika-F · 2025-02-27T09:04:53Z

@isuruf Please rebase the branch. It looks like the tests are failing in pre-commit because some fixes are missing in the branch.

This makes it possible to use the sse42, avx2 code paths on AMD processors

mergify · 2025-02-27T15:35:38Z

rebase

✅ Branch has been successfully rebased

napetrov · 2025-02-27T15:36:42Z

@Mergifyio rebase

mergify · 2025-02-27T15:36:53Z

rebase

✅ Nothing to do for rebase action

napetrov · 2025-02-27T15:37:55Z

/intelci: run

isuruf · 2025-02-28T18:07:50Z

Thanks everyone for the review and merge.

napetrov · 2025-02-28T18:27:53Z

Thanks everyone for the review and merge.

thanks for contribution!

isuruf force-pushed the cpuid branch from 48107e6 to e4ddec6 Compare December 18, 2024 02:43

isuruf mentioned this pull request Dec 18, 2024

Predict and Scoring Bug in sklearnex.svm.SVR and Train_test_split index bug uxlfoundation/scikit-learn-intelex#1000

Open

napetrov requested review from ahuber21, Vika-F and ethanglaser December 18, 2024 20:49

icfaust reviewed Dec 19, 2024

View reviewed changes

napetrov marked this pull request as ready for review January 29, 2025 22:32

napetrov requested review from Alexsandruss, samir-nasibli and Alexandr-Solovev as code owners January 29, 2025 22:32

Vika-F reviewed Feb 3, 2025

View reviewed changes

cpp/daal/src/services/service_defines.h Outdated Show resolved Hide resolved

Vika-F added the enhancement label Feb 7, 2025

isuruf and others added 3 commits February 27, 2025 15:35

Add proper CPUID eax checking

2d618d1

This makes it possible to use the sse42, avx2 code paths on AMD processors

remove __daal_serv_cpu_extensions_available and add comments

5c97022

revert DAAL_CHECK_CPU_ENVIRONMENT changes

eb1c2e9

napetrov force-pushed the cpuid branch from b867cfc to eb1c2e9 Compare February 27, 2025 15:35

Vika-F approved these changes Feb 28, 2025

View reviewed changes

napetrov approved these changes Feb 28, 2025

View reviewed changes

napetrov merged commit 7884821 into uxlfoundation:main Feb 28, 2025
17 of 22 checks passed

isuruf deleted the cpuid branch February 28, 2025 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proper CPUID eax checking #3026

Add proper CPUID eax checking #3026

isuruf commented Dec 17, 2024 •

edited by napetrov

Loading

icfaust left a comment

icfaust Dec 19, 2024

isuruf Dec 19, 2024

icfaust Dec 19, 2024

isuruf Dec 19, 2024

icfaust Dec 19, 2024

isuruf Dec 19, 2024

isuruf commented Dec 19, 2024

isuruf commented Jan 6, 2025

icfaust commented Jan 6, 2025

isuruf commented Jan 7, 2025

isuruf commented Jan 13, 2025

napetrov commented Jan 29, 2025

napetrov commented Jan 31, 2025

napetrov commented Feb 6, 2025

napetrov commented Feb 26, 2025

Vika-F commented Feb 27, 2025

mergify bot commented Feb 27, 2025

napetrov commented Feb 27, 2025

mergify bot commented Feb 27, 2025

napetrov commented Feb 27, 2025

isuruf commented Feb 28, 2025

napetrov commented Feb 28, 2025

Add proper CPUID eax checking #3026

Add proper CPUID eax checking #3026

Conversation

isuruf commented Dec 17, 2024 • edited by napetrov Loading

Description

icfaust left a comment

Choose a reason for hiding this comment

icfaust Dec 19, 2024

Choose a reason for hiding this comment

isuruf Dec 19, 2024

Choose a reason for hiding this comment

icfaust Dec 19, 2024

Choose a reason for hiding this comment

isuruf Dec 19, 2024

Choose a reason for hiding this comment

icfaust Dec 19, 2024

Choose a reason for hiding this comment

isuruf Dec 19, 2024

Choose a reason for hiding this comment

isuruf commented Dec 19, 2024

isuruf commented Jan 6, 2025

icfaust commented Jan 6, 2025

isuruf commented Jan 7, 2025

isuruf commented Jan 13, 2025

napetrov commented Jan 29, 2025

napetrov commented Jan 31, 2025

napetrov commented Feb 6, 2025

napetrov commented Feb 26, 2025

Vika-F commented Feb 27, 2025

mergify bot commented Feb 27, 2025

✅ Branch has been successfully rebased

napetrov commented Feb 27, 2025

mergify bot commented Feb 27, 2025

✅ Nothing to do for rebase action

napetrov commented Feb 27, 2025

isuruf commented Feb 28, 2025

napetrov commented Feb 28, 2025

isuruf commented Dec 17, 2024 •

edited by napetrov

Loading