[C++23] When using C++23, use [[assume]] attribute. #16457

bangerth · 2024-01-11T21:33:18Z

This is a follow-up to #16437 and #16393 : When using C++23, we can avoid all of the compiler-specific stuff.

We don't currently configure for C++23 at all, but this at least makes sure we don't forget about it when the time comes to add the cmake bits to at least recognize C++23.

masterleinad · 2024-01-11T21:54:38Z

include/deal.II/base/exceptions.h

+#  else
 #    define DEAL_II_ASSUME(expr) \
      do                         \
        {                        \
        }                        \
      while (false)
 #  endif


We still need to disable the warnings for gcc-13 so I would rather modify this last case

Suggested change

# else

# define DEAL_II_ASSUME(expr) \

do \

{ \

} \

while (false)

# endif

# elif defined(DEAL_II_HAVE_CXX23)

# define DEAL_II_ASSUME(expr) [[assume(expr)]]

# else

# define DEAL_II_ASSUME(expr) \

do \

{ \

} \

while (false)

I didn't actually change anything about the existing cases, github just has a hard time showing it as such. Take a look here: https://github.com/dealii/dealii/pull/16457/files?w=1

Yes, I know. My point was that we might already say that g++-13 supports C++23 to some extent and we would then not suppress the warnings with switch constructs.

I must admit that I'm confused. I put the ifdef DEAL_II_HAVE_CXX23 case at the very top, because then we have a standardized way of saying what we want to say. All the rest is in the #else branch. Your suggested addition of

# elif defined(DEAL_II_HAVE_CXX23) # define DEAL_II_ASSUME(expr) [[assume(expr)]] # else

further down is a dead branch that cannot be reached.

My only concern is that we run into the new branch instead of the GCC one if we decide that gcc-13 provides sufficient C++23 support. In that case, we would get a lot of warnings that I explicitly suppressed. We should also test for this feature as we usually do when setting DEAL_II_HAVE_CXX23 anyway.

I think what you're saying is that you want us to use the GCC-specific way even if [[assume]] is available? Why would we do this? If there's a standard way to do something, we should use it.

bangerth · 2024-01-13T04:19:36Z

Let us postpone this PR until one of us has a compiler to test this. I marked the title of the PR so that we can easily search for it.

bangerth · 2024-01-24T02:30:04Z

Updated, as suggested in #16513.

masterleinad · 2024-01-24T04:34:33Z

include/deal.II/base/config.h.in

-#elif defined(__GNUC__) && !defined(__ICC) && __GNUC__ >= 13
-#  define DEAL_II_CXX23_ASSUME(expr)                                   \
+#ifdef DEAL_II_HAVE_CXX23
+#  define define DEAL_II_ASSUME(expr) [[assume(expr)]]


This still means that we can't define DEAL_II_HAVE_CXX23 for gcc-13 without getting in a lot of places (switch constructs) where this macro might be used.

This comes back to the question I asked above that fell through the cracks:

I think what you're saying is that you want us to use the GCC-specific way even if [[assume]] is available? Why would we do this? If there's a standard way to do something, we should use it.

We are already using [[assume(expr)]] for gcc but that triggers a bunch of warnings in switch statements.

I may have missed this, but is there an example posted somewhere I could look at?

I'm seeing

In file included from /tmp/Software/dealii/include/deal.II/base/aligned_vector.h:22, from /tmp/Software/dealii/include/deal.II/base/table.h:21, from /tmp/Software/dealii/source/grid/manifold.cc:16: /tmp/Software/dealii/include/deal.II/grid/reference_cell.h: In member function 'double dealii::ReferenceCell::d_linear_shape_function(const dealii::Point<dim>&, unsigned int) const [with int dim = 2]': /tmp/Software/dealii/include/deal.II/base/exceptions.h:1529:15: error: this statement may fall through [-Werror=implicit-fallthrough=] 1529 | [[assume(expr)]]; \ | ^~~~~~~~~~~~~~~~ /tmp/Software/dealii/include/deal.II/base/exceptions.h:1669:29: note: in expansion of macro 'DEAL_II_ASSUME' 1669 | # define Assert(cond, exc) DEAL_II_ASSUME(cond) | ^~~~~~~~~~~~~~ /tmp/Software/dealii/include/deal.II/grid/reference_cell.h:2207:17: note: in expansion of macro 'Assert' 2207 | Assert(false, ExcInternalError()); | ^~~~~~ In file included from /tmp/Software/dealii/include/deal.II/fe/fe_data.h:23, from /tmp/Software/dealii/include/deal.II/fe/fe.h:23, from /tmp/Software/dealii/include/deal.II/fe/fe_poly.h:25, from /tmp/Software/dealii/include/deal.II/fe/fe_q_base.h:23, from /tmp/Software/dealii/include/deal.II/fe/fe_q.h:23, from /tmp/Software/dealii/source/grid/manifold.cc:20: /tmp/Software/dealii/include/deal.II/grid/reference_cell.h:2211:7: note: here 2211 | case ReferenceCells::Tetrahedron: | ^~~~

