Fix issue in find_package in cross-compilation for no OS

[stevesuzuki-arm]

When using toolchain where Threads libs are not available, which is the case in baremetal target cross-compilation, we were not able to load even HalideHelpers pacakge.

[alexreinking]

My comments are a bit mixed, but I wanted to keep my whole thought process around.

What I'm not sure about is whether the lack of threads must be fixed upon package export (i.e. does it constitute a package variant?) or if it can truly be set by the loader of the package.

In the latter case, we should move targets that depend on threads to a separate export set, so they aren't even created in the user's build when Halide_NO_THREADS is set.

[alexreinking]

In #7297 we removed the dependency on Threads::Threads for Halide::Halide, Halide::Generator, and Halide::RunGenMain. I wonder if this change can be improved in light of that.

[stevesuzuki-arm]

In #7297 we removed the dependency on Threads::Threads for Halide::Halide, Halide::Generator, and Halide::RunGenMain. I wonder if this change can be improved in light of that.

The latest commit no longer uses Halide_NO_THREADS.

Alternative approach where user explicitly declares is to add the option argument NO_THREADS to add_halide_library and add_halide_runtime to skip linking Threads.

[steven-johnson]

Where does this PR stand?

[steven-johnson]

Ping for status

[alexreinking]

Simple change for less-noisy UX. Please see my comment below

[alexreinking]

Overall, I'm a little bit concerned about how this PR determines whether or not a runtime will get a dependency on Threads::Threads.

The "real" problem as I understand it is that, currently, pipeline libraries that don't actually use threads in their schedules get a spurious dependency on Threads::Threads. Most of the time, this is benign, but on target machines that don't support threads in the first place, this becomes an error. Right?

Thus, it seems like the most correct solution would be to only add Threads::Threads if any schedule in the library uses .parallel(). However, this would be extremely complex to implement without at least requiring all Halide generators to be in a separate package from the libraries they produce, which makes the common cases as hard as the cross-compiling cases. It would also require work on the generator side to quickly check that property (but not run the whole compiler, which would be slow). Not awesome.

So we're electing to use a proxy instead. In this PR, that proxy is "whether or not the Threads package can be found". I think that's too loose, especially for larger projects.

Better would be to take a two step approach:

1. Add an argument to add_halide_library that passes through to add_halide_runtime (both functions have the argument) that optionally disables threads. Say, NO_THREADS. When NOT NO_THREADS and NOT TARGET Threads::Threads, then we find_package(Threads REQUIRED) inside add_halide_runtime.
2. The default for NO_THREADS is NO unless the variable Halide_RUNTIME_NO_THREADS is defined, in which case it provides the default value.

This is pretty similar to your prior implementation, with the major difference being the function argument qualification so that invocations of add_halide_library that the user knows will not use threads can have this property specified locally/declaratively in the call.

[steven-johnson]

only add Threads::Threads if any schedule in the library uses .parallel().

(1) .... or async()
(2) Doing so would also require upgrading LLVM_Runtime_Linker; it currently will unconditionally add threading-runtime-support on all platforms that can possibly support it, regardless of whether the thing being linked actually uses any of those features.

[alexreinking]

(1) .... or async()

D'oh. Yes.

(2) Doing so would also require upgrading LLVM_Runtime_Linker; it currently will unconditionally add threading-runtime-support on all platforms that can possibly support it, regardless of whether the thing being linked actually uses any of those features.

Maybe this means that whether or not threads are required can be accurately computed from the target string?

[stevesuzuki-arm]

1. Add an argument to add_halide_library that passes through to add_halide_runtime (both functions have the argument) that optionally disables threads. Say, NO_THREADS. When NOT NO_THREADS and NOT TARGET Threads::Threads, then we find_package(Threads REQUIRED) inside add_halide_runtime.
2. The default for NO_THREADS is NO unless the variable Halide_RUNTIME_NO_THREADS is defined, in which case it provides the default value.

I have updated the patch accordingly. I appreciate your efforts in your busy time!

[steven-johnson]

Looks like some breakage from top-of-tree LLVM changes, fixes underway

[steven-johnson]

So is this ready to land?

[alexreinking]

So is this ready to land?

Yep, merged.