Skip to content

Improve ClangCL support by disabling, fixing warnings #5876

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

Merged
merged 11 commits into from
Apr 7, 2021
Merged

Improve ClangCL support by disabling, fixing warnings #5876

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5aad444
Added support for ClangCL on MSVC
emmenlau Mar 25, 2021
a7d4e8c
Remove stack-size workarounds
alexreinking Apr 2, 2021
014afc3
Add ClangCL/lld-link.exe's warning to TargetExportScript.cmake
alexreinking Apr 2, 2021
cf946dd
Clean up TargetExportScript.cmake
alexreinking Apr 5, 2021
41e8bf8
Disable tons of warnings from ClangCL's `-Wall`.
alexreinking Apr 5, 2021
f7ddd48
Fix problems discovered by legitimate warnings.
alexreinking Apr 5, 2021
4265af5
Remove one of the long variables.
alexreinking Apr 5, 2021
b19a7b3
Disable introspection if MSVC or execinfo.h not available.
alexreinking Apr 6, 2021
3a60c98
Remove empty statements and unnecessary semicolons
alexreinking Apr 6, 2021
b502cae
Don't pass /MP except in MSBuild (Visual Studio)
alexreinking Apr 6, 2021
808c1e7
tickle buildbots
alexreinking Apr 6, 2021
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
33 changes: 19 additions & 14 deletions cmake/TargetExportScript.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,22 @@ function(target_export_script TARGET)
set(multiValueArgs)
cmake_parse_arguments(ARG "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})

get_property(target_type TARGET ${TARGET} PROPERTY TYPE)
if (NOT target_type STREQUAL "SHARED_LIBRARY")
# Linker scripts do nothing on non-shared libraries.
return()
endif ()

if (MSVC)
## TODO: implement something similar for Windows/link.exe
# https://github.com/halide/Halide/issues/4651
return()
endif ()

set(dummy_source [[ int main() { return 0; } ]])
set(is_shared "$<STREQUAL:$<TARGET_PROPERTY:TYPE>,SHARED_LIBRARY>")

# CMake doesn't recognize MSVC/link.exe's unknown-option warning.
set(extra_errors FAIL_REGEX "LNK4044: unrecognized option")
# CMake doesn't recognize MSVC/ldd link.exe's unknown-option warnings
set(extra_errors FAIL_REGEX "LNK4044: unrecognized option|warning : ignoring unknown argument")

## More linkers support the GNU syntax (ld, lld, gold), so try it first.
set(version_script "LINKER:--version-script=${ARG_GNU_LD}")
Expand All @@ -22,8 +33,8 @@ function(target_export_script TARGET)
check_cxx_source_compiles("${dummy_source}" LINKER_HAS_FLAG_VERSION_SCRIPT ${extra_errors})

if (LINKER_HAS_FLAG_VERSION_SCRIPT)
target_link_options(${TARGET} PRIVATE "$<${is_shared}:${version_script}>")
set_property(TARGET ${TARGET} APPEND PROPERTY LINK_DEPENDS "$<${is_shared}:${ARG_GNU_LD}>")
target_link_options(${TARGET} PRIVATE "${version_script}")
set_property(TARGET ${TARGET} APPEND PROPERTY LINK_DEPENDS "${ARG_GNU_LD}")
return()
endif ()

Expand All @@ -34,16 +45,10 @@ function(target_export_script TARGET)
check_cxx_source_compiles("${dummy_source}" LINKER_HAS_FLAG_EXPORTED_SYMBOLS_LIST ${extra_errors})

if (LINKER_HAS_FLAG_EXPORTED_SYMBOLS_LIST)
target_link_options(${TARGET} PRIVATE "$<${is_shared}:${exported_symbols_list}>")
set_property(TARGET ${TARGET} APPEND PROPERTY LINK_DEPENDS "$<${is_shared}:${ARG_APPLE_LD}>")
target_link_options(${TARGET} PRIVATE "${exported_symbols_list}")
set_property(TARGET ${TARGET} APPEND PROPERTY LINK_DEPENDS "${ARG_APPLE_LD}")
return()
endif ()

## TODO: implement something similar for Windows/link.exe
# https://github.com/halide/Halide/issues/4651

## Warn the user if we were supposed to have been able to attach a linker script.
if (BUILD_SHARED_LIBS AND NOT MSVC)
message(WARNING "Unknown linker! Could not attach Halide linker script.")
endif ()
message(WARNING "Unknown linker! Could not attach Halide linker script.")
endfunction()
117 changes: 75 additions & 42 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,7 @@ add_custom_command(OUTPUT "${Halide_BINARY_DIR}/include/Halide.h"
DEPENDS build_halide_h "${LICENSE_PATH}" ${HEADER_FILES}
WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}"
VERBATIM)
add_custom_target(HalideIncludes ALL
DEPENDS "${Halide_BINARY_DIR}/include/Halide.h")
add_custom_target(HalideIncludes ALL DEPENDS "${Halide_BINARY_DIR}/include/Halide.h")

##
# Define the Halide library target.
Expand All @@ -369,10 +368,8 @@ add_library(Halide::Halide ALIAS Halide)

target_link_libraries(Halide PRIVATE Halide::LLVM)
target_link_libraries(Halide PUBLIC Halide::LanguageOptions)
target_compile_definitions(Halide
PRIVATE
$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,STATIC_LIBRARY>:Halide_STATIC_DEFINE>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:WITH_INTROSPECTION>)
target_compile_definitions(Halide PRIVATE $<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,STATIC_LIBRARY>:Halide_STATIC_DEFINE>)
target_compile_features(Halide PUBLIC cxx_std_11)

include(TargetExportScript)
## TODO: implement something similar for Windows/link.exe
Expand All @@ -386,57 +383,93 @@ set_target_properties(Halide PROPERTIES
VERSION ${Halide_VERSION}
SOVERSION ${Halide_VERSION_MAJOR})

target_include_directories(Halide INTERFACE "$<BUILD_INTERFACE:${Halide_BINARY_DIR}/include>")
add_dependencies(Halide HalideIncludes)

option(Halide_WITH_INTROSPECTION "Enable use of debugging symbols for default Func, Var, etc. names" ON)
if (Halide_WITH_INTROSPECTION)
target_compile_definitions(Halide PRIVATE WITH_INTROSPECTION)
endif ()

if (TARGET wabt-obj)
target_link_libraries(Halide PRIVATE wabt-obj)
target_compile_definitions(Halide PRIVATE WITH_WABT)
endif ()

##
# Include paths for libHalide
##

set(Halide_INCLUDE_PATH "$<BUILD_INTERFACE:${Halide_BINARY_DIR}/include>")
target_include_directories(Halide INTERFACE ${Halide_INCLUDE_PATH})

##
# Set compiler options for libHalide
##

