Skip to content

When crosscompiling, build host tools externally. #5170

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

Closed
wants to merge 1 commit into from
Closed

When crosscompiling, build host tools externally. #5170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,7 @@ $(INCLUDE_DIR)/HalidePyTorchCudaHelpers.h: $(SRC_DIR)/runtime/HalidePyTorchCudaH
@mkdir -p $(@D)
cp $< $(INCLUDE_DIR)/

$(BIN_DIR)/build_halide_h: $(ROOT_DIR)/tools/build_halide_h.cpp
$(BIN_DIR)/build_halide_h: $(ROOT_DIR)/src/tools/build_halide_h.cpp
@-mkdir -p $(@D)
$(CXX) -std=c++11 $< -o $@

Expand Down Expand Up @@ -1081,7 +1081,7 @@ $(BUILD_DIR)/initmod.inlined_c.cpp: $(BIN_DIR)/binary2cpp $(SRC_DIR)/runtime/hal
$(BUILD_DIR)/initmod_ptx.%_ll.cpp: $(BIN_DIR)/binary2cpp $(SRC_DIR)/runtime/nvidia_libdevice_bitcode/libdevice.%.bc
./$(BIN_DIR)/binary2cpp halide_internal_initmod_ptx_$(basename $*)_ll < $(SRC_DIR)/runtime/nvidia_libdevice_bitcode/libdevice.$*.bc > $@

$(BIN_DIR)/binary2cpp: $(ROOT_DIR)/tools/binary2cpp.cpp
$(BIN_DIR)/binary2cpp: $(ROOT_DIR)/src/tools/binary2cpp.cpp
@mkdir -p $(@D)
$(CXX) $< -o $@

Expand Down Expand Up @@ -1390,7 +1390,7 @@ $(FILTERS_DIR)/cxx_mangling_externs.o: $(ROOT_DIR)/test/generator/cxx_mangling_e
$(FILTERS_DIR)/cxx_mangling.a: $(BIN_DIR)/cxx_mangling.generator $(FILTERS_DIR)/cxx_mangling_externs.o
@mkdir -p $(@D)
$(CURDIR)/$< -g cxx_mangling $(GEN_AOT_OUTPUTS) -o $(CURDIR)/$(FILTERS_DIR) target=$(TARGET)-no_runtime-c_plus_plus_name_mangling -f "HalideTest::AnotherNamespace::cxx_mangling"
$(ROOT_DIR)/tools/makelib.sh $@ $@ $(FILTERS_DIR)/cxx_mangling_externs.o
$(ROOT_DIR)/src/tools/makelib.sh $@ $@ $(FILTERS_DIR)/cxx_mangling_externs.o

ifneq ($(TEST_CUDA), )
# Also build with a gpu target to ensure that the GPU-Host generation
Expand All @@ -1399,7 +1399,7 @@ ifneq ($(TEST_CUDA), )
$(FILTERS_DIR)/cxx_mangling_gpu.a: $(BIN_DIR)/cxx_mangling.generator $(FILTERS_DIR)/cxx_mangling_externs.o
@mkdir -p $(@D)
$(CURDIR)/$< -g cxx_mangling $(GEN_AOT_OUTPUTS) -o $(CURDIR)/$(FILTERS_DIR) target=$(TARGET)-no_runtime-c_plus_plus_name_mangling-cuda-cuda_capability_30 -f "HalideTest::cxx_mangling_gpu"
$(ROOT_DIR)/tools/makelib.sh $@ $@ $(FILTERS_DIR)/cxx_mangling_externs.o
$(ROOT_DIR)/src/tools/makelib.sh $@ $@ $(FILTERS_DIR)/cxx_mangling_externs.o
endif

