This is an archive of the discontinued LLVM Phabricator instance.

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

Use the different approach

Harbormaster completed remote builds in B165304: Diff 430640.May 19 2022, 4:30 AM

ldionne added subscribers: rodgert, jwakely.May 19 2022, 7:46 AM

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

Closed by commit rG64748efc2740: [libc++] Use _LIBCPP_ASSERT by default for _PSTL_ASSERTions (authored by philnik). · Explain Why

This revision was automatically updated to reflect the committed changes.

philnik marked an inline comment as done.

philnik added a commit: rG64748efc2740: [libc++] Use _LIBCPP_ASSERT by default for _PSTL_ASSERTions.

Revision Contents

Path

Size

pstl/

include/

pstl/

internal/

pstl_config.h

7 lines

Diff 430975

pstl/include/pstl/internal/pstl_config.h

	Show All 21 Lines
	# error "A parallel backend must be specified"			# error "A parallel backend must be specified"
	#endif			#endif

	// Check the user-defined macro for warnings			// Check the user-defined macro for warnings
	#if defined(PSTL_USAGE_WARNINGS)			#if defined(PSTL_USAGE_WARNINGS)
	# define _PSTL_USAGE_WARNINGS			# define _PSTL_USAGE_WARNINGS
	#endif			#endif

	#if !defined(_PSTL_ASSERT)			#if defined(_LIBCPP_VERSION)
				# include <__assert>
				# define _PSTL_ASSERT(pred) _LIBCPP_ASSERT(pred, "")
				#elif defined(__GLIBCXX__)
				# define _PSTL_ASSERT(pred) __glibcxx_assert(pred)
				ldionneUnsubmitted Done Reply Inline Actions @jwakely @rodgert We'd like to remove `_PSTL_ASSERT` as a customization point and define it explicitly here instead, are you OK with that? ldionne: @jwakely @rodgert We'd like to remove `_PSTL_ASSERT` as a customization point and define it…
				jwakelyUnsubmitted Done Reply Inline Actions 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`. jwakely: Makes sense to me in principle, although I think `__glibcxx_assert` is the wrong definition…
				philnikAuthorUnsubmitted Done Reply Inline Actions 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. philnik: I'm going to take this as a yes. I think adding `_PSTL_DEBUG_ASSERT` or something similar…
				#else
	# include <cassert>			# include <cassert>
	# define _PSTL_ASSERT(pred) (assert((pred)))			# define _PSTL_ASSERT(pred) (assert((pred)))
	#endif			#endif

	// Portability "#pragma" definition			// Portability "#pragma" definition
	#ifdef _MSC_VER			#ifdef _MSC_VER
	# define _PSTL_PRAGMA(x) __pragma(x)			# define _PSTL_PRAGMA(x) __pragma(x)
	#else			#else
	▲ Show 20 Lines • Show All 161 Lines • Show Last 20 Lines

This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use _LIBCPP_ASSERT by default for _PSTL_ASSERTionsClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 430975

pstl/include/pstl/internal/pstl_config.h

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