rust-lang / stdarch

Rust's standard library vendor-specific APIs and run-time feature detection
https://doc.rust-lang.org/stable/core/arch/
Apache License 2.0
605 stars 267 forks source link

initial commit to enable amx #1618

Closed ziyizhang-1 closed 2 months ago

ziyizhang-1 commented 2 months ago

As mentioned in x86_amx_intrinsics

Completed AMX Intrinsics: amx-tile

rustbot commented 2 months ago

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

sayantn commented 2 months ago

Can you please run cargo test -p stdarch-verify with the PRINT_MISSING_LISTS_MARKDOWN constant set to true? This would update the missing-x86.md file to reflect the new added intrinsics. Also, please do not edit the x86-intel.xml file manually. Download the latest from Intel's website (I think v3.6.9 currently) and replace this.

Also, there are very few amx intrinsics, so they can be implemented in a single amx.rs file instead of 5 files

sayantn commented 2 months ago

Even though windows doesn't need the syscall, you still need to have the function to compile. Maybe add an empty init_amx function with #[cfg(all(target_arch = "x86_64", windows))]

ziyizhang-1 commented 2 months ago

Even though windows doesn't need the syscall, you still need to have the function to compile. Maybe add an empty init_amx function with #[cfg(all(target_arch = "x86_64", windows))]

yep, that's a good workaround to prevent repeated condition check

ziyizhang-1 commented 2 months ago

Can you please run cargo test -p stdarch-verify with the PRINT_MISSING_LISTS_MARKDOWN constant set to true? This would update the missing-x86.md file to reflect the new added intrinsics. Also, please do not edit the x86-intel.xml file manually. Download the latest from Intel's website (I think v3.6.9 currently) and replace this.

Also, there are very few amx intrinsics, so they can be implemented in a single amx.rs file instead of 5 files

updated x86-intel.xml to v3.6.9, please help to review the change

sayantn commented 2 months ago

The x86_64-unknown-linux-gnu-emulated CI runs all the tests (using Intel SDE), so your implementation amx-complex and amx-fp16 has been tested.

Also, maybe squash all these small commits into one single commit, just for the sake of cleanliness

bors commented 2 months ago

:umbrella: The latest upstream changes (presumably fb90dfa348265f5039dc7dbba24ee9adf6046507) made this pull request unmergeable. Please resolve the merge conflicts.

sayantn commented 2 months ago

@Amanieu should we keep the __tilecfg type? It indeed makes for a nice abstraction (all C programs that use amx have to define a struct or use an array because C lacks this type)

Amanieu commented 2 months ago

I don't think we should be inventing our own API in this crate, we should strictly follow the vendor's API. What if Intel decides to add its own __tilecfg type in the future that is incompatible with ours? I think it is best to leave this to an external crate. That crate can also deal with other AMX-related issues such as the syscalls needed for initialization.

ziyizhang-1 commented 2 months ago

I understood the concern raised by @Amanieu; there is indeed some probability that Intel will change the definition of the tile configuration in the future, for example, increasing/decreasing the number of tiles, which can lead to a change of the config layout. Anyway, I just moved __tilecfg to the tests mod, and considering the current feature x86_amx_intrinsics is still unstable, any different points of view can be welcomed and discussed.

Amanieu commented 2 months ago

Everything else looks good, so this can be merged once the i32 issue is fixed.