for example.

Ah, I see. The code that surrounds this is follows:

case ReferenceCells::Triangle: { switch (i) { case 0: return 1.0 - xi[std::min(0, dim - 1)] - xi[std::min(1, dim - 1)]; case 1: return xi[std::min(0, dim - 1)]; case 2: return xi[std::min(1, dim - 1)]; default: Assert(false, ExcInternalError()); // *************** The line in question } } // see also BarycentricPolynomials<3>::compute_value case ReferenceCells::Tetrahedron: { [...]

I see how a compiler could get confused by this. In other places, of course, we add a dummy return 0; after that. I could do this either by hand or by script if you happened to have a log file that has all of the places where this happens.

I think it is preferable to clean up our code base if the upside is that we can use a standard C++ feature.

I think our current consensus is to not blanket convert all Asserts to assume statements so this wouldn't be an issue.

I think it makes much more sense to create a new AssertOrAssume macro (or a similar construct) to selectively convert some of the asserts (where we know or anticipate that they have a benefit).

This would allow us to simply use the C++23 [[assume()]] with gcc as suggested in this pull request without working around compiler warnings. And also to drop -Wno-assume (which already happened).

I want to point out that in general a compiler warning tells us that something is going wrong, which, admittedly, can be a defect or deficiency in a compiler. But it is also an indicator that our expectations ("do something useful with this assume statement!") does not match with what a compiler can do (at least at the moment).

So sure, we can make the compiler shut up in the above case... but much more fundamentally what would we gain with an assume statement at this place? [[assume(false)]] is a useless statement. But the compiler will need to parse it, run a lexer on it and then decide to do nothing with it.

So my suggestion would be to have a DEAL_II_CXX23_ASSUME macro with as little workarounds as possible. And we explore how we can use it.

FWIW, [[assume(false)]] effectively behaves as std::unreachable().

@masterleinad You're correct.

Arguably, that might make the whole situation for our Assume(false, ...) statements even worse... at least in terms of "debuggability" for the release mode.

bangerth · 2024-02-01T00:26:19Z

@masterleinad Since I think we've all concluded that we shouldn't use this to replace Assert(cond,...) in release mode, but sparingly, do you still object to merging?

masterleinad · 2024-02-01T12:59:06Z

@masterleinad Since I think we've all concluded that we shouldn't use this to replace Assert(cond,...) in release mode, but sparingly, do you still object to merging?

I'm fine with it if we treat DEAL_II_HAVE_CXX23 and g++-13 the same, i.e., either make sure that g++-13 defines DEAL_II_HAVE_CXX23 or do something like

#if defined(DEAL_II_HAVE_CXX23) || (defined(__GNUC__) && !defined(__ICC) && __GNUC__ >= 13)
#  define DEAL_II_ASSUME(expr) [[assume(expr)]]
#endif
#ifndef DEAL_II_ASSUME
...
#endif

In particular, we should drop the warning suppression for g++-13 if we want to use it sparingly and rather fix warnings at the call site if they arise.

masterleinad · 2024-02-01T13:01:27Z

It would also be good if we used the macro at least in one place already so we are testing compiler support.

tamiko · 2024-02-01T17:23:43Z

@masterleinad This is now in place (I merged your PR).

masterleinad · 2024-02-01T17:35:26Z

@masterleinad This is now in place (I merged your PR).

Let's just drop the special case for g++-13 then.

masterleinad · 2024-02-08T19:18:33Z

Let's just drop the special case for g++-13 then.

I pushed a corresponding commit.

bangerth · 2024-02-09T01:44:50Z

Anyone other than @masterleinad or me willing to take a gamble on this?

masterleinad · 2024-02-13T18:21:27Z

@bangerth Are you fine with the current status?

bangerth · 2024-02-14T01:09:31Z

Yes yes, absolutely. But since both you and I contributed parts of this, I thought someone else should push the button.

bangerth · 2024-02-16T20:57:10Z

Ping?

bangerth added the ready to test label Jan 11, 2024

masterleinad reviewed Jan 11, 2024

View reviewed changes

bangerth mentioned this pull request Jan 11, 2024

Export C++23 status to config.h. #16459

Merged

bangerth changed the title ~~When using C++23, use [[assume]] attribute.~~ [C++23] When using C++23, use [[assume]] attribute. Jan 13, 2024

bangerth added the Do not merge ☠️ label Jan 13, 2024

bangerth added the C++23 label Jan 13, 2024

bangerth mentioned this pull request Jan 22, 2024

Reenable assume macro #16513

Merged

bangerth force-pushed the assume branch 2 times, most recently from 7d715d2 to 7991ea9 Compare January 24, 2024 01:51

bangerth removed the Do not merge ☠️ label Jan 24, 2024

masterleinad reviewed Jan 24, 2024

View reviewed changes

bangerth mentioned this pull request Jan 24, 2024

Create DEAL_II_UNREACHABLE #16536

Closed

masterleinad mentioned this pull request Feb 1, 2024

Require support for attribute [[assume]] for C++23 support #16582

Merged

When using C++23, use [[assume]] attribute.

0badb93

bangerth force-pushed the assume branch from 7991ea9 to 0badb93 Compare February 6, 2024 15:28

Remove dead code for gcc-13 and newer

46c0e6c

kronbichler approved these changes Feb 14, 2024

View reviewed changes

masterleinad merged commit 2e2dd3f into dealii:master Feb 19, 2024
15 checks passed

bangerth deleted the assume branch February 19, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++23] When using C++23, use [[assume]] attribute. #16457

[C++23] When using C++23, use [[assume]] attribute. #16457

bangerth commented Jan 11, 2024

masterleinad Jan 11, 2024

bangerth Jan 11, 2024

masterleinad Jan 11, 2024

bangerth Jan 11, 2024

masterleinad Jan 12, 2024

bangerth Jan 13, 2024

bangerth commented Jan 13, 2024

bangerth commented Jan 24, 2024

masterleinad Jan 24, 2024

bangerth Jan 24, 2024

masterleinad Jan 24, 2024

bangerth Jan 24, 2024

masterleinad Jan 24, 2024

bangerth Jan 24, 2024

tamiko Jan 24, 2024

masterleinad Jan 24, 2024

tamiko Jan 25, 2024 •

edited

bangerth commented Feb 1, 2024

masterleinad commented Feb 1, 2024

masterleinad commented Feb 1, 2024

tamiko commented Feb 1, 2024

masterleinad commented Feb 1, 2024

masterleinad commented Feb 8, 2024

bangerth commented Feb 9, 2024

masterleinad commented Feb 13, 2024

bangerth commented Feb 14, 2024

bangerth commented Feb 16, 2024

[C++23] When using C++23, use [[assume]] attribute. #16457

[C++23] When using C++23, use [[assume]] attribute. #16457

Conversation

bangerth commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangerth commented Jan 13, 2024

bangerth commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamiko Jan 25, 2024 • edited

Choose a reason for hiding this comment

bangerth commented Feb 1, 2024

masterleinad commented Feb 1, 2024

masterleinad commented Feb 1, 2024

tamiko commented Feb 1, 2024

masterleinad commented Feb 1, 2024

masterleinad commented Feb 8, 2024

bangerth commented Feb 9, 2024

masterleinad commented Feb 13, 2024

bangerth commented Feb 14, 2024

bangerth commented Feb 16, 2024

tamiko Jan 25, 2024 •

edited