-
Notifications
You must be signed in to change notification settings - Fork 308
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 detection for Intel Advanced Matrix Extensions (AMX) instructions #231
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation looks problematic (guess tab vs. space). Others LGTM.
988603a
to
94a969a
Compare
Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but let's add a separator for those names to match how it's done for AVX512.
Another question: are there CPUs on the market that say has fp16 but not int8 AMX support?
Would be good to add a much more details description with links back to the docs explaning what those extensions do and what CPUs support them
94a969a
to
aceeff6
Compare
aceeff6
to
f738510
Compare
Currently we do not have platforms that supports
@malfet If you find a better place to put this note, please let me know! |
@@ -812,6 +812,10 @@ struct cpuinfo_x86_isa { | |||
bool avx512vp2intersect; | |||
bool avx512_4vnniw; | |||
bool avx512_4fmaps; | |||
bool amx_bf16; | |||
bool amx_tile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove amx_tile?
is tile useful? all cpus that support amx_bf16 or amx_int8 will support amx_tile, and amd_tile by itself is not useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we keep these flags at the low level that are mapped exactly to underlying CPU ISA feature bits. We can probably have some helper functions like has_amx_support
at the higher level for ease of use purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true that the existing platforms that supports amx_bf16 or amx_int8 will support amx_tile. However I prefer to leave a low level flag here, just in case of possible future changes.
* AMX_TILE instructions: | ||
* - Intel: edx[bit 24] in structured feature info (ecx = 0). | ||
*/ | ||
isa.amx_tile = avx512_regs && !!(structured_feature_info0.edx & UINT32_C(0x01000000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you confirm this works on gnr256 with avx10 but not avx512?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to this:
Granite Rapids
AMXBF16: yes
AMXTILE: yes
AMXINT8: yes
AMXFP16: yes
Granite Rapids (AVX10.1 / 256VL)
AMXBF16: yes
AMXTILE: yes
AMXINT8: yes
AMXFP16: yes
Results collected with intel Software Development Emulator
quote from https://www.tomshardware.com/news/intels-new-avx10-brings-avx-512-capabilities-to-e-cores
Intel will support AVX10 version 1 (AVX10.1) beginning with its sixth-gen Xeon "Granite Rapids" chips, but that generation will only support 512-bit vector instructions, and not the new converged 256-bit vector instructions. Instead, this first gen will serve as the transition chip from AVX-512 to AVX10.
Tested using intel SDE: https://www.intel.com/content/www/us/en/download/684897/intel-software-development-emulator.html
Test scripts:
Results: