Added support for ClangCL

[emmenlau]

This PR makes minor changes to the build in order to support ClangCl on VS. The required change is mostly about the stack size. Currently MSVC requires a higher stack size, and this setting was in Util.h. However clang does not support the pragma. Therefore this PR sets the same flag via CMake compiler options for the executables built by this project. The flag is currently not passed transitively to downstream projects. And it is not discriminating between executables in the Halide project - all executables get the higher stack size. This seems to be correct at least for tutorial, test and benchmark executables.

To be discussed:

• What other build systems (than CMake) need to be supported?
  • Quoting from Alex Reinking (Mar 26 20:15) on Gitter: We only support CMake on Windows and that flag does not affect our non-Windows users (due to _MSC_VER check), so it's ok to move.
  • Therefore it seems the CMake build provides the same functionality as the previous solution except it may handle differently the case of passing the stack size to downstream projects. See the next question below.
• Should the stack size be passed transitively to downstream projects?

[alexreinking]

• Should the stack size be passed transitively to downstream projects?

If Halide on Windows requires a larger stack size to not crash, then yes, I believe it should because it is therefore a usage requirement.

[alexreinking]

From Gitter:

It turns out that using the Debug build of LLVM is not such a big issue as long as LLVM is also built with some level of optimization. This required a bit of tweaking in LLVMs cmake instructions, so it may not be the right choice for Halide. But for me it works quite ok so far. Let me know if you're interested in the patch, its a 10-liner only.

-- @emmenlau

Would you mind posting this as a comment so we can see?

[steven-johnson]

You should sync to master to retry the bots.

[alexreinking]

I've been playing around with a few different ways of communicating our stack size requirement to our users. Here are my take-aways.

1. The removal of the pragma from Halide.h warrants an entry in the release notes since users who are not on CMake will need to increase the StackReserveSize in their Visual Studio projects (or whatever the equivalent is in their build system)
2. ClangCL's lld-link.exe does not support this pragma right now, but there's no reason it shouldn't. I have patched it and opened a review with upstream. See here: https://reviews.llvm.org/D99680
3. As this is a usage requirement for Halide on Windows, we need to communicate it to our linkees in CMake, which is our One True Build System on Windows. We need to do this in such a way that it can be raised by the user in case they have extra stack space requirements. I will give a solution below, which will need to be documented in README_cmake.md
4. We can check the stack size programatically on library load. We should do so and emit a warning if the stack size is too low.

---

To do this effectively in CMake, we will need a custom property:

# Inside HalideGeneratorHelpers.cmake
define_property(
    TARGET PROPERTY STACK_RESERVE_SIZE
    BRIEF_DOCS "The minimum stack size required to run."
    FULL_DOCS [=[
Set the stack stack reserved by the operating system on process start. 
Currently Windows-only. Operates by passing the /STACK:N flag to link.exe
]=]
)

We will then need to tell CMake how this custom property works when we attach it to the Halide target.

# Inside src/CMakeLists.txt
add_library(Halide ...)
...
# ClangCL fails to support setting the /STACK flag via #pragma comment(linker, ...), which
# is how we accomplished this with MSVC previously. CMake doesn't have native support for 
# setting the stack size like this, so we use a custom property.
if (MSVC)
    set_property(TARGET Halide PROPERTY INTERFACE_STACK_RESERVE_SIZE 8388608)
    set_property(TARGET Halide APPEND PROPERTY COMPATIBLE_INTERFACE_NUMBER_MAX STACK_RESERVE_SIZE)
    target_link_options(Halide INTERFACE "/STACK:$<TARGET_PROPERTY:STACK_RESERVE_SIZE>")
endif ()

We will need to add an example to README_cmake.md to show how to override this:

add_executable(my_app main.cpp)
target_link_libraries(my_app PRIVATE Halide::Halide)
# override Halide's 8 MB stack to 16 MB.
if (MSVC)
    set_property(TARGET my_app PROPERTY STACK_RESERVE_SIZE 16777216)
endif ()

Previously, I'm not sure users could override this, or maybe they could only with a #pragma comment(linker, ...), which I'm not sure will work.

---

I have a few TODOs here (I will update the above solution if these TODOs change my understanding of things):

1. Validate that all of this exports correctly
2. Check to see if users can override with #pragma comment(linker, "/STACK:...")

[abadams]

Why delete the pragma (instead of wrapping it in an ifndef that detects clang)?

[emmenlau]

Why delete the pragma (instead of wrapping it in an ifndef that detects clang)?

Because clang does need a higher stack size too (actually pretty identical values to the MSVC compiler). But clang does not accept the same pragma. You would effectively be maintaining two parallel solutions, where the Clang-solution would work for MSVC too, but not vice versa.

[emmenlau]

@alexreinking You are progressing really nicely here! I'm terribly busy before easter and will be away afterwards for a week, so please feel free to take over and edit this PR to your liking, or start over in a new PR and drop this one here. I'm not keen on my authorship. I place all my changes from this PR under the public domain, to be used by you or anyone in their own PRs, or to edit this PR in any way you see fit.

[abadams]

The reason to keep the pragma (in addition to the cmake changes) would be to support people including Halide.h from vanilla msvc projects, or gyp-generated msvc projects.

[emmenlau]

The reason to keep the pragma (in addition to the cmake changes) would be to support people including Halide.h from vanilla msvc projects, or gyp-generated msvc projects.

That makes perfect sense, I agree. Its hard for me to suggest a best approach, since I can see that @alexreinking has gone to some lengths to support setting the stack size transitively via cmake. What I can say is that the clang compiler has many benefits over the standard MSVC compiler, so maybe its worthwhile to support it (at least in the longer run). But that's just my two cents...

[alexreinking]

The reason to keep the pragma (in addition to the cmake changes) would be to support people including Halide.h from vanilla msvc projects, or gyp-generated msvc projects.

We don't support vanilla VS or Gyp as a first party build system. We don't test those scenarios, so they're liable to break at any point. Those users are welcome to submit and maintain integration tests for those build systems if they expect reliable support from upstream (especially gyp).

Furthermore, I'm not convinced the pragma is the right solution anyway. It adds linker flags in the order they are encountered with later ones taking precedence. For example:

#include "my_company_header.h"

#include <Halide.h>

would override a (potentially larger) stack size set via pragma in my_company_header.h.

[abadams]

I'd be willing to bet money that the majority of our windows users don't use cmake. We shouldn't casually and intentionally break non-cmake usage.

Edit: I'm talking about users of a precompiled libHalide/Halide.h, not people building Halide itself.

[emmenlau]

2. ClangCL's lld-link.exe does not support this pragma right now, but there's no reason it shouldn't. I have patched it and opened a review with upstream. See here: https://reviews.llvm.org/D99680

Awesome!

3. As this is a usage requirement for Halide on Windows, we need to communicate it to our linkees in CMake, which is our One True Build System on Windows. We need to do this in such a way that it can be raised by the user in case they have extra stack space requirements. I will give a solution below, which will need to be documented in README_cmake.md

This looks like a nice and clean solution and covers the case that users can easily override the setting with higher values (that the current implementation with the pragma makes less clear/easy).

4. We can check the stack size programatically on library load. We should do so and emit a warning if the stack size is too low.

This sounds very reasonable!

[abadams]

Discussion on the gitter is converging to just spawning a thread or fiber with a larger stack to do lowering on, and not touching the build system at all (either via pragmas or cmake stuff).

[steven-johnson]

or gyp-generated msvc projects

...please tell me there aren't still people using gyp :-/

[alexreinking]

Stack size linker requirement obviated by #5873

[alexreinking]

Closing in favor of #5876 (no edit access here, unfortunately).