Require C++17 for Halide. #5282

steven-johnson · 2020-09-18T21:31:33Z

As of Halide 13, we require C++17 to build or use Halide. (We previously claimed to require C++11, although Make builds typically ended up using C++14 due to llvm-config --cxxflags specifying it.)

Fixes #4216

steven-johnson · 2020-09-22T19:37:08Z

The Linux failures appear to be a latent, pre-existing bug that compiler changes have revealed. Stay tuned...

…0 committer Steven Johnson <[email protected]> 1600973808 -0700 Require C++17 for Halide. (We previously required C++11, although Make builds typically ended up using C++14 due to `llvm-config --cxxflags` specifying it.)

steven-johnson · 2020-09-24T19:18:04Z

No rush to land this (we probably want to wait until after the release/11.x branch is created) but marking 'ready for review' so that people can review/comment at their leisure.

abadams · 2020-09-24T19:59:19Z

apps/bgu/CMakeLists.txt

 set(CMAKE_CXX_STANDARD_REQUIRED YES)
 set(CMAKE_CXX_EXTENSIONS NO)

+# MSVC won't set __cplusplus correctly unless this flag is also set


Is there no way to hide this idiocy from our users?

We reported this to the CMake folks and they insisted they couldn't change it without a 'policy' flag to enable it, because existing (broken) code might rely on this (insanely broken) behavior, so they aren't gonna change it anytime soon.

The alternative that most people seem to end up using is to combine sniffing of __cplusplus with sniffing of _MSC_LANG, which would work I guess, but is just so f***ing wrong that I didn't want to do it, just on principle. Maybe that's a more pragmatic approach than requiring the build files to insert extra things to work around bad decisions by both MSVC and CMake.

Can we move the setting onto our side of the fence somehow, instead of in the user CMakeLists?

Well, yes and no.

We currently don't need to check __cplusplus at all (there's a reality check in Codegen_LLVM.cpp but it is just as a backstop).

If we only used C++17 features internal to libHalide, then we wouldn't need this.

If we eventually use a C++17 feature in headers exposed to user code (e.g. in templates), though, then we'd definitely have this problem (ie the user code's build system would have to set this flag).

I guess the better answer is to bite the bullet and accept that checks of that sort will have to check both __cplusplus and _MSC_LANG for the foreseeable future, which is just a sorry state of affairs.

(fix pushed)

For some reason, these are only getting flagged under C++17 builds, but they are legit (minor) issues we want to fix.

@alexreinking

v2.4.3 can generate a lot of compiler warnings under C++17; v2.5 fixes there. Note 1: I am unsure about the issues with keeping in sync with Ubuntu 20.04; tagging @alexreinking for comments Note 2: the current version of PyBind11 is actually v2.6, but it has many more changes and upgrading looks nontrivial; deliberately holding off on that upgrade for now.

steven-johnson · 2021-06-02T22:59:47Z

So now that Halide 12 is done, are we ready to land this? Are there any major downstream stakeholders that have any objections?

alexreinking · 2021-06-03T02:22:42Z

README.md

+(Note that *using* Halide requires C++11 or later, but *building* Halide
+requires C++17.)
+


This needs to be fixed now that we decided to go C++17 wholesale.

src/Generator.h

alexreinking · 2021-06-03T02:24:34Z

src/Generator.h