target_compile_options(Halide
PRIVATE
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wall>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-unused-function>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wcast-qual>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wignored-qualifiers>
target_compile_options(
Halide
PRIVATE
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wall>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wcast-qual>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wignored-qualifiers>

$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-c++98-compat-pedantic>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-c++98-compat>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-cast-align>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-comma>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-covered-switch-default>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-deprecated-declarations>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-documentation-unknown-command>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-documentation>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-double-promotion>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-exit-time-destructors>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-extra-semi-stmt>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-extra-semi>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-float-conversion>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-float-equal>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-global-constructors>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-implicit-float-conversion>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-implicit-int-conversion>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-implicit-int-float-conversion>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-missing-field-initializers>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-missing-prototypes>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-nonportable-system-include-path>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-old-style-cast>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-reserved-id-macro> # can't have an underscore followed by a capital letter
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-return-std-move-in-c++11>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-shadow-field-in-constructor>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-shadow-field>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-shadow>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-shorten-64-to-32>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-sign-conversion>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-switch-enum>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-undef>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-undefined-func-template>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-unused-function>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-unused-member-function>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-unused-parameter>
$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wno-unused-template>

$<$<COMPILE_LANG_AND_ID:CXX,GNU,Clang,AppleClang>:-Woverloaded-virtual>
$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-Wsuggest-override>
$<$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>:-Winconsistent-missing-override>
$<$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>:-Winconsistent-missing-destructor-override>
$<$<COMPILE_LANG_AND_ID:CXX,GNU,Clang,AppleClang>:-Woverloaded-virtual>
$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-Wsuggest-override>
$<$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>:-Winconsistent-missing-override>
$<$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>:-Winconsistent-missing-destructor-override>

$<$<CXX_COMPILER_ID:MSVC>:/W3>
$<$<CXX_COMPILER_ID:MSVC>:/wd4018> # disable "signed/unsigned mismatch"
$<$<CXX_COMPILER_ID:MSVC>:/wd4503> # disable "decorated name length exceeded, name was truncated"
$<$<CXX_COMPILER_ID:MSVC>:/wd4267> # disable "conversion from 'size_t' to 'int', possible loss of data"
$<$<CXX_COMPILER_ID:MSVC>:/wd4800> # forcing value to bool 'true' or 'false' (performance warning)
$<$<CXX_COMPILER_ID:MSVC>:/wd4244> # 4244: conversion, possible loss of data
$<$<CXX_COMPILER_ID:MSVC>:/wd4267> # 4267: conversion, possible loss of data
$<$<CXX_COMPILER_ID:MSVC>:/wd4800> # 4800: BOOL -> true or false
$<$<CXX_COMPILER_ID:MSVC>:/wd4996> # 4996: compiler encountered deprecated declaration
$<$<CXX_COMPILER_ID:MSVC>:/W3>
$<$<CXX_COMPILER_ID:MSVC>:/wd4018> # disable "signed/unsigned mismatch"
$<$<CXX_COMPILER_ID:MSVC>:/wd4503> # disable "decorated name length exceeded, name was truncated"
$<$<CXX_COMPILER_ID:MSVC>:/wd4267> # disable "conversion from 'size_t' to 'int', possible loss of data"
$<$<CXX_COMPILER_ID:MSVC>:/wd4800> # forcing value to bool 'true' or 'false' (performance warning)
$<$<CXX_COMPILER_ID:MSVC>:/wd4244> # 4244: conversion, possible loss of data
$<$<CXX_COMPILER_ID:MSVC>:/wd4267> # 4267: conversion, possible loss of data
$<$<CXX_COMPILER_ID:MSVC>:/wd4800> # 4800: BOOL -> true or false
$<$<CXX_COMPILER_ID:MSVC>:/wd4996> # 4996: compiler encountered deprecated declaration

# Injected from recent LLVM:
$<$<CXX_COMPILER_ID:MSVC>:/wd4141> # 'inline' used more than once
$<$<CXX_COMPILER_ID:MSVC>:/wd4146> # unary minus applied to unsigned type
$<$<CXX_COMPILER_ID:MSVC>:/wd4291> # No matching operator delete found
# Injected from recent LLVM:
$<$<CXX_COMPILER_ID:MSVC>:/wd4141> # 'inline' used more than once
$<$<CXX_COMPILER_ID:MSVC>:/wd4146> # unary minus applied to unsigned type
$<$<CXX_COMPILER_ID:MSVC>:/wd4291> # No matching operator delete found

# We could expose the /MP flag to all targets, but that might end up saturating the build
# since multiple MSBuild projects might get built in parallel, each of which compiling their
# source files in parallel; the Halide library itself is a "knot" point of the build graph,
# so compiling its files in parallel should not oversubscribe the system
$<$<CXX_COMPILER_ID:MSVC>:/MP>
)
# We could expose the /MP flag to all targets, but that might end up saturating the build
# since multiple MSBuild projects might get built in parallel, each of which compiling their
# source files in parallel; the Halide library itself is a "knot" point of the build graph,
# so compiling its files in parallel should not oversubscribe the system
$<$<CXX_COMPILER_ID:MSVC>:/MP>
)