$(FILTERS_DIR)/cxx_mangling_define_extern_externs.o: $(ROOT_DIR)/test/generator/cxx_mangling_define_extern_externs.cpp $(FILTERS_DIR)/cxx_mangling.h
Expand All @@ -1409,7 +1409,7 @@ $(FILTERS_DIR)/cxx_mangling_define_extern_externs.o: $(ROOT_DIR)/test/generator/
$(FILTERS_DIR)/cxx_mangling_define_extern.a: $(BIN_DIR)/cxx_mangling_define_extern.generator $(FILTERS_DIR)/cxx_mangling_define_extern_externs.o
@mkdir -p $(@D)
$(CURDIR)/$< -g cxx_mangling_define_extern $(GEN_AOT_OUTPUTS) -o $(CURDIR)/$(FILTERS_DIR) target=$(TARGET)-no_runtime-c_plus_plus_name_mangling-user_context -f "HalideTest::cxx_mangling_define_extern"
$(ROOT_DIR)/tools/makelib.sh $@ $@ $(FILTERS_DIR)/cxx_mangling_define_extern_externs.o
$(ROOT_DIR)/src/tools/makelib.sh $@ $@ $(FILTERS_DIR)/cxx_mangling_define_extern_externs.o

# pyramid needs a custom arg.
$(FILTERS_DIR)/pyramid.a: $(BIN_DIR)/pyramid.generator
Expand Down
2 changes: 1 addition & 1 deletion apps/support/autoscheduler.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ AUTOSCHED_WEIGHT_OBJECTS=$(AUTOSCHED_BIN)/baseline_weights.o

# TODO(srj): depending on something not in the distrib folder isn't strictly
# kosher, but this is still experimental
$(AUTOSCHED_BIN)/binary2cpp: ../../tools/binary2cpp.cpp
$(AUTOSCHED_BIN)/binary2cpp: ../../src/tools/binary2cpp.cpp
@mkdir -p $(@D)
$(CXX) $< -o $@

Expand Down
42 changes: 42 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,48 @@ set(SOURCE_FILES
WrapCalls.cpp
)

##
# Build time tools
##

if (CMAKE_CROSSCOMPILING)
# Note: using CMAKE_EXECUTABLE_SUFFIX breaks cross-OS builds involving Windows.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this comment -- it breaks the builds but we use it anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't break any existing scenarios, it just isn't a supported scenario without going full superbuild. I can make this clearer.

include(ExternalProject)
ExternalProject_Add(host-tools
Copy link
Contributor

Choose a reason for hiding this comment

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

super-duper-nit: I find it hard to quickly read the keywords-vs-arg-values here; I wonder if it would be more obvious with indentation like:

    ExternalProject_Add(host-tools
                        BUILD_BYPRODUCTS
                              "<INSTALL_DIR>/bin/build_halide_h${CMAKE_EXECUTABLE_SUFFIX}"
                              "<INSTALL_DIR>/bin/binary2cpp${CMAKE_EXECUTABLE_SUFFIX}"
                        SOURCE_DIR 
                              ${CMAKE_CURRENT_LIST_DIR}/tools
                        INSTALL_DIR 
                              ${CMAKE_CURRENT_BINARY_DIR}/host-tools
                        CMAKE_ARGS
                              -D CMAKE_TOOLCHAIN_FILE=${Halide_HOST_TOOLCHAIN_FILE}
                              -D CMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
                              -D CMAKE_INSTALL_PREFIX=<INSTALL_DIR>
                        BUILD_ALWAYS 
                              YES)

Copy link
Member Author

@alexreinking alexreinking Aug 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The formatter in CLion is not very smart and it tramples such indentation 😞 I could add some blank lines to make it a bit more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know there existed an autoformatter for CMake. Does it exist anywhere other than CLion?

Copy link
Member Author

@alexreinking alexreinking Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not that I'm aware of. There are two open-source CMake formatters I'm aware of. I've tried both. One, cmake-format does a bad job, the linter is even worse. The other, gersemi, is not very configurable, is in pre-alpha state and led to a huge diff.

BUILD_BYPRODUCTS
"<INSTALL_DIR>/bin/build_halide_h${CMAKE_EXECUTABLE_SUFFIX}"
"<INSTALL_DIR>/bin/binary2cpp${CMAKE_EXECUTABLE_SUFFIX}"
SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/tools
INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/host-tools
CMAKE_ARGS
-D CMAKE_TOOLCHAIN_FILE=${Halide_HOST_TOOLCHAIN_FILE}
-D CMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-D CMAKE_INSTALL_PREFIX=<INSTALL_DIR>
BUILD_ALWAYS YES)

