vcpkg icon indicating copy to clipboard operation
vcpkg copied to clipboard
verified publisher icon microsoft

[gmp] arm64 windows asm optimization

Open eukarpov opened this issue 3 years ago • 9 comments

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

eukarpov avatar Sep 12 '22 16:09 eukarpov

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

Neumann-A avatar Sep 12 '22 17:09 Neumann-A

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?

eukarpov avatar Sep 12 '22 17:09 eukarpov

https://github.com/microsoft/vcpkg/pull/23660 can be reduced to just use vcpkg_find_acquire_program(CLANG)

Neumann-A avatar Sep 12 '22 18:09 Neumann-A

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

Cheney-W avatar Sep 13 '22 09:09 Cheney-W

@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.

eukarpov avatar Sep 13 '22 21:09 eukarpov

all gmp tests passed and gmp benchmark shows performance improvement

eukarpov avatar Sep 14 '22 22:09 eukarpov

This PR has focus only on arm64-windows

I don't think that is a strong argument. It makes the build more consist on all windows archs if clang-cl is used for x64/x86. It also simplifies the portfile. Furthermore, you have to correct the manifest in the context of the yasm dep any way for this PR. (since currently it will assume yasm is used for arm.)

Neumann-A avatar Sep 15 '22 10:09 Neumann-A

This PR has focus only on arm64-windows

I don't think that is a strong argument. It makes the build more consist on all windows archs if clang-cl is used for x64/x86. It also simplifies the portfile. Furthermore, you have to correct the manifest in the context of the yasm dep any way for this PR. (since currently it will assume yasm is used for arm.)

yasm is not used on arm-windows. also assembly optimization is disabled on arm-windows.

eukarpov avatar Sep 15 '22 14:09 eukarpov

yasm is not used on arm-windows. also assembly optimization is disabled on arm-windows.

so the dependency needs to be remove from the manifest for arm......

Neumann-A avatar Sep 19 '22 12:09 Neumann-A

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

Cheney-W avatar Nov 11 '22 08:11 Cheney-W

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

Requested change has been done

eukarpov avatar Nov 12 '22 08:11 eukarpov

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 avatar Nov 12 '22 14:11 dg0yt

I may try to integrate arm64-windows changes.

Done, https://github.com/microsoft/vcpkg/pull/27787.

dg0yt avatar Nov 13 '22 13:11 dg0yt

The changes in this PR were incorporated into https://github.com/microsoft/vcpkg/pull/27787. Thanks for your contribution!

BillyONeal avatar Nov 18 '22 20:11 BillyONeal