Skip to content
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

Fix various problems with architecture version handling #987

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

serge-sans-paille
Copy link
Contributor

Some architectures were not registered in the xsimd::all_architectures arch list.

The test for issorted was incorrect.

Some architectures add wrong version values but this went unnoticed because of the invalid check.

@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch from a3ddd47 to 254a839 Compare December 5, 2023 15:51
@serge-sans-paille
Copy link
Contributor Author

@JohanMabille : I think I unveiled a serious issue there: due to bad ordering of the version, wasm was never selected as a the default arch when available, which in turns nullify most of its test (intel arch was tested instead!)

It's good that I caught this, but it's going to have some impact...

@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch 3 times, most recently from 4df0d0e to 5660ec3 Compare December 5, 2023 18:11
@JohanMabille
Copy link
Member

dawson

@JohanMabille
Copy link
Member

JohanMabille commented Dec 5, 2023

I think we don't have fallback implementation for all operations, therefore we cannot add new instruction sets and registers if we don't implement a minimal subset.

What I don't understand is that when Anutosh was implementing the support for WASM, the build was failing until he had implemented the minimal subset; so WASM was selected at some point. Also this explains why removing the workaround Thorsten implemented for arg / conj did not make the tests fail: at this point, we were in this configuration, where WASM was not selected anymore.

@anutosh491
Copy link
Contributor

anutosh491 commented Dec 6, 2023

Yes I believe there was a minimalistic subset of operations. After I had implemented all of those, the build stopped failing.

@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch 3 times, most recently from 6aa4137 to b3fec3f Compare December 6, 2023 14:02
Some architectures were not registered in the `xsimd::all_architectures`
arch list.

The test for issorted was incorrect.

Some architectures add wrong version values but this went unnoticed
because of the invalid check.
@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch from 3280390 to 9d2860e Compare December 6, 2023 17:13
@SylvainCorlay
Copy link
Member

Thanks @serge-sans-paille! It is looking like it is going to be in working order with wasm.

@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch from 9d2860e to a3d08da Compare December 6, 2023 18:41
@serge-sans-paille
Copy link
Contributor Author

@SylvainCorlay yep, wasm fixed, I'm handling the final touches and this should be mergeable. @JohanMabille this deserves a minor release (patch + new features)

@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch from a3d08da to 917d690 Compare December 6, 2023 19:43
@serge-sans-paille serge-sans-paille force-pushed the feature/fix-version-values branch from 917d690 to c153824 Compare December 6, 2023 20:22
The actual hierarchy is

avx512-vbmi
avx512-ifma
avx512-bw
avx512-dq
avx512-vl
avx512-f
avx512-cd

for cannonlake <- skylake

and

avx512-er
avx512-pf
avx512-f
avx512-cd

for kights landing
@JohanMabille JohanMabille merged commit c5c2101 into master Dec 7, 2023
108 checks passed
@JohanMabille JohanMabille deleted the feature/fix-version-values branch December 7, 2023 05:03
@serge-sans-paille
Copy link
Contributor Author

\o/ hurray, that one was a tough one!

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

Successfully merging this pull request may close these issues.

4 participants