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

[OpenBLAS] Upgrade to 0.3.21 and add arm64-windows support #26396

Merged
merged 3 commits into from
Aug 25, 2022
Merged

[OpenBLAS] Upgrade to 0.3.21 and add arm64-windows support #26396

merged 3 commits into from
Aug 25, 2022

Conversation

nursik
Copy link
Contributor

@nursik nursik commented Aug 17, 2022

  • What does your PR fix?

    Upgrades to 0.3.21 and adds arm64-windows triplet. arm-uwp removed

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

    !(arm & uwp), Yes

  • 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

@FrankXie05 FrankXie05 self-assigned this Aug 18, 2022
@FrankXie05 FrankXie05 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision labels Aug 18, 2022
@FrankXie05
Copy link
Contributor

Thanks for submitting this PR, I got an error when I tested arm64-windows locally, can you confirm it? :) @nursik

PS E:\feature-test\vcpkg> ./vcpkg install openblas[*]:arm64-windows                                                    
 Computing installation plan...                                                                                        
 The following packages will be built and installed:                                                                   
	openblas[core,dynamic-arch,simplethread,threads]:arm64-windows -> 0.3.21                                           
 Detecting compiler hash for triplet arm64-windows...                                                               
 Restored 0 package(s) from C:\Users\test\AppData\Local\vcpkg\archives in 299.1 us. Use --debug to see more details.     
 Installing 1/1 openblas:arm64-windows...                                                                             
 Building openblas[core,dynamic-arch,simplethread,threads]:arm64-windows...                                           
 -- Using cached xianyi-OpenBLAS-b89fb708caa5a5a32de8f4306c4ff132e0228e9a.tar.gz.                                     
 -- Cleaning sources at E:/feature-test/vcpkg/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean.  Use --editable to skip cleaning for the packages you specify.                                      
 -- Extracting source E:/feature-test/vcpkg/downloads/xianyi-OpenBLAS-b89fb708caa5a5a32de8f4306c4ff132e0228e9a.tar.gz   
 -- Applying patch uwp.patch                                                                                           
 -- Applying patch fix-space-path.patch                                                                               
 -- Applying patch fix-redefinition-function.patch                                                                    
 -- Applying patch fix-uwp-build.patch                                                                               
 -- Applying patch install-tools.patch                                                                               
 -- Using source at E:/feature-test/vcpkg/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean                        
 -- Found external ninja('1.10.2').                                                                                  
 -- Configuring arm64-windows                                                                                         
 -- Building arm64-windows-dbg                                                                                        
 CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:129 (message):                                        
	 Command failed: "C:/Program Files/CMake/bin/cmake.exe" --build . --config Debug --target install -- -v -j9           
	 Working Directory: E:/feature-test/vcpkg/buildtrees/openblas/arm64-windows-dbg                                       
	 See logs for more information:                                                                                       
		E:\feature-test\vcpkg\buildtrees\openblas\install-arm64-windows-dbg-out.log    
		
 Call Stack (most recent call first):                                                                                  
	 installed/x64-windows/share/vcpkg-cmake/vcpkg_cmake_build.cmake:74 (vcpkg_execute_build_process)                      
	 installed/x64-windows/share/vcpkg-cmake/vcpkg_cmake_install.cmake:16 (vcpkg_cmake_build)                              
	 ports/openblas/portfile.cmake:71 (vcpkg_cmake_install)                                                                
	 scripts/ports.cmake:147 (include)   
	 
 error: building openblas:arm64-windows failed with: BUILD_FAILED                                                  
 . . .
PS E:\feature-test\vcpkg> 

Log from install-arm64-windows-dbg-out.log:

E:/feature-test/vcpkg/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean/driver/level3/syrk_kernel.c(65): error C2057: expected constant expression
E:/feature-test/vcpkg/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean/driver/level3/syrk_kernel.c(65): error C2466: cannot allocate an array of constant size 0
E:/feature-test/vcpkg/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean/driver/level3/syrk_kernel.c(65): error C2133: 'subbuffer': unknown size

install-arm64-windows-dbg-out.log

@nursik
Copy link
Contributor Author

nursik commented Aug 18, 2022

