This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add __has_constexpr_builtin support
ClosedPublic

Authored by Izaron on Oct 16 2022, 4:34 AM.

Details

Summary

The __has_constexpr_builtin macro can be used to check
whether the builtin in constant-evaluated by Clang frontend.

Diff Detail

Event Timeline

Izaron created this revision.Oct 16 2022, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 4:34 AM
Izaron requested review of this revision.Oct 16 2022, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 4:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

A builtin is considered "constexpr" if it has E in its attributes in Builtins.def.

The list of constexpr builtins is consistent, the code in ExprConstant.cpp (where the actual constant evaluation of builtins is being done) guards it.
If builtin is not marked with E, it surely won't be evaluated, and vice versa - if builtin is marked but there is no code for evaluating it, there is a llvm_unreachable.

This macro will be needed because we are making constexpr <cmath> and <cstdlib>. We will conditionally make the functions constexpr until all supported compilers have full support.

We had a discussion where we found out that we need __has_builtin_constexpr: https://discourse.llvm.org/t/how-do-we-plan-to-make-constexpr-cmath-and-cstdlib/65930 Please read it if you wonder why we need it =) Thank you for your attention!

Izaron updated this revision to Diff 468063.Oct 16 2022, 4:46 AM

Slightly changed the test.

Izaron retitled this revision from [Clang] Add __has_builtin_constexpr support to [Clang] Add __has_constexpr_builtin support.Oct 16 2022, 4:47 AM
Izaron edited the summary of this revision. (Show Details)
Izaron updated this revision to Diff 468068.Oct 16 2022, 7:16 AM

Changed llvm_unreachable to just return false. I thought it is more secure.

Can you also add documentation and a release note for the new feature testing macro?

clang/lib/Lex/PPMacroExpansion.cpp
370

It looks like unrelated formatting changes snuck in to this file.

clang/test/Preprocessor/feature_tests.cpp
56

I'd appreciate some parsing tests:

#if __has_constexpr_builtin
#endif

#if __has_constexpr_builtin("__builtin_fmax")
#endif

#if __has_constexpr_builtin(__builtin_fmax, __builtin_fmin)
#endif

need some Sema tests to verify a constexpr builtin is const evaluated.

shafik added a subscriber: shafik.Oct 17 2022, 10:05 AM

This macro will be needed because we are making constexpr <cmath> and <cstdlib>. We will conditionally make the functions constexpr until all supported compilers have full support.

We had a discussion where we found out that we need __has_builtin_constexpr: https://discourse.llvm.org/t/how-do-we-plan-to-make-constexpr-cmath-and-cstdlib/65930 Please read it if you wonder why we need it =) Thank you for your attention!

Please add some of this context to the release notes and/or documentation, thank you.

The changes to the various Vist*CallExpr functions don't seem directly related to supporting __has_constexpr_builtin, can you explain the purpose of those changes? If they are not directly related maybe it makes more sense for them to be split off into a follow-up PR?

Izaron updated this revision to Diff 468314.Oct 17 2022, 2:07 PM
Izaron marked 2 inline comments as done.

Add more parsing tests. Fix unrelated formatting changes. Add release notes and docs.

clang/lib/Lex/PPMacroExpansion.cpp
370

git clang-format HEAD~1 did it =) I reformatted unrelated changes back to original state.

Can you also add documentation and a release note for the new feature testing macro?

Please add some of this context to the release notes and/or documentation, thank you.

Hi! Added some text to the docs and to the release notes mentioning <cmath>. Please look at my wording =)

need some Sema tests to verify a constexpr builtin is const evaluated.

Hi! If I understood you correctly, every (or over 95%?) such function already has its tests checking its constant evaluation. For example test/Sema/constant-builtins-fmax.cpp checks constant __builtin_fmax.

The changes to the various Vist*CallExpr functions don't seem directly related to supporting __has_constexpr_builtin, can you explain the purpose of those changes? If they are not directly related maybe it makes more sense for them to be split off into a follow-up PR?

We have a protocol that the E character in attributes marks that the builtin can be constant evaluated.

But the actual constant evaluation is done in lib/AST/ExprConstant.cpp and has little to do with builtin attributes.

So how do we keep the consistency of this protocol? In other words, how can we guarantee that IF the builtin is constant evaluated, it DOES have the E in Builtins.def?

I solved it with changes to Visit*CallExpr. Whenever we support constant evaluation in a builtin, we won't forget to mark its attributes with E (otherwise the evaluator returns early).

I was thinking to make a consistency test (outside the Clang code), but couldn't do it =(

Izaron added inline comments.Oct 17 2022, 2:12 PM
clang/docs/LanguageExtensions.rst
96 ↗(On Diff #468314)

Just to be clear: this isn't a true statement now (std::fmax calls __builtin_fmax), but soon will be, after @philnik commits this patch https://reviews.llvm.org/D134584 part by part

inline _LIBCPP_HIDE_FROM_ABI float fmax(float __x, float __y) _NOEXCEPT { return __builtin_fmaxf(__x, __y); }
Izaron added inline comments.Oct 17 2022, 2:16 PM
clang/docs/LanguageExtensions.rst
96 ↗(On Diff #468314)

BTW in future it could look something like this:

inline _LIBCPP_CONSTEXPR_CXX23_IF_CONSTEXPR_BUILTIN(__builtin_fmax) _LIBCPP_HIDE_FROM_ABI float fmax(float __x, float __y) _NOEXCEPT { return __builtin_fmaxf(__x, __y); }

It looks like unrelated formatting changes snuck in to this file.

@aaron.ballman JFYI after I reverted what git clang-format HEAD~1 did to the code, the build has failed

ERROR   git-clang-format returned an non-zero exit code 1
Build completed with failures

https://buildkite.com/llvm-project/premerge-checks/builds/117202#0183e7ca-7aa7-4838-aa21-ae5ec717a18a

Can I apply git clang-format HEAD~1 again?

It looks like unrelated formatting changes snuck in to this file.

@aaron.ballman JFYI after I reverted what git clang-format HEAD~1 did to the code, the build has failed

ERROR   git-clang-format returned an non-zero exit code 1
Build completed with failures

https://buildkite.com/llvm-project/premerge-checks/builds/117202#0183e7ca-7aa7-4838-aa21-ae5ec717a18a

Can I apply git clang-format HEAD~1 again?

That's our pre-commit CI reporting failures where there shouldn't be any. It's safe to ignore that failure, so there's no need to run clang-format again.

aaron.ballman accepted this revision.Oct 18 2022, 10:34 AM

LGTM, but please wait for @shafik before landing, in case he has more feedback or questions.

This revision is now accepted and ready to land.Oct 18 2022, 10:34 AM
shafik accepted this revision.Oct 20 2022, 4:30 PM

LGTM, thanks for explaining what I was missing.

This revision was landed with ongoing or failed builds.Oct 21 2022, 4:23 AM
This revision was automatically updated to reflect the committed changes.