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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Feb 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0badb93
When using C++23, use [[assume]] attribute.
bangerth Jan 11, 2024
46c0e6c
Remove dead code for gcc-13 and newer
masterleinad Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 10 additions & 15 deletions include/deal.II/base/config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,29 +183,24 @@
* attribute for older standards we rely on compiler intrinsics when
* available.
*/
#if defined(__clang__)
# define DEAL_II_CXX23_ASSUME(expr) __builtin_assume(static_cast<bool>(expr))
#elif defined(__GNUC__) && !defined(__ICC) && __GNUC__ >= 13
# define DEAL_II_CXX23_ASSUME(expr) \
do \
{ \
_Pragma("GCC diagnostic push") \
_Pragma("GCC diagnostic ignored \"-Wimplicit-fallthrough\"") \
[[assume(expr)]]; \
_Pragma("GCC diagnostic pop") \
} \
while (false)
#elif defined(_MSC_VER) || defined(__ICC)
# define DEAL_II_CXX23_ASSUME(expr) __assume(expr);
#ifdef DEAL_II_HAVE_CXX23
# define define DEAL_II_ASSUME(expr) [[assume(expr)]]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@tamiko tamiko Jan 25, 2024 •
edited

Choose a reason for hiding this comment

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

@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.

#else
# if defined(__clang__)
# define DEAL_II_CXX23_ASSUME(expr) __builtin_assume(static_cast<bool>(expr))
# elif defined(_MSC_VER) || defined(__ICC)
# define DEAL_II_CXX23_ASSUME(expr) __assume(expr);
# else
/* no way with GCC to express this without evaluating 'expr' */
# define DEAL_II_CXX23_ASSUME(expr) \
# define DEAL_II_CXX23_ASSUME(expr) \
do \
{ \
} \
while (false)
# endif
#endif


/**
* Macro indicating that the current feature will be removed in a future
* release.
Expand Down
1 change: 1 addition & 0 deletions include/deal.II/base/exceptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,7 @@ namespace deal_II_exceptions
} /*namespace deal_II_exceptions*/



/**
* A macro that serves as the main routine in the exception mechanism for debug
* mode error checking. It asserts that a certain condition is fulfilled,
Expand Down