@FrankXie05 Are you using Visual Studio 17.3 Preview 3 or later? Because it only works with it.

@FrankXie05
Copy link
Contributor

@nursik Sorry, the version I tested is Visual Studio 16.11 , I will retest with the latest version Visual Studio 17.4 Preview 1. :)

@FrankXie05
Copy link
Contributor

I tested with the latest VS version, but the result is still the same error. :(
Log from install-arm64-windows-dbg-out.log:

E:/test/openblas/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean/driver/level3/syrk_kernel.c(65): error C2057: expected constant expression
E:/test/openblas/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean/driver/level3/syrk_kernel.c(65): error C2466: cannot allocate an array of constant size 0
E:/test/openblas/buildtrees/openblas/src/32e0228e9a-ba8c0c7bab.clean/driver/level3/syrk_kernel.c(65): error C2133: 'subbuffer': unknown size

And the version of Visual studio version is Visual Studio 17.4.0 Preview 1.0

image

Log from config-arm64-windows-out.log:

-DVCPKG_PLATFORM_TOOLSET=v143

install-arm64-windows-dbg-out.log
config-arm64-windows-out.log

@nursik
Copy link
Contributor Author

nursik commented Aug 18, 2022

Can you send me your Visual Studio's configuration file?

@nursik
Copy link
Contributor Author

nursik commented Aug 18, 2022

There is a configuration file for 17.3 preview 5 for x64 host. BTW the build process was successful on x64 and ARM64 machines.
vsconfig.txt
.

@FrankXie05
Copy link
Contributor

@nursik Sorry for the late reply, here is the configuration file I exported for my Visual Studio.
vsconfig.txt

@nursik
Copy link
Contributor Author

nursik commented Aug 19, 2022

@FrankXie05 I was trying to figure out why you cannot build it, while my machine and CI/CD could do that. And only now I figured out that you trying to install openblas[*], not only core :) I tried openblas[core,simplethread,threads] on x64 and it works. For dynamic-arch feature I need to check a possible regression from 0.3.20.

@nursik
Copy link
Contributor Author

nursik commented Aug 19, 2022

@FrankXie05 I tested various combinations of features, and you can see the results below. Could you please confirm that openblas[*]:x64-windows did not work before (using 0.3.20 version)? Because it seems there is no regression, and it was not supported before.

ARM machine

arm64-windows triplet

V\F core core,simplethread,threads *
0.3.20 no no no
0.3.21 yes yes no

x64-windows triplet

V\F core core,simplethread,threads *
0.3.20 no no no
0.3.21 yes yes no

x86_64 machine

arm64-windows triplet

V\F core core,simplethread,threads *
0.3.20 no no no
0.3.21 yes yes no

x64-windows triplet

V\F core core,simplethread,threads *
0.3.20 yes yes no
0.3.21 yes yes no

@FrankXie05
Copy link
Contributor

@nursik Thank you for your test results, based on the latest test results of CI, I chose to approve this PR, maybe there is something wrong with my local vs setup, I asked other members of my team to test and the test passed. :)

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Aug 22, 2022
@@ -201,6 +201,7 @@ discord-rpc:x64-uwp=fail
# requires g++11
discordcoreapi:x64-linux=fail
dlib:arm-uwp=fail
dlib:arm64-windows=fail
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok that some ports are marked as fail instead of being fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By fixing you mean adding !(arm & windows) to vcpkg.json files or fixing the ports' build processes?

@@ -234,6 +235,7 @@ embree2:arm64-osx=skip
embree2:arm64-osx=skip
epsilon:arm-uwp=fail
epsilon:x64-uwp=fail
faiss:arm64-windows=fail
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why these ports are being marked as fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These ports are dependent on OpenBLAS and did not implicitly support arm64-windows. Because OpenBLAS was not supported for arm64-windows, these ports were failing to be installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JavierMatosD Are you suggesting changing vcpkg.json files, instead of removing these packages from baseline

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think this is fine :)

@JavierMatosD JavierMatosD added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 22, 2022
@FrankXie05 FrankXie05 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 25, 2022
@FrankXie05
Copy link
Contributor

Change label for JavierMatosD review.

@JavierMatosD
Copy link
Contributor

Thank you!

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 category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants