Skip to content

Comments

Enable C++ 20 standard globally#1257

Merged
marty-johnson59 merged 10 commits intoKhronosGroup:mainfrom
SaschaWillems:cpp_20
Mar 24, 2025
Merged

Enable C++ 20 standard globally#1257
marty-johnson59 merged 10 commits intoKhronosGroup:mainfrom
SaschaWillems:cpp_20

Conversation

@SaschaWillems
Copy link
Collaborator

@SaschaWillems SaschaWillems commented Jan 15, 2025

Description

Note: This is work-in-progress and not ready for review yet. I didn't set it to "draft", as I wanted to test CI to see where things break with C++ 20.

Refs #1201

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

  • I did a full batch run using the batch command line argument to make sure all samples still work properly

Old style no longer worked due to C++20 compile time requirements
@SaschaWillems
Copy link
Collaborator Author

Once we merge this, I think we can remove one or two external dependencies (submodules). Right now. printing formatted messages (e.g. errors) is knda complicated and goes through our framework and two externeal dependencies. One of those is "fmt", and it seems that it's functionality is also present in the STL with C++20. So we could get rid of "fmt" and maybe also "spdlog". I don't think we need to have such a complicated setup just for logging.

jackk18912525

This comment was marked as spam.

@SaschaWillems
Copy link
Collaborator Author

@jackk18912525: Can you please remove your spam comment and also remove yourself from the reviewers?

@SaschaWillems
Copy link
Collaborator Author

A blocker is the HCWPipe third party dependency. I can't get it to compile after updating to C++20, and updating the submodule doesn't help either (esp.a s the library is now differently named). I need help on this from someone from arm, maybe @JoseEmilio-ARM?

@JoseEmilio-ARM JoseEmilio-ARM removed the request for review from jackk18912525 January 27, 2025 16:28
@JoseEmilio-ARM JoseEmilio-ARM dismissed jackk18912525’s stale review January 27, 2025 16:40

Submitted accidentally

@SaschaWillems
Copy link
Collaborator Author

@JoseEmilio-ARM : Any update on this? I'm stuck until someone with proper knowledge updates HCWPipe.

@JoseEmilio-ARM JoseEmilio-ARM mentioned this pull request Feb 9, 2025
13 tasks
@JoseEmilio-ARM
Copy link
Collaborator

@JoseEmilio-ARM : Any update on this? I'm stuck until someone with proper knowledge updates HCWPipe.

Hi @SaschaWillems , please try again with the changes in #1273

@SaschaWillems
Copy link
Collaborator Author

Awesome. Thank you very much :)

# Conflicts:
#	framework/platform/platform.cpp
@CLAassistant
Copy link

CLAassistant commented Feb 25, 2025

CLA assistant check
All committers have signed the CLA.

@SaschaWillems
Copy link
Collaborator Author

SaschaWillems commented Feb 25, 2025

Builds are still failing for me on Android. Seems the HCWStatsProvider needs to be updated to work with C++20:

/home/runner/work/Vulkan-Samples/Vulkan-Samples/framework/stats/hwcpipe_stats_provider.cpp:47:14: error: no matching constructor for initialization of 'StatDataMap' (aka 'unordered_map<StatIndex, StatData, StatIndexHash>')
2025-02-25T16:05:45.9780418Z      47 |         StatDataMap hwcpipe_stats = {
2025-02-25T16:05:45.9781083Z         |                     ^               ~
2025-02-25T16:05:45.9781771Z      48 |             {StatIndex::gpu_cycles,            {hwcpipe_counter::MaliGPUActiveCy}},
2025-02-25T16:05:45.9782517Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9783593Z      49 |             {StatIndex::gpu_vertex_cycles,     {hwcpipe_counter::MaliNonFragQueueActiveCy, {MaliNonFragActiveCy, MaliBinningQueueActiveCy} }},
2025-02-25T16:05:45.9785185Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9785969Z      50 |             {StatIndex::gpu_load_store_cycles, {hwcpipe_counter::MaliLSIssueCy}},
2025-02-25T16:05:45.9786661Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9787351Z      51 |             {StatIndex::gpu_tiles,             {hwcpipe_counter::MaliFragTile}},
2025-02-25T16:05:45.9787997Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9788683Z      52 |             {StatIndex::gpu_killed_tiles,      {hwcpipe_counter::MaliFragTileKill}},
2025-02-25T16:05:45.9789578Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9790788Z      53 |             {StatIndex::gpu_fragment_cycles,   {hwcpipe_counter::MaliFragQueueActiveCy, {MaliFragActiveCy, MaliMainQueueActiveCy}}},
2025-02-25T16:05:45.9791865Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9792759Z      54 |             {StatIndex::gpu_fragment_jobs,     {hwcpipe_counter::MaliFragQueueJob, {MaliMainQueueJob}}},
2025-02-25T16:05:45.9793575Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9794289Z      55 |             {StatIndex::gpu_ext_reads,         {hwcpipe_counter::MaliExtBusRdBt}},
2025-02-25T16:05:45.9794949Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9795639Z      56 |             {StatIndex::gpu_ext_writes,        {hwcpipe_counter::MaliExtBusWrBt}},
2025-02-25T16:05:45.9796294Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9796994Z      57 |             {StatIndex::gpu_ext_read_stalls,   {hwcpipe_counter::MaliExtBusRdStallCy}},
2025-02-25T16:05:45.9797691Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9798399Z      58 |             {StatIndex::gpu_ext_write_stalls,  {hwcpipe_counter::MaliExtBusWrStallCy}},
2025-02-25T16:05:45.9799103Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9799785Z      59 |             {StatIndex::gpu_ext_read_bytes,    {hwcpipe_counter::MaliExtBusRdBy}},
2025-02-25T16:05:45.9800443Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9801325Z      60 |             {StatIndex::gpu_ext_write_bytes,   {hwcpipe_counter::MaliExtBusWrBy}},
2025-02-25T16:05:45.9802002Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2025-02-25T16:05:45.9811329Z      61 |             {StatIndex::gpu_tex_cycles,        {hwcpipe_counter::MaliTexIssueCy}}};
2025-02-25T16:05:45.9812044Z         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Any idea on how to fix this?

@gpx1000
Copy link
Collaborator

gpx1000 commented Mar 11, 2025

Sorry Sascha, I meant to do that change on my branch and offer it as a PR. my troubles with CLion and Android Studio's github plugin continue to annoy. The change to the constructor in that class seems to allow Android to build locally. I don't want to step on your toes, so please back it out if you don't like it. I'm happy to fix the clang format and copyright header checks, but again, don't want to step on your toes; just wanted to make a suggestion for help.

@SaschaWillems
Copy link
Collaborator Author

No worries. That's perfectly fine. And thank you very much for taking care of this, I couldn't figure out how to fix this 👍🏻

@gpx1000
Copy link
Collaborator

gpx1000 commented Mar 11, 2025

I'm always happy to bang my head against a problem till it works ;-)

That said, I've been playing around locally with your branch on adding modules... that's... well it'll take a bit from what I can tell. At least CMake has most of the support. It should help a LOT with compile times. But the changes are getting gnarly. Anyway, I think this PR has all changes necessary for C++20, so that's most of the problem.

@SaschaWillems SaschaWillems added the framework This is relevant to the framework label Mar 11, 2025
@SaschaWillems
Copy link
Collaborator Author

So with Steven's fix this now builds on all platforms and is ready for review :)

gpx1000
gpx1000 previously approved these changes Mar 11, 2025
@gary-sweet
Copy link
Contributor

This does seem to run ok for me, but I am seeing some build warnings from the third party code with this change (using gcc-12.3-1.0 on aarch64):

In file included from third_party/catch2/src/catch2/benchmark/detail/catch_stats.cpp:10:
third_party/catch2/src/catch2/../catch2/benchmark/detail/catch_stats.hpp: In instantiation of 'Catch::Benchmark::Estimate<double> Catch::Benchmark::Detail::bootstrap(double, Iterator, Iterator, const sample&, Estimator&&) [with Iterator = __gnu_cxx::__normal_iterator<double*, std::vector<double> >; Estimator = double (*&)(__gnu_cxx::__normal_iterator<double*, std::vector<double> >, __gnu_cxx::__normal_iterator<double*, std::vector<double> >); sample = std::vector<double>]':
third_party/catch2/src/catch2/benchmark/detail/catch_stats.cpp:244:37:   required from here
third_party/catch2/src/catch2/../catch2/benchmark/detail/catch_stats.hpp:99:118: note: parameter passing for argument of type 'std::pair<double, double>' when C++17 is enabled changed to match C++14 in GCC 10.1
   99 |                 std::tie(sum_squares, sum_cubes) = std::accumulate(jack.begin(), jack.end(), std::make_pair(0., 0.), [jack_mean](std::pair<double, double> sqcb, double x) -> std::pair<double, double> {
      |                                                                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  100 |                     auto d = jack_mean - x;
      |                     ~~~~~~~~~~~~~~~~~~~~~~~                                                                           
  101 |                     auto d2 = d * d;
      |                     ~~~~~~~~~~~~~~~~                                                                                  
  102 |                     auto d3 = d2 * d;
      |                     ~~~~~~~~~~~~~~~~~                                                                                 
  103 |                     return { sqcb.first + d2, sqcb.second + d3 };
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                     
  104 |                 });
      |                 ~     

This one pops up 15 times:

In file included from third_party/ktx/other_include/glm/detail/type_half.hpp:19,
                 from third_party/ktx/other_include/glm/detail/func_packing.inl:5,
                 from third_party/ktx/other_include/glm/detail/func_packing.hpp:168,
                 from third_party/ktx/other_include/glm/packing.hpp:6,
                 from third_party/ktx/other_include/glm/glm.hpp:84,
                 from framework/common/glm_common.h:26,
                 from framework/common/helpers.h:39,
                 from framework/core/descriptor_set_layout.h:20,
                 from framework/core/hpp_descriptor_set_layout.h:20,
                 from framework/core/command_buffer.h:23,
                 from framework/gui.h:29,
                 from framework/platform/plugins/plugin.h:28,
                 from app/plugins/plugins.h:20,
                 from app/plugins/plugins.cpp:20:
third_party/ktx/other_include/glm/detail/type_half.inl: In function 'float glm::detail::overflow()':
third_party/ktx/other_include/glm/detail/type_half.inl:12:27: warning: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile]
   12 |                         f *= f; // this will overflow before the for loop terminates
      |                         ~~^~~~

@SaschaWillems
Copy link
Collaborator Author

Thanks for bringing that up. Catch2 is something we want to get rid of anyway (see #1303). As for KTX: we may want to update that in another PR, I guess it's using it's own outdated version of glm.

@marty-johnson59 marty-johnson59 merged commit cb6e274 into KhronosGroup:main Mar 24, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework This is relevant to the framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants