Skip to content

Using Clang __builtin_assume if available.#5

Merged
foonathan merged 2 commits into
foonathan:masterfrom
verri:clang-assume
Jan 10, 2017
Merged

Using Clang __builtin_assume if available.#5
foonathan merged 2 commits into
foonathan:masterfrom
verri:clang-assume

Conversation

@verri

@verri verri commented Jan 9, 2017

Copy link
Copy Markdown
Contributor

Clang must be verified first because it also defines __GNUC__.

__builtin_assume is a recent addition (since 2014), so I also check if it is available.

foonathan
foonathan reviewed Jan 9, 2017
View reviewed changes
Comment thread debug_assert.hpp
#ifdef __GNUC__
#ifdef __clang__
#if __has_builtin(__builtin_assume)
#define DEBUG_ASSERT_ASSUME(Expr) __builtin_assume(Expr)

@foonathan foonathan Jan 9, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you use defined(__clang__) && __has_builtin... there and then just have a single #elif defined(__GNUC__)? Avoids the repetition.

@verri

verri commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

That was my first tentative, but gcc does not compile.

@foonathan

foonathan commented Jan 9, 2017

Copy link
Copy Markdown
Owner

Ah, I see. Could you add a comment then?

@verri

verri commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

Where? Explaining the need for the repetition?

@foonathan

foonathan commented Jan 9, 2017

Copy link
Copy Markdown
Owner

Yes.

@verri

verri commented Jan 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Another possibility is:

// Checking for clang must come first because clang also defines __GNUC__.
#ifndef DEBUG_ASSERT_ASSUME
#ifdef __clang__
// __has_builtin may not work in other compilers.
#if __has_builtin(__builtin_assume)
#define DEBUG_ASSERT_ASSUME(Expr) __builtin_assume(Expr)
#endif
#endif
#endif

#ifndef DEBUG_ASSERT_ASSUME
#ifdef __GNUC__
#define DEBUG_ASSERT_ASSUME(Expr)                                                                  \
    do                                                                                             \
    {                                                                                              \
        if (!(Expr))                                                                               \
            __builtin_unreachable();                                                               \
    } while (0)
#elif defined(_MSC_VER)
#define DEBUG_ASSERT_ASSUME(Expr) __assume(Expr)
#else
/// Hint to the compiler that a condition is `true`.
/// Define it yourself prior to including the header to override it.
#define DEBUG_ASSERT_ASSUME(Expr)
#endif
#endif

Do you think it is better?

@verri

verri commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

I realized something that might be a problem. Compiling example.cpp with clang now emits this warning:

In file included from example.cpp:1:
./debug_assert.hpp:240:33: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
            DEBUG_ASSERT_ASSUME(expr());
                                ^~~~~~
./debug_assert.hpp:51:52: note: expanded from macro 'DEBUG_ASSERT_ASSUME'
#define DEBUG_ASSERT_ASSUME(Expr) __builtin_assume(Expr)
                                                   ^~~~
example.cpp:18:5: note: in instantiation of function template specialization 'debug_assert::detail::do_assert<(lambda at
      example.cpp:18:5), module_a, 2>' requested here
    DEBUG_ASSERT(2 + 2 == 4, module_a{}, debug_assert::level<2>{}); // assertion with level
    ^
./debug_assert.hpp:293:27: note: expanded from macro 'DEBUG_ASSERT'
    debug_assert::detail::do_assert([&] { return Expr; }, DEBUG_ASSERT_CUR_SOURCE_LOCATION, #Expr, \
                          ^
1 warning generated.

I think clang is not able to understand that Expr does not have side effects.

@verri verri force-pushed the clang-assume branch 2 times, most recently from 7af2d1c to 6ea3da0 Compare January 9, 2017 20:41
@foonathan

foonathan commented Jan 9, 2017

Copy link
Copy Markdown
Owner

I don't think it's a good idea then.

@verri

verri commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

Does MSC emit warnings?

@foonathan

foonathan commented Jan 9, 2017

Copy link
Copy Markdown
Owner

I'm afraid I haven't checked thoroughly but I haven't noticed any.

@verri verri force-pushed the clang-assume branch 2 times, most recently from 73a15e8 to 52b09fa Compare January 9, 2017 22:26
@verri

verri commented Jan 9, 2017

Copy link
Copy Markdown
Contributor Author

Marking the lambda as a pure function (as it should be) solves the warnings.

@verri
Fixed warning in clang.
1c80d36 
- Added an attribute to indicate that the assertion expression
is pure (as it should be).
@foonathan foonathan merged commit eff721c into foonathan:master Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@foonathan foonathan foonathan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@verri @foonathan