[libcxx] Add flag to disable __builtin_assume in _LIBCPP_ASSERT

[aeubanks]

Summary

Introduce _LIBCPP_ASSERTIONS_DISABLE_ASSUME which makes _LIBCPP_ASSERT
not call __builtin_assume when _LIBCPP_ENABLE_ASSERTIONS == 0.

Calling __builtin_assume was introduced in D122397.

__builtin_assume is generally supposed to improve optimizations, but can
cause regressions when LLVM has trouble handling the calls to
llvm.assume() (see comments in D122397).

[philnik]

libcxx/include/__assert

36–45

I think the disable things flags are normally 'define it to disable this part', so the flag should be _LIBCPP_ASSERTIONS_DISABLE_ASSUME. That would allow you to remove this whole block and just have !defined(_LIBCPP_ASSERTIONS_DISABLE_ASSUME) in line 51.

[ldionne]

Thanks for the patch @aeubanks. I agree with @philnik's suggestion, the macro should be "defined => no assume, undefined => assume". That way, it would be defined by default but we'd still have the escape hatch until we figure out issues with the builtin. Do you have an ETA for that (it's more than just this libc++ issue -- if Clang has a broken builtin, we need to fix it ASAP).

[aeubanks]

use disable define

[aeubanks]

In D123175#3432673, @ldionne wrote:

Thanks for the patch @aeubanks. I agree with @philnik's suggestion, the macro should be "defined => no assume, undefined => assume". That way, it would be defined by default but we'd still have the escape hatch until we figure out issues with the builtin. Do you have an ETA for that (it's more than just this libc++ issue -- if Clang has a broken builtin, we need to fix it ASAP).

I don't have an ETA, so far I have one patch that only partially solves the very reduced case I was looking at, it still needs followup work and also followup investigation to see if there are any remaining issues.

[philnik]

LGTM

[ldionne]

@aeubanks Hi, I just came across this again, did you ever get around to fixing the broken builtin? It would be nice to remove _LIBCPP_ASSERTIONS_DISABLE_ASSUME.

[aeubanks]

In D123175#4381933, @ldionne wrote:

@aeubanks Hi, I just came across this again, did you ever get around to fixing the broken builtin? It would be nice to remove _LIBCPP_ASSERTIONS_DISABLE_ASSUME.

sorry, I never fixed the regressions, I'll try to find some time to take a look at this again

[aeubanks]

I was talking to some people about this and due to the way assumes are handled in the IR, it's not an automatic win to sprinkle assumes everywhere. There is inherently a tradeoff when you add assumes (assumes are instructions and are control flow dependent, adding assumes can prevent some optimizations) so they should be added carefully. Was there a concrete speedup seen when you initially landed this? But either way I think there needs to be a way to opt out of this behavior.

[ldionne]

In D123175#4427053, @aeubanks wrote:

I was talking to some people about this and due to the way assumes are handled in the IR, it's not an automatic win to sprinkle assumes everywhere. There is inherently a tradeoff when you add assumes (assumes are instructions and are control flow dependent, adding assumes can prevent some optimizations) so they should be added carefully. Was there a concrete speedup seen when you initially landed this? But either way I think there needs to be a way to opt out of this behavior.

No, see https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609/11?u=ldionne.

I'll disable them by default -- this is nuts!

[ldionne]

See D153968 for the patch that stops using __builtin_assume unconditionally.

libcxx/include/__assert

 #ifndef _LIBCPP_ENABLE_ASSERTIONS
 # define _LIBCPP_ENABLE_ASSERTIONS _LIBCPP_ENABLE_ASSERTIONS_DEFAULT
 #endif
 
 #if _LIBCPP_ENABLE_ASSERTIONS != 0 && _LIBCPP_ENABLE_ASSERTIONS != 1
 # error "_LIBCPP_ENABLE_ASSERTIONS must be set to 0 or 1"
 #endif
 
 #if _LIBCPP_ENABLE_ASSERTIONS
 # define _LIBCPP_ASSERT(expression, message)                                        \
     (__builtin_expect(static_cast<bool>(expression), 1) ?                           \
       (void)0 :                                                                     \
       ::std::__libcpp_assertion_handler(__FILE__, __LINE__, #expression, message))
-#else
-# if __has_builtin(__builtin_assume)
-#   define _LIBCPP_ASSERT(expression, message)                                      \
-      (_LIBCPP_DIAGNOSTIC_PUSH                                                      \
-      _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume")                                  \
-      __builtin_assume(static_cast<bool>(expression))                               \
-      _LIBCPP_DIAGNOSTIC_POP)
-# else
-#   define _LIBCPP_ASSERT(expression, message) ((void)0)
-# endif
-#endif
+#elif !defined(_LIBCPP_ASSERTIONS_DISABLE_ASSUME) && __has_builtin(__builtin_assume)
+# define _LIBCPP_ASSERT(expression, message)                                        \
+    (_LIBCPP_DIAGNOSTIC_PUSH                                                        \
+    _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume")                                    \
+    __builtin_assume(static_cast<bool>(expression))                                 \
+#else
+# define _LIBCPP_ASSERT(expression, message) ((void)0)
+#endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 _LIBCPP_OVERRIDABLE_FUNC_VIS _LIBCPP_AVAILABILITY_ASSERTION_HANDLER
 void __libcpp_assertion_handler(char const* __file, int __line, char const* __expression, char const* __message);
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ASSERT

libcxx/test/libcxx/assertions/single_expression.sh.cpp

 // it in places that require an expression, such as in a constructor initializer list.
 
 // RUN: %{build} -Wno-macro-redefined -D_LIBCPP_ENABLE_ASSERTIONS=1
 // RUN: %{run}
 
 // RUN: %{build} -Wno-macro-redefined -D_LIBCPP_ENABLE_ASSERTIONS=0
 // RUN: %{run}
 
+// RUN: %{build} -Wno-macro-redefined -D_LIBCPP_ENABLE_ASSERTIONS=0 -D_LIBCPP_ASSERTIONS_DISABLE_ASSUME
+// RUN: %{run}
+
 // We flag uses of the assertion handler in older dylibs at compile-time to avoid runtime
 // failures when back-deploying.
 // XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
 
 #include <__assert>
 #include <cassert>
 
 void f() {