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.

Sign up for GitHub

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

Jump to bottom

[halide] Update to version 10.0.0 #13860

Merged
merged 9 commits into from
Oct 14, 2020
Merged

[halide] Update to version 10.0.0 #13860

merged 9 commits into from
Oct 14, 2020

Conversation

alexreinking
Copy link
Contributor

@alexreinking alexreinking commented Oct 3, 2020 •
edited
Loading

I thought I would take a shot at updating Halide to version 10.0.0. This is my first PR for vcpkg, so I apologize in advance for anything I did wrong.

This PR updates Halide to version 10.0.0 which was released a few weeks ago.

Attn: @LilyWangL (we spoke on a related issue on the Halide tracker: halide/Halide#4539)
Attn: @NancyLi1013 (we spoke on #11761)
Attn: @qis (we spoke on the CppLang Slack a couple weeks ago)
Attn: @ras0219-msft (we spoke on the CppLang Slack today)

  • What does your PR fix?

Fixes #13580
Fixes #13823

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

Halide doesn't work with ump or emscripten because it JIT compiles. CI is set to skip x86-windows because the debug configuration is broken by MSVC mis-compiling LLVM (see my comment below)

Users should also avoid linking to Halide statically, especially on macOS and Linux. The autoscheduler plugins do not support building Halide as a static library and are a critical feature. I could be convinced to print a warning, but it is surprising to me that on OS X and Linux, that static libraries are the default. I have used vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) here.

Yes, but with one exception: this PR also fixes the LLVM port features to allow correct dependencies in Halide's features.

Here's the problem: The target-all feature of LLVM is disconnected from the other target features, such as target-nvptx. So enabling the NVPTX backend in Halide would depend on EITHER llvm[core,target-nvptx] OR llvm[core,target-all], which isn't possible. I reworked the target-all feature of LLVM to depend on llvm[core,target-aarch64,...,target-xcore], which fixes the issue (and simplifies the portfile as a bonus). Now Halide's target-nvptx feature depends correctly on llvm[core,target-nvptx] and it will still work when the user previously installed llvm[target-all].

This is sort of a grey area by my reading of the Maintainer Guide. Changing LLVM in this way would seem frivolous were it not for the fact that Halide needs this change. I'm breaking the tie between "Make separate Pull Requests per port" and "Avoid trivial changes in untouched files" in favor of the latter since my primary goal is updating the Halide port.

I did split the changes into two commits in case I made the wrong call here and you want me to open a separate PR for the LLVM changes.

@ghost
Copy link

ghost commented Oct 3, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@alexreinking
[halide] bump Halide to version 10.0.0
de8f014
@alexreinking
Merge remote-tracking branch 'origin' into halide-10.0.0
ae6337a
@alexreinking
Use empty package policy to allow plugins to be located in lib.
3bfdc2f 
This is following the recommendations in a conversation I had
with Robert Schumacher on the #vcpkg CppLang Slack channel.
This recommendation was derived from the fact that "cmake
defaults module DLLs into the lib folder, which makes vcpkg's
current policy very inconvenient for authors" and that I do not
plan to enable build systems other than CMake.
@alexreinking
Copy link
Contributor Author

alexreinking commented Oct 6, 2020 •
edited
Loading

x86-windows is failing due to this compiler bug in MSVC: https://developercommunity.visualstudio.com/content/problem/1179643/msvc-copies-overaligned-non-trivially-copyable-par.html

LLVM has a patch for this in trunk, but backporting it should be a separate PR... https://reviews.llvm.org/rG4e3edef4b8b637c0c76897497eb7c66f00157210

@alexreinking alexreinking marked this pull request as ready for review October 7, 2020 03:54
@alexreinking
Copy link
Contributor Author

This is ready for review if any of y'all have a chance to take a look!

@alexreinking alexreinking mentioned this pull request Oct 7, 2020
Closed
@LilyWangL LilyWangL added category:port-update The issue is with a library, which is requesting update new revision requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Oct 9, 2020
@LilyWangL
Copy link
Contributor

@alexreinking Thanks for your PR. When I installing halide[target-all], I get the following error:
image

@alexreinking
Copy link
Contributor Author

alexreinking commented Oct 10, 2020 •
edited
Loading

That is a known issue with MSVC mis-compiling LLVM. There's a link to the MSVC issue in the CI baseline. It's also mentioned in my second comment, higher in this PR.

Until that is fixed, Halide will not work with or without this PR. It's only because we now execute some Halide code during the v10 build that this is appearing. The problem is between LLVM and MSVC.

Note that this only affects x86-windows-dbg. Release mode on x86 works, as does all of x64

@LilyWangL
Copy link
Contributor

LilyWangL commented Oct 10, 2020 •
edited
Loading

That is a known issue with MSVC mis-compiling LLVM. There's a link to the MSVC issue in the CI baseline. It's also mentioned in my second comment, higher in this PR.

Until that is fixed, Halide will not work with or without this PR. It's only because we now execute some Halide code during the v10 build that this is appearing. The problem is between LLVM and MSVC.

Note that this only affects x86-windows-dbg. Release mode on x86 works, as does all of x64

I test all features on x64 and they work fine.

@LilyWangL LilyWangL removed the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Oct 10, 2020
NancyLi1013
NancyLi1013 reviewed Oct 12, 2020
View reviewed changes
ports/halide/portfile.cmake Show resolved Hide resolved
ports/halide/CONTROL Outdated Show resolved Hide resolved
ports/halide/CONTROL Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Oct 13, 2020
@alexreinking
Copy link
Contributor Author

Thanks for the reviews @LilyWangL and @NancyLi1013! Is there anything else that needs to be done before merging?

@dan-shaw dan-shaw added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 13, 2020
@dan-shaw dan-shaw merged commit 7e3d3be into microsoft:master Oct 14, 2020
@dan-shaw
Copy link
Contributor

Thanks for the PR!

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Oct 14, 2020
@Jimmy-Hu
Merge pull request #355 from microsoft/master
6b4cf31 
[halide] Update to version 10.0.0 (microsoft#13860)
@alexreinking alexreinking mentioned this pull request Oct 14, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dan-shaw dan-shaw dan-shaw approved these changes

@LilyWangL LilyWangL LilyWangL approved these changes

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

@LilyWangL LilyWangL

Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Halide:x64-windows] build failure [Halide] update to 10.0.0
4 participants
@alexreinking @LilyWangL @dan-shaw @NancyLi1013