Skip to content
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.

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

Migration to C++17 #1152

Merged
merged 7 commits into from
Dec 20, 2022
Merged

Conversation

pruthvistony
Copy link
Collaborator

@pruthvistony pruthvistony commented Dec 20, 2022

cherry-pick of - pytorch#85969
and dependent patches.

Issue - https://ontrack-internal.amd.com/browse/SWDEV-374101

malfet and others added 7 commits December 19, 2022 18:24
Prep change for moving the codebase to C++17 standard
Was part of pytorch#85969

Pull Request resolved: pytorch#89605
Approved by: https://github.com/weiwangmeta, https://github.com/kit1980
Not sure why, but top-level `using namespace` directive causes VC++ fail with (if C++17 standard is used, but everything is fine with C++14):
```
C:\actions-runner\_work\pytorch\pytorch\third_party\pybind11\include\pybind11\detail\../pytypes.h(1520): error C2872: 'attr': ambiguous symbol
C:\actions-runner\_work\pytorch\pytorch\aten\src\ATen/core/interned_strings.h(349): note: could be 'c10::attr'
C:\actions-runner\_work\pytorch\pytorch\torch/csrc/jit/ir/ir.h(75): note: or       'torch::jit::attr'
C:\actions-runner\_work\pytorch\pytorch\cmake\..\third_party\pybind11\include\pybind11/pybind11.h(1094): note: see reference to function template instantiation 'pybind11::str pybind11::str::format<_Ty1&>(_Ty1 &) const' being compiled
        with
        [
            _Ty1=pybind11::handle
        ]
```

Solve this by replacing global `using namespace torch::jit;` with
specific usages of objects/methods from namespaces

Another prep change for https://github.com/pytorch/pytorch/70188

Pull Request resolved: pytorch#90228
Approved by: https://github.com/kit1980, https://github.com/albanD
`register` keyword is removed in C++17, but keeping it there under ifdef
as I have not measured the perf implication on older compiler, though
there shouldn't be any: all modern compilers supposed to downright
ignore it.

This code originates from facebookresearch/xformers#375 will propose similar PR to remove register keyword usage to that repo.

Yet another thing discovered while working on pytorch#85969

Pull Request resolved: pytorch#90389
Approved by: https://github.com/drisspg
Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/pytorch/pytorch/pull/90174).
* __->__ pytorch#90174

fix c10::detail::integer_iterator for C++17

Summary: std::iterator is deprecated.

Test Plan: Rely on CI.

Pull Request resolved: pytorch#90174
Approved by: https://github.com/clee2000, https://github.com/malfet
Get rid of std::iterator inheritance/references for `c10::DictIterator`, `c10::IListRefIterator` and `c10::ListIterator`

Followup after pytorch#90174

Fixes deprecation warning and extension compilation failures using VC++
that raises following errors:
```
C:\actions-runner\_work\pytorch\pytorch\build\win_tmp\build\torch\include\ATen/core/IListRef.h(517): error C4996: 'std::iterator<std::bidirectional_iterator_tag,T,ptrdiff_t,T *,T &>::value_type': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning.

C:\actions-runner\_work\pytorch\pytorch\build\win_tmp\build\torch\include\ATen/core/List.h(169): error C4996: 'std::iterator<std::random_access_iterator_tag,T,ptrdiff_t,T *,T &>::difference_type': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The <iterator> header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning.

```

Discovered while working on pytorch#85969
Pull Request resolved: pytorch#90379
Approved by: https://github.com/ezyang, https://github.com/dagitses
Set `cmake.dir` to `/usr/local` in `.circleci/scripts/build_android_gradle.sh `
Prep change for raising compiler standard to C++17: cmake-3.18 is the first one to support CUDA17 language

Pull Request resolved: pytorch#89570
Approved by: https://github.com/atalman
With CUDA-10.2 gone we can finally do it!

This PR mostly contains build system related changes, invasive functional ones are to be followed.
Among many expected tweaks to the build system, here are few unexpected ones:
 - Force onnx_proto project to be updated to C++17 to avoid `duplicate symbols` error when compiled by gcc-7.5.0, as storage rule for `constexpr` changed in C++17, but gcc does not seem to follow it
 - Do not use `std::apply` on CUDA but rely on the built-in variant, as it results in test failures when CUDA runtime picks host rather than device function when `std::apply` is invoked from CUDA code.
 - `std::decay_t` -> `::std::decay_t` and `std::move`->`::std::move` as VC++ for some reason claims that `std` symbol is ambigious
 - Disable use of `std::aligned_alloc` on Android, as its `libc++` does not implement it.

Some prerequisites:
 - pytorch#89297
 - pytorch#89605
 - pytorch#90228
 - pytorch#90389
 - pytorch#90379
 - pytorch#89570
 - facebookincubator/gloo#336
 - facebookincubator/gloo#343
 - pytorch/builder@919676f

Fixes pytorch#56055

Pull Request resolved: pytorch#85969
Approved by: https://github.com/ezyang, https://github.com/kulinseth
@pruthvistony
Copy link
Collaborator Author

pytorch Build is verified - http://rocm-ci.amd.com/job/mainline-framework-pytorch-ci/555/

@pruthvistony pruthvistony self-assigned this Dec 20, 2022
@pruthvistony pruthvistony changed the title Rocm5.5 internal testing c17 Migration to C++17 Dec 20, 2022
@pruthvistony pruthvistony merged commit 2a8a833 into rocm5.5_internal_testing Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants