[gmp] arm64 windows asm optimization by eukarpov · Pull Request #26764 · microsoft/vcpkg

eukarpov · 2022-09-12T16:04:08Z

Describe the pull request

What does your PR fix?

Add arm64:windows asm optimization compatibility to improve performance
Which triplets are supported/not supported? Have you updated the CI baseline?

All, No
Does your PR follow the maintainer guide?

Yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

Neumann-A · 2022-09-12T17:37:30Z

the correct approach is probably to drop yasm and simply switch to clang (#23660).

eukarpov · 2022-09-12T17:49:21Z

the correct approach is probably to drop yasm and simply switch to clang (#23660).

It looks like switch to clang requires a lot of changes.
This PR introduces a simple bash converter from gas to armasm64 and does not use yasm.

Did (#23660) pass all tests provided by gmp package?

Neumann-A · 2022-09-12T18:02:24Z

#23660 can be reduced to just use vcpkg_find_acquire_program(CLANG)

Cheney-W · 2022-09-13T09:15:13Z

Failure logs:

D:\buildtrees\gmp\config-arm64-windows-dbg-err.log

D:\buildtrees\gmp\config-arm-uwp-dbg-err.log

/Users/vagrant/Data/buildtrees/gmp/build-x64-osx-dbg-err.log

All logs

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for gmp have changed but the version was not updated
version: 6.2.1#14
old SHA: a84071723756ce12828e029f87fb770787d69ac7
new SHA: a48c67e5a1f83d6beb79bf191c8b4ef96e6f1be7
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for gmp have changed but the version was not updated
version: 6.2.1#14
old SHA: a48c67e5a1f83d6beb79bf191c8b4ef96e6f1be7
new SHA: 5159d4a4b0cb125910889820115601aa3ca8a6aa
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for gmp have changed but the version was not updated
version: 6.2.1#14
old SHA: a48c67e5a1f83d6beb79bf191c8b4ef96e6f1be7
new SHA: 5159d4a4b0cb125910889820115601aa3ca8a6aa
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

* remove gas2armasm64 converter

…om/eukarpov/vcpkg into gmp-arm64-windows-asm-optimization

eukarpov · 2022-09-13T21:39:49Z

@Neumann-A thank you for your comment. Clang from Visual Studio has been used to prepare changes in this PR. Tests and performance benchmark have not been executed yet.

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for gmp have changed but the version was not updated
version: 6.2.1#14
old SHA: 4a8c0cd061abb65475b1ec285e61a2507064a788
new SHA: c2fccf3d63c01fc47ab821a58790ea70edd60f00
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

dg0yt · 2022-10-21T05:26:16Z

ports/gmp/portfile.cmake

+        vcpkg_download_distfile(
+            ARM64PATCH
+            URLS https://gmplib.org/repo/gmp/raw-rev/5f32dbc41afc
+            FILENAME 5f32dbc41afc.patch


IMO this is not a good filename for a source artifact. When we can choose names, they should tell us the port and/or origin they belong to.

eukarpov · 2022-10-22T11:48:34Z

ports/gmp/portfile.cmake

+    if(VCPKG_TARGET_ARCHITECTURE STREQUAL "arm64")
+        vcpkg_add_to_path("$ENV{VCINSTALLDIR}/Tools/Llvm/bin")
+        set(ENV{CCAS} "clang-cl --target=arm64-pc-win32 -c")
+        # Avoid the x18 register since it is reserved on Darwin.


The x18 register is a platform register based on ARM64 ABI for Windows convention. It turned out that Darwin has the same issue. I would change the comment to avoid confusion why Darwin patch is used to avoid issue on ARM64 Windows.

That's fine; I was just copying the upstream comment.

ports/gmp/portfile.cmake

BillyONeal · 2022-10-26T22:17:06Z

ports/gmp/portfile.cmake

+    if(VCPKG_TARGET_ARCHITECTURE STREQUAL "arm64")
+        vcpkg_add_to_path("$ENV{VCINSTALLDIR}/Tools/Llvm/bin")
+        set(ENV{CCAS} "clang-cl --target=arm64-pc-win32 -c")
+        # Avoid the x18 register since it is reserved on Darwin.


That's fine; I was just copying the upstream comment.

Cheney-W · 2022-11-11T08:43:45Z

Pinging @eukarpov Could you address the review suggestions of Billy? Thanks!

github-actions

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
vcpkg_extract_source_archive_ex -> vcpkg_extract_source_archive
vcpkg_build_msbuild -> vcpkg_install_msbuild
vcpkg_copy_tool_dependencies -> vcpkg_copy_tools
vcpkg_apply_patches should be replaced by the PATCHES arguments to the "extract" helpers (e.g. vcpkg_from_github())

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

ports/gmp/portfile.cmake

eukarpov · 2022-11-12T08:32:22Z

Pinging @eukarpov Could you address the review suggestions of Billy? Thanks!

Requested change has been done

github-actions

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)
vcpkg_extract_source_archive_ex -> vcpkg_extract_source_archive
vcpkg_build_msbuild -> vcpkg_install_msbuild
vcpkg_copy_tool_dependencies -> vcpkg_copy_tools
vcpkg_apply_patches should be replaced by the PATCHES arguments to the "extract" helpers (e.g. vcpkg_from_github())

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

ports/gmp/portfile.cmake

dg0yt · 2022-11-12T14:25:20Z

I'm working on fixing general cross build errors with this port (arm linux, mingw). The structural changes to the portfile go into a different direction, however I may try to integrate arm64-windows changes.

dg0yt · 2022-11-13T13:25:51Z

I may try to integrate arm64-windows changes.

Done, #27787.

BillyONeal · 2022-11-18T20:58:17Z

The changes in this PR were incorporated into #27787. Thanks for your contribution!

Evgeny Karpov added 2 commits September 12, 2022 17:39

* add arm64:windows asm optimization compatibility

47a1a72

* update version

1b98bfe

github-actions bot previously approved these changes Sep 12, 2022

View reviewed changes

JonLiu1993 assigned Cheney-W Sep 13, 2022

Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Sep 13, 2022

Cheney-W added the requires:author-response label Sep 13, 2022

* using clang-cl to compile arm64 gas assembly

4b10959

eukarpov dismissed github-actions[bot]’s stale review via 4b10959 September 13, 2022 17:18

github-actions bot requested changes Sep 13, 2022

View reviewed changes

eukarpov changed the title ~~[gmp] arm64 windows asm optimization~~ [gmp] arm64 windows asm optimization DRAFT Sep 13, 2022

* fix portfile configuration for arm target

606779d

github-actions bot requested changes Sep 13, 2022

View reviewed changes

Merge branch 'microsoft:master' into gmp-arm64-windows-asm-optimization

5116efe

github-actions bot requested changes Sep 13, 2022

View reviewed changes

Evgeny Karpov added 3 commits September 13, 2022 21:58

* fix portfile configuration for arm target

367a4c1

* remove gas2armasm64 converter

* update version

7f0a155

Merge branch 'gmp-arm64-windows-asm-optimization' of https://github.c…

9060d64

…om/eukarpov/vcpkg into gmp-arm64-windows-asm-optimization

github-actions bot previously approved these changes Sep 13, 2022

View reviewed changes

Cheney-W added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Sep 14, 2022

Evgeny Karpov added 2 commits September 14, 2022 23:24

* arm64 patch and portfile refactoring

5efb2e6

* update version

8c19de4

eukarpov dismissed github-actions[bot]’s stale review via 8c19de4 September 14, 2022 21:29

github-actions bot previously approved these changes Sep 14, 2022

View reviewed changes

eukarpov changed the title ~~[gmp] arm64 windows asm optimization DRAFT~~ [gmp] arm64 windows asm optimization Sep 14, 2022

BillyONeal dismissed stale reviews from Cheney-W and github-actions[bot] via eb299f2 October 21, 2022 03:10

github-actions bot requested changes Oct 21, 2022

View reviewed changes

Version database.

80661de

github-actions bot previously approved these changes Oct 21, 2022

View reviewed changes

dg0yt reviewed Oct 21, 2022

View reviewed changes

Cheney-W added the requires:author-response label Oct 21, 2022

Use a better name for the patch download.

f0e7878

BillyONeal dismissed github-actions[bot]’s stale review via f0e7878 October 21, 2022 19:02

github-actions bot previously approved these changes Oct 21, 2022

View reviewed changes

eukarpov commented Oct 22, 2022

View reviewed changes

BillyONeal requested changes Oct 26, 2022

View reviewed changes

Cheney-W removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 11, 2022

Evgeny Karpov added 2 commits November 11, 2022 23:04

* add clang-cl check

ee37e13

* Update version

f696c88

eukarpov dismissed github-actions[bot]’s stale review via f696c88 November 11, 2022 22:14

github-actions bot approved these changes Nov 11, 2022

View reviewed changes

eukarpov requested a review from BillyONeal November 11, 2022 22:18

eukarpov closed this Nov 12, 2022

eukarpov reopened this Nov 12, 2022

github-actions bot approved these changes Nov 12, 2022

View reviewed changes

This was referenced Nov 12, 2022

[gmp] build failure #24323

Closed

[gmp] Cleanup, fix cross-builds, switch windows from yasm to clang #27787

Merged

dg0yt pushed a commit to dg0yt/vcpkg that referenced this pull request Nov 13, 2022

Integrate arm64 patches from microsoft#26764

ddced27

BillyONeal closed this Nov 18, 2022

Conversation

eukarpov commented Sep 12, 2022

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Uh oh!

Neumann-A commented Sep 12, 2022

Uh oh!

eukarpov commented Sep 12, 2022

Uh oh!

Neumann-A commented Sep 12, 2022

Uh oh!

Cheney-W commented Sep 13, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

eukarpov commented Sep 13, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

eukarpov Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

BillyONeal Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BillyONeal Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Cheney-W commented Nov 11, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

eukarpov commented Nov 12, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

dg0yt commented Nov 12, 2022

Uh oh!

dg0yt commented Nov 13, 2022

Uh oh!

BillyONeal commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?