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

Bump min cxx standard to 17 #50

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

oandreeva-nv
Copy link
Contributor

@oandreeva-nv oandreeva-nv commented Dec 19, 2023

This PR is one of the series PRs to update Triton to C++17 standard.

Here if TRITON_MIN_CXX_STANDARD is not specified in cache, it will be set to 17.

The main discussion point is if we even need THIRD_PARTY_CMAKE_CXX_STANDARD or we can simply use TRITON_MIN_CXX_STANDARD instead.

Current implementation:

Then if CMAKE_CXX_STANDARD is not set, THIRD_PARTY_CMAKE_CXX_STANDARD is set to TRITON_MIN_CXX_STANDARD, otherwise, THIRD_PARTY_CMAKE_CXX_STANDARD is set to CMAKE_CXX_STANDARD.

Note, that by the time third party is going to be build, TRITON_MIN_CXX_STANDARD and CMAKE_CXX_STANDARD should already be set in CMakeCache (server or core)

@oandreeva-nv oandreeva-nv changed the title Passing CXX standard to all third party libraries Bump min cxx standard to 17 Dec 19, 2023
@oandreeva-nv oandreeva-nv marked this pull request as ready for review December 26, 2023 21:36
CMakeLists.txt Outdated
# [FIXME] apply the same version to all third party, and pick a Triton default
# C++ standard
set(TRITON_MIN_CXX_STANDARD 17 CACHE STRING "The minimum C++ standard whose features are requested to build this target.")

if (NOT DEFINED CMAKE_CXX_STANDARD)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still check CMAKE_CXX_STANDARD if we are introducing a new variable? I think CMAKE_CXX_STANDARD is set in server CMakeLists and can be replaced by setting the new variable directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we set CMAKE_CXX_STANDARD anywhere. I can remove check for CMAKE_CXX_STANDARD and go with TRITON_MIN_CXX_STANDARD only

@Tabrizian
Copy link
Member

The main discussion point is if we even need THIRD_PARTY_CMAKE_CXX_STANDARD or we can simply use TRITON_MIN_CXX_STANDARD instead.

I feel like TRITON_MIN_CXX_STANDARD and THIRD_PARTY_CMAKE_CXX_STANDARD serve different purposes but I agree that we should get rid of the THIRD_PARTY_CMAKE_CXX_STANDARD. I think we can probably just use CMAKE_CXX_STANDARD and make sure to provide a default value when using build.py script.

@oandreeva-nv oandreeva-nv requested a review from GuanLuo January 3, 2024 18:28
@@ -180,7 +181,7 @@ ExternalProject_Add(absl
EXCLUDE_FROM_ALL ON
DOWNLOAD_COMMAND ""
CMAKE_CACHE_ARGS
-DCMAKE_CXX_STANDARD:STRING=${THIRD_PARTY_CMAKE_CXX_STANDARD}
-DCMAKE_CXX_STANDARD:STRING=${TRITON_MIN_CXX_STANDARD}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be propagating CMAKE_CXX_STANDARD rather than TRITON_MIN_CXX_STANDARD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please elaborate? Since for this set of PRs, I introduce TRITON_MIN_CXX_STANDARD, so I use it to set CMAKE_CXX_STANDARD for other third_party packages (note: not all of them take it).

Copy link
Member

Choose a reason for hiding this comment

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

My understanding was that TRITON_MIN_CXX_STANDARD determines the minimum required standard when compiling the project (i.e., since we'll start using C++ X version features). CMAKE_CXX_STANDARD determines the version that the server is actually compiled with. Based on this understanding, I think we should propagate CMAKE_CXX_STANDARD and not TRITON_MIN_CXX_STANDARD. Feel free to correct me if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. However, there are multiple ways how to specify c++ standard for compilation. In third_party library I set each library's CMAKE_CXX_STANDARD to be equal to TRITON_MIN_CXX_STANDARD, since TRITON_MIN_CXX_STANDARD is used throughout our triton libraries.

I am not sure if it makes sense to explicitly set CMAKE_CXX_STANDARD here, since we do not compile any custom code, and third parties will use provided CMAKE_CXX_STANDARD through -DCMAKE_CXX_STANDARD:STRING=${TRITON_MIN_CXX_STANDARD}.

In other repositories, target_compile_features(... PRIVATE cxx_std_${TRITON_MIN_CXX_STANDARD}) will make sure that libraries are compiled at least with the provided standard, and if newer standard is needed, then compiler will add appropriate flag, eg -std=gnu++20.

Since I don't introduce hard set of CMAKE_CXX_STANDARD throughout Triton's server, core, backend, common libraries, I didn't see the reason why we would need to set it here. And my logic was, since Triton requires at least TRITON_MIN_CXX_STANDARD standard, third parties should be compiled with the same standard as minimum required. Note, that some third-parties do not take CMAKE_CXX_STANDARD.

Copy link
Member

Choose a reason for hiding this comment

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

I'll be approving the PR but still to me it seems like the correct thing would be to propagate CMAKE_CXX_STANDARD. I think the interface for specifying the CMAKE standard is CMAKE_CXX_STANDARD. IMO, TRITON_MIN_CXX_STANDARD is an internal variable that determines the minimum required standard but CMAKE_CXX_STANDARD should be greater than or equal to TRITON_MIN_CXX_STANDARD.

Tabrizian
Tabrizian previously approved these changes Jan 9, 2024
@oandreeva-nv oandreeva-nv merged commit 5e93990 into main Jan 11, 2024
2 checks passed
@oandreeva-nv oandreeva-nv deleted the oandreeva_upgrade_cxx_standard_17 branch January 11, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants