Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Backports C++17/20 <chrono> features to C++14 #45

Merged
merged 65 commits into from
Nov 20, 2020

Conversation

jrhemstad
Copy link
Collaborator

@jrhemstad jrhemstad commented Oct 9, 2020

Depends on #44

  • Moves <chrono> features guarded to C++17/20 to C++14

  • Adds missing _LIBCUDACXX_INLINE_VISIBILITY functions in <chrono> to ensure they are annotated with __host__ __device__

  • Backports libc++ std/utilities/time tests that were previously only tested in C++17/20 to be tested with C++14

  • Added explicit casts to operator- for weekday to workaround an NVCC bug

  • Removed the _LIBCUDACXX_HAS_NO_CXX20_CHRONO_LITERALS definition since the literals were backported This turned out to not be possible because compilers warn on the "user" defined literal.

Porting the tests required a few changes/workarounds to upstream tests:

  • Updated tests to build with --expt-relaxed-constexpr to allow accessing constexpr globals from <chrono> in device code

  • nvcc does not support ODR-user of constexpr global variables (e.g., cuda::std::chrono::March). To workaround, in several tests I created a constexpr local copy of the global, e.g.,

__device__ void odr_use_month(cuda::std::chrono::month const& m);

__device__ void foo(){
   // odr_use_month(cuda::std::March); // This won't work
   auto constexpr March = cuda::std::March;
   odr_use_month(March); // this works
}
  • Rename operator[].pass.cpp to operator_index.pass.cpp because nvcc doesn't like square brackets in a file name

  • Marked the following tests as XFAIL with gcc below 7.0 due to gcc bug:

    • time/time.cal/time.cal.ymdlast/time.cal.ymdlast.members/plus_minus_equal_month.pass.cpp
    • time/time.cal/time.cal.ymdlast/time.cal.ymdlast.members/plus_minus_equal_year.pass.cpp
    • time/time.cal/time.cal.ymwd/time.cal.ymwd.members/plus_minus_equal_month.pass.cpp
    • time/time.cal/time.cal.ymwd/time.cal.ymwd.members/plus_minus_equal_year.pass.cpp

Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

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

How many tests are there that require relaxed constexpr? Could we perhaps have a mode where we don't enable the flag, add a feature that let's tests know it's not added, and XFAIL or UNSUPPORTED them when the flag is not on?

@brycelelbach any opinions on that? Another option would be to wrap the offending code in the check for relaxed constexpr, but I think we talked about that and discarded it as an option previously?

Otherwise this looks good, but I'd like to resolve the question above first. I have a feeling that without a mode without that flag, we may accidentally brick NVRTC completely by introducing a constexpr function without an annotation somewhere; I want to make sure we don't accidentally force end users to add NVRTC flags that shouldn't be necessary. (NVCC users should be able to deal with it, maybe just not be able to use some functionality on the device side - NVRTC however errors when it merely parses a host definition, so NVRTC users would just be bricked if we miss something because of this flag.)


#include "test_macros.h"

#if TEST_STD_VER > 14
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is consciously C++17 only? Or you just missed it? (Same in the next file.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, looks like I missed it. I wasn't as diligent about checking test files for pre-processor C++ version checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I looked into this further. These tests are guarded with TEST_STD_VER > 14 because they are testing functionality that wasn't made constexpr until C++17, e.g., https://en.cppreference.com/w/cpp/chrono/time_point/operator_arith

It doesn't appear there's any language reason these operations can't be made constexpr in C++14. Are we comfortable with backporting the constexprness of these operations from C++17 to C++14?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need them be constexpr in C++14? I think it'd be fine if there's nothing C++17 specific in that code, given we are already backporting parts of this, but I'm unsure here.

Copy link
Collaborator 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 have a strong need for this to be constexpr, but if we're already backporting, might as well?

.upstream-tests/utils/nvidia/linux/perform_tests.bash Outdated Show resolved Hide resolved
libcxx/include/chrono Outdated Show resolved Hide resolved
libcxx/include/chrono Show resolved Hide resolved
@brycelelbach
Copy link
Collaborator

Yah, let's not assume relaxed constexpr. It's fine to have some separate tests that are relaxed constexpr specific.

@jrhemstad
Copy link
Collaborator Author

Yah, let's not assume relaxed constexpr. It's fine to have some separate tests that are relaxed constexpr specific.

Sure, how do I configure it so that only some tests are compiled with relaxed constexpr?

@jrhemstad
Copy link
Collaborator Author

So it turns out that we don't need --expt-relaxed-constexpr for any of the tests after all. You can access constexpr globals w/o the relaxed constexpr.

https://godbolt.org/z/Y9f4fq

I originally had thought it was necessary because the tests were ODR using constexpr globals, but that isn't allowed even with --expt-relaxed-constexpr. My WAR of using a constexpr local variable looks to have eliminated the need for relaxed constexpr at all.

Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

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

LGTM, I'll start doing CI runs on your changes today.

@jrhemstad jrhemstad force-pushed the port-time-cal-tests branch 2 times, most recently from 992563e to 5935716 Compare October 28, 2020 20:17
@brycelelbach brycelelbach added this to the 1.4.0 milestone Oct 29, 2020
Previously I had consolidated _LIBUCDACXX_STD_VER > 11
version checks, but neglected that this ended up guarding
the closing of the chrono namespace. Therefore, the
chrono namespace was never properly closed in C++11
mode.
Explicitly using the cuda::std:: namespace to access
the February global breaks libc++. Changed to use the
unqualified name and store it into a reserved name local.
Also added a comment on why this is necessary.
@griwes
Copy link
Collaborator

griwes commented Nov 17, 2020

Internal CI launched on CL 29332841.

MSVC complained about the possible loss of precision
casting an unsigned to an unsigned char.
@griwes
Copy link
Collaborator

griwes commented Nov 18, 2020

Relaunched internal CI on CL 29338478.

@griwes
Copy link
Collaborator

griwes commented Nov 19, 2020

Relaunched internal CI on CL 29342510.

@griwes griwes merged commit 753a0dd into NVIDIA:main Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants