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

Rework of x86 intrinsics support #3

Closed
wants to merge 14 commits into from

Conversation

andreittr
Copy link
Contributor

Massive rework of the x86 intrinsics library, featuring:

  • update baseline to upstream LLVM 14
  • proper support of all x86 intrinsics (depending on build flags), not only a minimal subset
  • delegate -march= compile flags to main build system; do not interfere
  • split off llvm & gcc trees; keeping a single compatible set of files turned out unmaintainable
  • gcc headers based off GCC 13
  • compatibility patches for other clang & gcc versions
  • tested w/ clang 11, 12, 13, 14, 15, 16; gcc 11, 12, 13

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change removes the Kconfig options selecting CPU feature set (i.e.,
`-march=` compiler flag) from the configuration of this library.
CPU feature flags are set elsewhere in the build configuration and the
intrinsics library can already seamlessly pick up these features via
preprocessor defines.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change removes code changes meant for compatibility with GCC,
reverting the headers to ones released with LLVM 7.0.1.
This is in anticipation of an update of upstream code, as well as a
rework of compiler compatibility.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
In addition, this change greatly expands the available feature headers.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in LLVM 15.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in LLVM 16.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in LLVM 13.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change makes explicit that this library only supports clang by
renaming the include directory and only adding it to the include path if
compiling with clang.
A future change may thus explicitly add GCC-compatible headers.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change adds on-par support for GCC by providing the native x86
intrinsics headers from gcc release 13.1.0.
The file `mm_malloc.h` is taken from upstream `pmm_malloc.h`.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in GCC release 12.3.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in GCC release 11.4.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in LLVM 12.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Implementations based off headers in LLVM 11.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@razvand razvand self-assigned this Aug 5, 2023
@razvand razvand added the enhancement New feature or request label Aug 5, 2023
Copy link

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Works well, thanks @andreittr! Tested the changes with gcc-11 and clang-14 and it performed just fine.

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Reformat to Markdown.
CONTRIBUTING: remove obsolete references, point users at website
README: add a short description

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

I tried running samples from here, but for some reason, they don't compile for Unikraft, the issue being

cc1: error: attribute ‘mwait’ argument ‘target’ is unknown` . 

Do you know what may be the issue for that?

menuconfig LIBINTEL_INTRINSICS
bool "Intel Intrinsics - C style functions that provide access Intel instructions"
config LIBINTEL_INTRINSICS
bool "Intel Intrinsics - C style functions that provide access to x86 instructions"

Choose a reason for hiding this comment

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

Suggested change
bool "Intel Intrinsics - C style functions that provide access to x86 instructions"
bool "Intel Intrinsics: C style functions that provide access to x86 instructions"

Choose a reason for hiding this comment

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

I also think the string before : should start with a lowercase letter, for example musl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: - vs :, there's not really any consistency between libs; musl and builtins are :, while libcxx*, libunwind, compiler-rt, newlib, bzip, lwip, and others are -. I'm not in favour of picking a particular side here unless we agree for a style project-wide, and until then I err on the side of least change.

Re: capitalization, I'd rather use the project's preferred style, caps & all, since that will be the most familiar for a user, e.g., musl, Python, Intel Intrinsics; they spell it like this too (modulo the registered trademark).

A list of these intrinsics can be browsed at [Intel Intrinsics](https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html).
Normally you would receive these headers together with your compiler, however, due to Unikraft's isolated build environment, we provide them explicitly.

Please refer to `README.md` in the main unikraft repository as well as the [Unikraft Homepage](https://unikraft.org/) for further information.

Choose a reason for hiding this comment

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

Suggested change
Please refer to `README.md` in the main unikraft repository as well as the [Unikraft Homepage](https://unikraft.org/) for further information.
Please refer to `README.md` in the main unikraft repository as well as the [Unikraft Homepage](https://unikraft.org/) for further information.

Please provide a link to the README.md file

Choose a reason for hiding this comment

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

I don't think that's necessary @RaduNichita. Check the other libs, neither of them links the core Unikraft README which is also easily available. Consistency between repos is key.

Choose a reason for hiding this comment

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

OK, fair point here

@mariasfiraiala
Copy link

@RaduNichita @andreittr I'd leave the discussion regarding capitalization and so on for a later time. If the functionality is fine, I'd say we should aim to merge this as part of the 0.14 release.

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

OK, so I think we can approve this as it is (as it is necessarily for the Numpy Library) and work in the future in integrating those samples.

I will open an issue on that after this PR is merged.

Reviewed-by: Radu Nichita radunichita99@gmail.com

Copy link

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
This change removes the Kconfig options selecting CPU feature set (i.e.,
`-march=` compiler flag) from the configuration of this library.
CPU feature flags are set elsewhere in the build configuration and the
intrinsics library can already seamlessly pick up these features via
preprocessor defines.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
This change removes code changes meant for compatibility with GCC,
reverting the headers to ones released with LLVM 7.0.1.
This is in anticipation of an update of upstream code, as well as a
rework of compiler compatibility.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
In addition, this change greatly expands the available feature headers.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in LLVM 15.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in LLVM 16.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in LLVM 13.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
This change makes explicit that this library only supports clang by
renaming the include directory and only adding it to the include path if
compiling with clang.
A future change may thus explicitly add GCC-compatible headers.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
This change adds on-par support for GCC by providing the native x86
intrinsics headers from gcc release 13.1.0.
The file `mm_malloc.h` is taken from upstream `pmm_malloc.h`.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in GCC release 12.3.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in GCC release 11.4.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in LLVM 12.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Implementations based off headers in LLVM 11.0.0.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Reformat to Markdown.
CONTRIBUTING: remove obsolete references, point users at website
README: add a short description

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #3
@andreittr andreittr deleted the ttr/llvm14gcc13 branch August 10, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants