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

[OpenSSL] Enabling Arm64 assembly on Windows by using clang #34711

Conversation

eukarpov
Copy link
Contributor

@eukarpov eukarpov commented Oct 24, 2023

Improving OpenSSL performance on Windows by using Arm64 assembly with clang-cl

Extra Arm64 assembly will be enabled in the next stable OpenSSL build

@eukarpov eukarpov changed the title Enabling arm64 assembly on Windows by using clang Enabling Arm64 assembly on Windows by using clang Oct 24, 2023
vcpkg_add_to_path("${clang_path}")

set(as clang-cl)
set(cc clang-cl)
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT the vcpkg owners do not want to use clang as a C compiler in triplet which are meant to use MSVC as a C compiler.
(However, using clang as an assembler is accepted.)

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to check that the toolchain has not a valid ASM_COMPILER being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpenSSL provides two options to build Arm64 assembly with clang-cl, VC-WIN64-CLANGASM-ARM and VC-CLANG-WIN64-CLANGASM-ARM.
https://github.com/openssl/openssl/blob/master/NOTES-WINDOWS.md

In this PR, VC-CLANG-WIN64-CLANGASM-ARM is used. It provides better performance and uses clang-cl as both compiler and assembler.

@Neumann-A could you please clarify how valid ASM_COMPILER should be used if we need clang-cl for AS in any case?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this PR, VC-CLANG-WIN64-CLANGASM-ARM is used. It provides better performance and uses clang-cl as both compiler and assembler.

The compiler must follow the toolchain. The port must not make own decisions, no matter if crypto or video.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you please clarify how valid ASM_COMPILER should be used if we need clang-cl for AS in any case?

If my toolchain specifies a valid CMAKE_ASM_COMPILER (which is not cl) i expect it to be used together with CMAKE_ASM_FLAGS(_<CONFIG>). The toolchain is the king deciding what to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like NASM was used earlier without CMAKE_ASM_COMPILER check, should it be also improved?

Copy link
Contributor

Choose a reason for hiding this comment

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

OpenSSL uses a proprietary configuration/build system. Feeding in our precise toolchain settings might be a different task.

But selecting the best-matching configuration identifier is a fair request. VC-CLANG-WIN64-CLANGASM-ARM is for clang triplets. VC-WIN64-CLANGASM-ARM seems to be an acceptable option for the standard windows triplets.

This means you need to detect the C compiler, and you need to find/acquire the suitable clang as an assembler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been changed as it was suggested

@Cheney-W Cheney-W changed the title Enabling Arm64 assembly on Windows by using clang [OpenSSL] Enabling Arm64 assembly on Windows by using clang Oct 25, 2023
@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Oct 25, 2023
@Cheney-W
Copy link
Contributor

Don't forget modify the vcpkg/ports/openssl/vcpkg.json file:

{
  "name": "openssl",
  "version": "3.1.3",
  "port-version": 1,

@Cheney-W Cheney-W marked this pull request as draft October 25, 2023 10:12
@eukarpov eukarpov marked this pull request as ready for review October 26, 2023 21:21
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Oct 30, 2023
@data-queue data-queue added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 31, 2023
elseif(VCPKG_DETECTED_CMAKE_C_COMPILER_ID MATCHES "Clang")
set(OPENSSL_ARCH VC-CLANG-WIN64-CLANGASM-ARM)
else()
set(OPENSSL_ARCH VC-WIN64-CLANGASM-ARM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean if the C compiler does not match clang we are still assuming Clang is the compiler? Since Clang is in both variable names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clang-cl is used as the assembler in both cases, with clang as the C compiler or with some other C compiler.

@data-queue data-queue removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 2, 2023
@JavierMatosD JavierMatosD merged commit 795111f into microsoft:master Nov 5, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants