This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use _LIBCPP_ASSERT by default for _PSTL_ASSERTions
ClosedPublic

Authored by philnik on May 15 2022, 9:15 AM.

Diff Detail

Event Timeline

philnik created this revision.May 15 2022, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 9:15 AM
philnik requested review of this revision.May 15 2022, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 9:15 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 18 2022, 12:44 PM

LGTM with the different approach.

libcxx/include/__assert
51–53 ↗(On Diff #429543)

I would rather not start pretending that _PSTL_ASSERT is a customization point. Instead, I think we should do something like this inside pstl_config.h:

#if defined(_LIBCPP_VERSION)
#    define _PSTL_ASSERT(pred) _LIBCPP_ASSERT(pred, "")
#elif defined(__GLIBCXX__)
#   // does libstdc++ want something special? if so, they can add it here
#else
#    include <cassert>
#    define _PSTL_ASSERT(pred) (assert((pred)))
#endif
This revision is now accepted and ready to land.May 18 2022, 12:44 PM
philnik updated this revision to Diff 430640.May 19 2022, 4:11 AM
philnik marked an inline comment as done.
  • Use the different approach
ldionne added inline comments.
pstl/include/pstl/internal/pstl_config.h
33–34

@jwakely @rodgert We'd like to remove _PSTL_ASSERT as a customization point and define it explicitly here instead, are you OK with that?

jwakely added inline comments.May 19 2022, 12:03 PM
pstl/include/pstl/internal/pstl_config.h
33–34

Makes sense to me in principle, although I think __glibcxx_assert is the wrong definition (even though that's what we use today).

I see several assertions in parallel_backend_tbb.h like _PSTL_ASSERT(std::is_sorted(_M_x_beg + _M_xs, _M_x_beg + _M_xe, _M_comp)); which changes the complexity guarantees of the algo. That's not appropriate for __glibcxx_assert, but would be appropriate for the more heavyweight _GLIBCXX_DEBUG_ASSERT.

Maybe that should be fixed as a separate issue, as I assume that isn't appropriate for libc++ either.

We could introduce _PSTL_DEBUG_ASSERT for the more expensive assertions, and map that to _LIBCPP_DEBUG_ASSERT or _GLIBCXX_DEBUG_ASSERT.

philnik marked 2 inline comments as done.May 20 2022, 7:57 AM
philnik added inline comments.
pstl/include/pstl/internal/pstl_config.h
33–34

I'm going to take this as a yes. I think adding _PSTL_DEBUG_ASSERT or something similar should be deferred to after having added a CI configuration with the PSTL enabled, so we actually check it instead of hoping that it works.

This revision was landed with ongoing or failed builds.May 20 2022, 7:59 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.