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

Add framework for implementation of support for AVX512VNNI #3894

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

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Aug 6, 2022

Using AVX512VNNI instructions is still missing.

@stweil stweil marked this pull request as draft August 6, 2022 17:10
@@ -170,6 +170,14 @@ libtesseract_la_LIBADD += libtesseract_avx512.la
noinst_LTLIBRARIES += libtesseract_avx512.la
endif

if HAVE_AVX512VNNI
libtesseract_avx512vnni_la_CXXFLAGS = -mavx512vnni -mavx512vl
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious: what is the use of -mavx512vl? Should it be checked in configure, too?

Copy link
Collaborator

@amitdo amitdo Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not compile without the second flag. It is needed because the code uses ymm registers (256 bit) instead of zmm registers (512 bit).

Yes, we can also add another check in configure.

Note that AFAIK all Intel's CPUs that support AVX512VNNI also support AVXVL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. Maybe we don't need the additional check then.

@amitdo
Copy link
Collaborator

amitdo commented Aug 9, 2022

Ready for testing.

I suggest to backup the home directory and any other important stuff, in case this patch will trigger a hard crash (reboot).

@stweil
Copy link
Contributor Author

stweil commented Aug 9, 2022

I suggest to backup the home directory and any other important stuff, in case this patch will trigger a hard crash (reboot).

Will it? I cannot test it before August 21 because I currently don't have access to the required hardware.

@amitdo
Copy link
Collaborator

amitdo commented Aug 9, 2022

Will it?

I don't think so...

@amitdo
Copy link
Collaborator

amitdo commented Aug 21, 2022

Stefan, will you able to test the code today or in the upcoming days?

@stweil
Copy link
Contributor Author

stweil commented Aug 21, 2022

I just finished a first run of intsimdmatrix_test with the new code. It works, but I was surprised because the performance is identical to the AVX2 implementation.

@stweil
Copy link
Contributor Author

stweil commented Aug 21, 2022

Support for sw build is still missing. Therefore CI currently fails.

@amitdo
Copy link
Collaborator

amitdo commented Aug 21, 2022

I was surprised because the performance is identical to the AVX2 implementation.

This is disappointing.

We can also remove the 'ones' vector. The vnni version does not need it.

@amitdo
Copy link
Collaborator

amitdo commented Aug 21, 2022

Did you fetch and merge my commit locally?

git switch avx512-vnni
git log -1

Do you see my commit?

@stweil
Copy link
Contributor Author

stweil commented Aug 22, 2022

We can also remove the 'ones' vector. The vnni version does not need it.

I removed it locally. But that did not change the generated code. Obviously the compiler also removed it.

stweil and others added 4 commits November 25, 2022 10:18
Signed-off-by: Stefan Weil <sw@weilnetz.de>
This dummy implementation just copied the code from AVX2.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
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.

None yet

2 participants