search

kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
403 stars 67 forks source link

Enable CTAD by default for GCC 9 #222

Closed mhoemmen closed 1 year ago

mhoemmen commented 1 year ago

The implementation currently disables CTAD by default for GCC < 11.

https://github.com/youyu3/mdspan/blob/d4ba2621d097d31c85399f16b2976b3643bce08e/include/experimental/__p0009_bits/config.hpp#L220

However, it is known to work for GCC 9.2.0. @youyu3 confirmed that

Thus, we should consider enabling CTAD by default for GCC 9, and only disabling it by default for GCC < 9 or GCC 10.

youyu3 commented 1 year ago

To clarify, I only tested with https://github.com/kokkos/mdspan/blob/stable/examples/godbolt_starter/godbolt_starter.cpp.

crtrott commented 1 year ago

Did you test all other GCC 9.x compilers? I.e. do they work all?

youyu3 commented 1 year ago

My machine only has GCC 9.1, 9.2 and 9.3. It worked for all of them. I'll need to install GCC 9.4 and 9.5 to test them.

adah1972 commented 1 year ago

I do not know what made Bryce @brycelelbach say "GCC 10's CTAD seems sufficiently broken to prevent its use" in commit 972a4bcb, but currently CTAD seems to work well in all C++17-supporting GCC versions. The following command works well:

g++ -std=c++17 -D_MDSPAN_USE_CLASS_TEMPLATE_ARGUMENT_DEDUCTION \
    -Iinclude examples/godbolt_starter/godbolt_starter.cpp

I tested with the following GCC versions:

The comment in commit 2141453 mentioned a GCC bug 100983, which was fixed only in GCC 12. However, the current mdspan code does not seem to rely on the absence of this bug.

So I would suggest removing the (defined(_MDSPAN_COMPILER_CLANG) || !defined(__GNUC__) || __GNUC__ >= 11) conditionals, unless we have new reasons to believe otherwise.

brycelelbach commented 1 year ago

I think whatever we were doing at the time didn't work with GCC 10.

On Thu, Feb 9, 2023 at 5:13 AM Wu Yongwei @.***> wrote:

I do not know what made Bryce @brycelelbach https://github.com/brycelelbach say "GCC 10's CTAD seems sufficiently broken to prevent its use" in commit 972a4bc https://github.com/kokkos/mdspan/commit/972a4bcb5ca5d7af4e056ab2abc24f1e7912e237, but currently CTAD seems to work well in all C++17-supporting GCC versions. The following command works well:

g++ -std=c++17 -D_MDSPAN_USE_CLASS_TEMPLATE_ARGUMENT_DEDUCTION \ -Iinclude examples/godbolt_starter/godbolt_starter.cpp

I tested with the following GCC versions:

  • 7.50
  • 8.4.0
  • 9.4.0
  • 10.3.0
  • 10.4.0
  • 11.3.0
  • 12.2.0

The comment in commit 2141453 https://github.com/kokkos/mdspan/commit/21414531de3ed78eb1eb57ebebfdbf91238e18e4 mentioned a GCC bug 100983 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100983, which was fixed only in GCC 12. However, the current mdspan code does not seem to rely on the absence of this bug.

So I would suggest removing the (defined(_MDSPAN_COMPILER_CLANG) || !defined(GNUC) || GNUC >= 11) conditionals, unless we have new reasons to believe otherwise.

— Reply to this email directly, view it on GitHub https://github.com/kokkos/mdspan/issues/222#issuecomment-1424174193, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBG4XDHZ34OG2ZVS5BORTWWTUPDANCNFSM6AAAAAATBLBKNQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Bryce Adelstein Lelbach aka wash (he/him/his) US Programming Language Standards Chair ISO C++ Library Evolution Chair Principal Architect @ NVIDIA

adah1972 commented 1 year ago

@brycelelbach

Hi Bryce, I believe there was a reason, but I do not know (as the current example does not even work with clang at commit 2141453). The key question is whether the reason still holds.

I believe not.

Do you think I should submit a patch to remove all the GCC conditionals?

brycelelbach commented 1 year ago

Yes

-- Bryce Adelstein Lelbach aka wash (he/him/his) US Programming Language Standards Chair ISO C++ Library Evolution Chair Principal Architect @ NVIDIA

On Fri, Feb 10, 2023, 18:46 Wu Yongwei @.***> wrote:

@brycelelbach https://github.com/brycelelbach

Hi Bryce, I believe there was a reason, but I do not know (as the current example does not even work with clang at commit 2141453 https://github.com/kokkos/mdspan/commit/21414531de3ed78eb1eb57ebebfdbf91238e18e4). The key question is whether the reason still holds.

I believe not.

Do you think I should submit a patch to remove all the GCC conditionals?

— Reply to this email directly, view it on GitHub https://github.com/kokkos/mdspan/issues/222#issuecomment-1426583337, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBG4RJJN5SX5RD2WA6OBLWW34RNANCNFSM6AAAAAATBLBKNQ . You are receiving this because you were mentioned.Message ID: @.***>

adah1972 commented 1 year ago

PR submitted. Please review.

@brycelelbach

brycelelbach commented 1 year ago

I think it'd be best for Christian Trott or one of the current maintainers to review. It seems fine to me but I'm not as familiar with the codebase.

On Sat, Feb 11, 2023 at 5:11 AM Wu Yongwei @.***> wrote:

PR submitted. Please review.

@brycelelbach https://github.com/brycelelbach

— Reply to this email directly, view it on GitHub https://github.com/kokkos/mdspan/issues/222#issuecomment-1426764699, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBG4VEKSTHQQ22DSGKHFTWW6FYZANCNFSM6AAAAAATBLBKNQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Bryce Adelstein Lelbach aka wash (he/him/his) US Programming Language Standards Chair ISO C++ Library Evolution Chair Principal Architect @ NVIDIA

adah1972 commented 1 year ago

@crtrott Hi Chris, would you please take a look? It is a simple change.

mhoemmen commented 1 year ago

@adah1972 FYI, his name is Christian, not Chris. It's Sunday and he just finished a week-long WG21 meeting (in fact, we all just did), so he might not get to this immediately. Thank you for your contribution!

adah1972 commented 1 year ago

Ha, looking up the dictionary, I've found that Chris is not the shortened form of Christian. Sorry for my mistake.

No hurry.