Update C++ Standard from 14 to 17 by snnn · Pull Request #8041 · microsoft/onnxruntime

snnn · 2021-06-11T22:41:04Z

Description:

Switched the code to C++17. To build ONNX Runtime on old distros like CentOS 7, you need to install a newer GCC from additionary repos. If you build onnxruntime with the newer GCC, typically the result binary can't be distributed to other places because it depends on the new GCC's runtime libraries, something that the stock OS doesn't have. But on RHEL/CentOS, it can be better. We use Red Hat devtoolset 8/9/10 with CentOS7 building our code. The new library features(like std::filesystem) that not exists in the old C++ runtime will be statically linked into the applications with some restrictions:

GCC has dual ABI, but we can only use the old one. It means std::string is still copy-on-write and std::list::size() is still O(n). Also, if you build onnxruntime on CentOS 7 and link it with some binaries that were built on CentOS 8 or Ubuntu with the new ABI and export C++ symbols directly(instead of using a C API), the it won't work.
We still can't use std::optional. It is a limitation coming from macOS. We will solve it when we got macOS 11 build machines. It won't be too long.
Because CUDA 10.2 doesn't support C++17 and we still need to generate CUDA 10.2 packages, please avoid to use C++17 in CUDA files(*.cu). Also, the *.h files that they include(like core/framework/float16.h). You are welcome to use the new features in any *.cc files.

Also, update the minimum supported macOS version to 10.13.
Also, fix some compile warnings.

Motivation and Context

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

onnxruntime/core/optimizer/fast_gelu_fusion.cc

onnxruntime/core/optimizer/skip_layer_norm_fusion.cc

onnxruntime/core/providers/cpu/math/element_wise_ops.cc

onnxruntime/core/providers/cpu/tensor/slice.cc

tools/ci_build/build.py

cmake/CMakeLists.txt

edgchen1 · 2021-06-11T23:49:09Z

cmake/CMakeLists.txt


      target_compile_definitions(${target_name} PUBLIC -DNSYNC_ATOMIC_CPP11)
+      if(APPLE)
+        target_compile_definitions(${target_name} PRIVATE -Doptional_CONFIG_SELECT_OPTIONAL=1)


we can use the value optional_OPTIONAL_NONSTD, according to the docs: https://github.com/martinmoene/optional-lite#select-stdoptional-or-nonstdoptional

also, is ORT's optional.h a public header? include/onnxruntime/core/common/optional.h
if it may be included by external code, that code would need to configure it the same way. should it be a PUBLIC compile definition? #Closed

should it be PUBLIC instead of PRIVATE?

cmake/onnxruntime_providers.cmake

onnxruntime/core/optimizer/utils.h

edgchen1 · 2021-06-12T00:15:05Z

onnxruntime/core/providers/cuda/cuda_call.cc

      char* hostname = nullptr;
      if (-1 == _dupenv_s(&hostname, &hostname_len, "COMPUTERNAME"))
-        hostname = "?";
+        hostname = const_cast<char*>("?");


can we avoid const_cast? #Closed

I would prefer to leave it as a TODO as it is not related to C++ 14/17 upgrade. Currently this PR is already not small.

ok. TODO comment?

onnxruntime/python/onnxruntime_pybind_state.cc

onnxruntime/test/contrib_ops/tokenizer_test.cc

edgchen1 · 2021-06-25T16:24:52Z

include/onnxruntime/core/common/common.h

+#if ((__cplusplus >= 201703L) || (defined(_MSVC_LANG) && (_MSVC_LANG >= 201703L)))
+#define IF_CONSTEXPR if constexpr
+#else
+#define IF_CONSTEXPR if


IF_CONSTEXPR

why do we need a macro for this? #Closed

Because CUDA 10.2 doesn't support C++17 and we still need to generate CUDA 10.2 packages, please avoid to use C++17 in CUDA files(*.cu). Also, the *.h files that they include(like core/framework/float16.h). In these *.h files, we need to use such a macro until we retire the CUDA 10.2 pipelines.

ok, can we name it ORT_IF_CONSTEXPR?

include/onnxruntime/core/common/logging/macros.h

edgchen1 · 2021-06-25T16:55:24Z

onnxruntime/core/providers/cuda/reduction/reduction_functions.cc


    // otherwise, not contiguous
-    return nullopt;
+    return std::nullopt;


std::nullopt;

using std::nullopt with onnxruntime::optional here, can we use std or onnxruntime consistently? #Closed

edgchen1 · 2021-06-25T16:56:42Z

onnxruntime/core/providers/cpu/tensor/transpose.cc


-const auto type_constraints = BuildKernelDefConstraintsFromTypeList<DataTypes>();
-const auto enabled_type_constraints = BuildKernelDefConstraintsFromTypeList<EnabledDataTypes>();
+#define type_constraints (BuildKernelDefConstraintsFromTypeList<DataTypes>())


type_constraints

replace at call site instead of macro #Closed

edgchen1 · 2021-06-25T17:05:01Z

onnxruntime/test/providers/cuda/reduction_functions_test.cc

         ApplicableMatrixReduction expected_reduction,
-         const optional<int>& expected_m = nullopt,
-         const optional<int>& expected_n = nullopt) {
+         const optional<int>& expected_m = std::nullopt,


std::nullopt

would prefer to be consistent, e.g., nullopt
alternatively, this can be {} and it won't need to change when we move to std::optional #Closed

edgchen1 · 2021-06-25T17:20:17Z

orttraining/orttraining/test/gradient/gradient_ops_test.cc

 template <typename T>
 static std::vector<std::vector<T>> GetRandomValuesForMaxPool(const std::vector<TensorInfo>& infos) {
  std::vector<std::vector<T>> datas(infos.size());
+  std::random_device rd;


std::random_device rd;

can we use generator initialized with GetTestRandomSeed()? then it's easier to reproduce failures #Closed

pranavsharma · 2021-06-25T19:53:24Z

onnxruntime/core/framework/execution_frame.cc

 }

-static Status AllocateSparseTensor(MLValue& mlvalue, const DataTypeImpl& ml_type, AllocatorPtr allocator,
+static Status AllocateSparseTensor(OrtValue& mlvalue, const DataTypeImpl& ml_type, AllocatorPtr allocator,


Since you've made this change in a number of places, should we also rename the file ml_value.h to ort_value.h? The filename doesn't match the class it houses. Can be done in a separate PR.

Sure. I will do it in another PR.

pranavsharma · 2021-06-25T20:07:13Z

include/onnxruntime/core/common/common.h

 inline std::string ToWideString(const std::string& s) { return s; }
 #endif

+#if ((__cplusplus >= 201703L) || (defined(_MSVC_LANG) && (_MSVC_LANG >= 201703L)))


If we're moving to C++17 for good, why do we need this condition?

Because CUDA 10.2 doesn't support C++17 and we still need to generate CUDA 10.2 packages, we must avoid to use C++17 in CUDA files(*.cu). Also, the *.h files that they include(like core/framework/float16.h).

Ok. thanks. Would be good to document this somewhere.

edgchen1

snnn requested a review from a team as a code owner June 11, 2021 22:41