+    inline void build_inputs(std::vector<std::vector<StubInput>> &inputs, int idx) {
+        internal_assert(idx == (int)inputs.size());
+    }
+
+    template<typename Last>
+    inline void build_inputs(std::vector<std::vector<StubInput>> &inputs, int idx, const Last &last) {
+        inputs[idx] = build_input(idx, last);
+    }
+
+    template<typename First, typename Second, typename... Rest>
+    inline void build_inputs(std::vector<std::vector<StubInput>> &inputs, int idx, const First &first, const Second &second, const Rest &...rest) {
+        build_inputs<First>(inputs, idx, first);
+        build_inputs<Second, Rest...>(inputs, idx + 1, second, rest...);


Since we're going full C++17, this can be reverted, right?

dependencies/llvm/CMakeLists.txt

src/CodeGen_LLVM.cpp

steven-johnson · 2021-06-07T18:56:40Z

All green (again). Are we ready to land this?

abadams · 2021-06-07T19:12:55Z

test/correctness/interpreter.cpp

@@ -136,6 +136,7 @@ int main(int argc, char **argv) {



This file doesn't seem related?

...huh, weird, will revert

abadams · 2021-06-07T19:13:26Z

No objections here.

alexreinking · 2021-06-07T19:30:05Z

README_cmake.md

@@ -558,7 +558,7 @@ immediately after setting the minimum version.

 The next three variables set the project-wide C++ standard. The first,
 [`CMAKE_CXX_STANDARD`][cmake_cxx_standard], simply sets the standard version.
-Halide requires at least C++11. The second,
+Halide requires at least c++17. The second,


@steven-johnson - can you fix this one little thing before merging? No need to wait on buildbots for this, obviously.

abadams · 2021-06-08T16:56:31Z

We have discussed this quite a lot at dev meetings, so I think it's good to just land at this point without waiting for explicit approvals from everyone. It can always be rolled back if someone has a serious complaint.

steven-johnson force-pushed the srj/c++17 branch from 8ab7809 to 33b96df Compare September 24, 2020 18:57

steven-johnson force-pushed the srj/c++17 branch from 33b96df to b6ddbc1 Compare September 24, 2020 19:17

steven-johnson marked this pull request as ready for review September 24, 2020 19:17

steven-johnson requested review from abadams and jrk and removed request for jrk and abadams September 24, 2020 19:18

abadams reviewed Sep 24, 2020

View reviewed changes

Check _MSVC_LANG

461c7d5

abadams approved these changes Sep 24, 2020

View reviewed changes

Merge branch 'master' into srj/c++17

57918d8

steven-johnson mentioned this pull request Sep 30, 2020

Modernize C++ in runtime code #5306

Open

steven-johnson added 2 commits September 30, 2020 16:13

Merge branch 'master' into srj/c++17

6f7c679

Merge branch 'master' into srj/c++17

b0400c3

alexreinking added this to the v12.0.0 milestone Oct 16, 2020

steven-johnson added 12 commits November 2, 2020 10:10

Merge branch 'master' into srj/c++17

ffc8a26

Fix for trunk LLVM

fd4a18e

fixes

83bd779

Fix various clang-tidy issues

8be3b6a

For some reason, these are only getting flagged under C++17 builds, but they are legit (minor) issues we want to fix.

Merge branch 'srj/tidyness' into srj/c++17

b16603e

Merge remote-tracking branch 'origin/srj/llvm-fix' into srj/c++17

9154de1

Update HalideTraceViz.cpp

6b6282f

Update HalideTraceViz.cpp

f3b33f2

more fixes

a88c088

Merge branch 'srj/tidyness' into srj/c++17

e320f3f

Update FindCalls.cpp

6cf73b6

alexreinking added 2 commits May 24, 2021 15:50

Use simpler method to set default BUILD_SHARED_LIBS

da9b812

make sure C++17 requirement propagates to libHalide consumers

232aead

steven-johnson requested review from alexreinking, abadams and BachiLi June 2, 2021 22:59

alexreinking requested changes Jun 3, 2021

View reviewed changes

Merge branch 'master' into srj/c++17

cd6c8da

steven-johnson commented Jun 3, 2021

View reviewed changes

dependencies/llvm/CMakeLists.txt Show resolved Hide resolved

steven-johnson commented Jun 3, 2021

View reviewed changes

src/CodeGen_LLVM.cpp Show resolved Hide resolved

steven-johnson added 9 commits June 3, 2021 09:52

Fixes

b50bedc

Update apps, tutorials, etc

377c2b6

Add user-facing C++ version check

1eb8b18

Force tflite to build in C++17 mode.

4b5091f

Oops

f0d717a

Experimental fix for hannk

354eff3

More fix attempts

ff1e67c

Still more

c8fd401

Update CMakeLists.txt

abce80a

abadams reviewed Jun 7, 2021

View reviewed changes

alexreinking approved these changes Jun 7, 2021

View reviewed changes

steven-johnson added 3 commits June 7, 2021 12:36

Merge branch 'master' into srj/c++17

2e48574

Update README_cmake.md

f1a84b8

Revert unrelated change

64d1cda

steven-johnson merged commit cdac77b into master Jun 8, 2021

steven-johnson deleted the srj/c++17 branch June 8, 2021 17:05

alexreinking removed the release_notes For changes that may warrant a note in README for official releases. label Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require C++17 for Halide. #5282

Require C++17 for Halide. #5282

steven-johnson commented Sep 18, 2020 •

edited

Loading

steven-johnson commented Sep 22, 2020

steven-johnson commented Sep 24, 2020

abadams Sep 24, 2020

steven-johnson Sep 24, 2020

abadams Sep 24, 2020

steven-johnson Sep 24, 2020

steven-johnson Sep 24, 2020

steven-johnson commented Jun 2, 2021

alexreinking Jun 3, 2021

alexreinking Jun 3, 2021

steven-johnson commented Jun 7, 2021

abadams Jun 7, 2021

steven-johnson Jun 7, 2021

abadams commented Jun 7, 2021

alexreinking Jun 7, 2021

abadams commented Jun 8, 2021

		(Note that using Halide requires C++11 or later, but building Halide
		requires C++17.)

Require C++17 for Halide. #5282

Require C++17 for Halide. #5282

Conversation

steven-johnson commented Sep 18, 2020 • edited Loading

steven-johnson commented Sep 22, 2020

steven-johnson commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadams commented Jun 7, 2021

Choose a reason for hiding this comment

abadams commented Jun 8, 2021

steven-johnson commented Sep 18, 2020 •

edited

Loading