# We avoid a full super-build by manually creating imported targets here.
# If we start adding lots of build-time tools, we should migrate to the
# export() mechanism, which would also avoid the cross-OS bug.
ExternalProject_Get_Property(host-tools INSTALL_DIR)

add_executable(build_halide_h IMPORTED GLOBAL)
set_target_properties(build_halide_h
PROPERTIES
IMPORTED_LOCATION "${INSTALL_DIR}/bin/build_halide_h${CMAKE_EXECUTABLE_SUFFIX}"
CROSSCOMPILING_EMULATOR "")
add_dependencies(build_halide_h host-tools)

add_executable(binary2cpp IMPORTED GLOBAL)
set_target_properties(binary2cpp
PROPERTIES
IMPORTED_LOCATION "${INSTALL_DIR}/bin/binary2cpp${CMAKE_EXECUTABLE_SUFFIX}"
CROSSCOMPILING_EMULATOR "")
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we don't explicitly set CROSSCOMPILING_EMULATOR? Is empty not the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure how CMake will treat it given that it's an imported target, but for non-imported executable targets, it wants CROSSCOMPILING_EMULATOR to be defined if the target is used in add_custom_command. So I'm being defensive here.

add_dependencies(binary2cpp host-tools)
else ()
# Do the safe, reliable thing when not cross-compiling.
add_subdirectory(tools)
endif ()

##
# Build and import the runtime.
##
Expand Down
19 changes: 19 additions & 0 deletions src/tools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
cmake_minimum_required(VERSION 3.16)
project(host-tools)

# Require standard C++14
set(CMAKE_CXX_STANDARD 14)
Copy link
Contributor

Choose a reason for hiding this comment

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

...don't we actually only require C++11?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC when we went through and looked at our list of supported compilers (GCC 5+, Clang 9+, MSVC 2017+), C++14 was the GCD among them.

set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

add_executable(binary2cpp binary2cpp.cpp)
add_executable(build_halide_h build_halide_h.cpp)
add_executable(find_inverse find_inverse.cpp)

# Don't install when included via add_subdirectory.
if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
include(GNUInstallDirs)
install(TARGETS build_halide_h binary2cpp find_inverse
EXPORT host-tools
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
endif ()
0 tools/binary2cpp.cpp → src/tools/binary2cpp.cpp
View file
Open in desktop
File renamed without changes.
0 tools/build_halide_h.cpp → src/tools/build_halide_h.cpp
View file
Open in desktop
File renamed without changes.
0 tools/find_inverse.cpp → src/tools/find_inverse.cpp
View file
Open in desktop
File renamed without changes.
0 tools/makelib.sh → src/tools/makelib.sh
View file
Open in desktop
File renamed without changes.
12 changes: 1 addition & 11 deletions tools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,3 @@
##
# Build time tools
##

add_executable(build_halide_h build_halide_h.cpp)
target_compile_options(build_halide_h PRIVATE $<$<CXX_COMPILER_ID:MSVC>:/wd4996>)

add_executable(binary2cpp binary2cpp.cpp)
target_compile_options(binary2cpp PRIVATE $<$<CXX_COMPILER_ID:MSVC>:/wd4996>)

##
# Interface target for enabling PNG/JPEG support in Halide
##
Expand Down Expand Up @@ -54,4 +44,4 @@ add_library(Halide::Tools ALIAS Halide_Tools)
target_include_directories(Halide_Tools
INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>)
set_target_properties(Halide_Tools PROPERTIES EXPORT_NAME Tools)
set_target_properties(Halide_Tools PROPERTIES EXPORT_NAME Tools)