target_compile_definitions(Halide
PRIVATE
Expand Down
2 changes: 0 additions & 2 deletions src/CodeGen_Hexagon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,6 @@ class SloppyUnpredicateLoadsAndStores : public IRMutator {

return Call::make(op->type, Call::if_then_else,
{condition, load, make_zero(op->type)}, Call::Intrinsic);

return load;
Comment on lines -286 to -287
Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch. I wonder if this was a bad merge. Probably worth turning on this warning explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yow. I'm genuinely surprised this is merely a warning.

} else {
// It's a predicated vector gather. Just scalarize. We'd
// prefer to keep it in a loop, but that would require
Expand Down
8 changes: 8 additions & 0 deletions src/Introspection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
#include "Introspection.h"

#if defined(_MSC_VER)
#undef WITH_INTROSPECTION
#elif defined(__has_include)
#if !__has_include(<execinfo.h>)
#undef WITH_INTROSPECTION
#endif
#endif

#ifdef WITH_INTROSPECTION

#include "Debug.h"
Expand Down
2 changes: 1 addition & 1 deletion src/LowerWarpShuffles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ class LowerWarpShuffles : public IRMutator {

Expr wild = Variable::make(Int(32), "*");
vector<Expr> result;
int bits;
int bits = 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason this isn't needed is extremely subtle: the function is_const_power_of_two_integer returns true iff it initializes bits. The first read of bits is guarded by short-circuit && below. ClangCL isn't clever enough to see this, and I don't blame it.

There's no downside to this (this is not performance-critical) and disabling uninitialized use warnings seems significantly worse.


// Move this_lane as far left as possible in the expression to
// reduce the number of cases to check below.
Expand Down
7 changes: 3 additions & 4 deletions src/Util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,8 @@ std::string get_env_variable(char const *env_var_name) {
if (lvl) {
return std::string(lvl);
}
#endif

return "";
#endif
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This emits an unreachable code warning.


string running_program_name() {
Expand Down Expand Up @@ -529,7 +528,7 @@ struct TickStackEntry {
int line;
};

vector<TickStackEntry> tick_stack;
Copy link
Member Author

Choose a reason for hiding this comment

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

Throws a warning about missing extern and that it should be static if module-local, which it is. No need to export this.

static vector<TickStackEntry> tick_stack;

void halide_tic_impl(const char *file, int line) {
string f = file;
Expand Down Expand Up @@ -609,7 +608,7 @@ void WINAPI generic_fiber_entry_point(LPVOID argument) {

void run_with_large_stack(const std::function<void()> &action) {
#if _WIN32
constexpr SIZE_T required_stack = 8 * 1024 * 1024;
constexpr auto required_stack = 8 * 1024 * 1024;

// Only exists for its address, which is used to compute remaining stack space.
ULONG_PTR approx_stack_pos;
Expand Down
2 changes: 1 addition & 1 deletion src/VectorizeLoops.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,4 +1672,4 @@ Stmt vectorize_loops(const Stmt &stmt, const map<string, Function> &env, const T
}

} // namespace Internal
} // namespace Halide
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the only file without a trailing newline.

} // namespace Halide
4 changes: 2 additions & 2 deletions src/WasmExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1586,11 +1586,11 @@ WasmModule WasmModule::compile(
#if !defined(WITH_WABT)
user_error << "Cannot run JITted WebAssembly without configuring a WebAssembly engine.";
return WasmModule();
#endif

#else
WasmModule wasm_module;
wasm_module.contents = new WasmModuleContents(module, arguments, fn_name, jit_externs, extern_deps);
return wasm_module;
#endif
}
Comment on lines -1589 to 1594
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to above: unreachable code.


/** Run generated previously compiled wasm code with a set of arguments. */
Expand Down