This is an archive of the discontinued LLVM Phabricator instance.

[Builtins] Implement __builtin_is_constant_evaluated for use in C++2a
ClosedPublic

Authored by EricWF on Dec 9 2018, 5:49 PM.

Details

Summary

This patch implements __builtin_is_constant_evaluated as specifier by P0595R2. It is built on the back of Bill Wendling's work for __builtin_constant_p().

More tests to come, but early feedback is appreciated.

I plan to implement warnings for common mis-usages like those belowe in a following patch:

void foo(int x) {
  if constexpr (std::is_constant_evaluated())) { // condition is always `true`. Should use plain `if` instead.
   foo_constexpr(x);
  } else {
    foo_runtime(x);
  }
}

Diff Detail

Event Timeline

EricWF created this revision.Dec 9 2018, 5:49 PM

Hi Eric,

Thanks for working on this!

include/clang/Basic/Builtins.def
762

Name bikeshedding : perhaps the builtin name could be detached from the std:: name? Suggestion: __builtin_in_constant_evaluation_context

lib/AST/ExprConstant.cpp
8207

Do we need compat and extension warnings for the use of this builtin before c++2a? I assume people will play with the builtin before the library facility is there. OTOH, since this will be mainly exposed as a library thing, whatever check for c++ version is done at the library level should be enough?

EricWF marked 2 inline comments as done.Dec 10 2018, 8:47 PM

Sorry for the delayed response. My email filters were incorrect.

include/clang/Basic/Builtins.def
762

I'm not sure detaching it from the std:: name is desirable. Most importantly it should match w/e GCC does/decides to do.

But if it is, we should name in deference to the standardese it implements. Specifically weither an expression or conversion is manifestly constant-evaluated [expr.const]p11.

Therefore I proffer __builtin_is_manifestly_constant_evaluated() or __builtin_is_being_manifestly_constant_evaluated().

lib/AST/ExprConstant.cpp
8207

No. We don't warn for any other builtin used to serve the standard library (__is_constructible(), __builtin_addressof(), etc)

The builtin is useful in all dialects, and I suspect users will frequently want to call it directly. If __has_builtin(__builtin_is_constant_evaluated) returns true, it should be safe to use.

EricWF marked an inline comment as done.Jan 5 2019, 5:43 PM

Ping! I would like to land this before the next release.

@rsmith, what sorts of additional tests are needed?

include/clang/Basic/Builtins.def
762

Actually, GCC has __builtin_is_constant_evaluated so we should use that name too.

bruno added inline comments.Jan 7 2019, 4:46 PM
include/clang/Basic/Builtins.def
762

Agreed!

@rsmith, what sorts of additional tests are needed?

Let's see...

  • test that we use the constant initializer for suitably-constant-initialized locals even if we can't constant-fold the reference to them:
bool f(int k) {
  const bool n = is_constant_evaluated();
  const bool *volatile p = n;
  return *p;
}
  • test that we suitably handle local references initialized by constant expressions (and local constexpr variables), and non-const globals
  • test that __builtin_is_constant_evaluated() returns false in contexts that IR generation might constant-fold (eg, the operand of a non-constexpr if or the left-hand side of a ?: expression)
  • ensure that the right value is returned in all the cases that are syntactically constant-expressions in the C++ grammar:
    • second or subsequent array bound in a new-expression
    • case labels
    • operand of static_assert
    • array bounds (but be careful around the treatment of VLAs! for them we want the "try evaluating it as a constant expression and fall back to treating it as a non-constant expression" rule that we have for the initializers of globals)
    • values of enumerators
    • operand of alignas
    • width of a bit-field
    • template arguments (but bizarrely not default template arguments! I think that's a bug and I'll bring it up on the core reflector.)
    • operand of noexcept (and eventually explicit -- please write a test for the latter with a FIXME to update the expectations, so we don't forget)
include/clang/Basic/Builtins.def
761

This should be grouped with __builtin_launder, though maybe moving that one here makes more sense than grouping them both under "GCC builtins" since they're directly implementing C++ stdlib functions.

762

I don't think the "u" flag has any meaning here (there are no arguments, so no unevaluated arguments), and it would reduce the complexity of this declaration if you remove it.

I don't think the "c" flag is formally correct -- a program can contain two calls to __builtin_is_constant_evaluated() that return different values (even "U" appears to be incorrect). It would, for example, be reasonable for Clang to warn on:

const bool b = pure_fn();
return b == pure_fn();

... on the basis that the comparison must always evaluate to true. But such a warning would be incorrect for __builtin_is_constant_evaluated, so it's not a pure function.

EricWF updated this revision to Diff 181974.Jan 15 2019, 10:04 PM
EricWF marked 5 inline comments as done.

Add the requested tests.

Thanks for the added tests! They seem to have found a bug in the handling of local const integral variables.

test/CodeGenCXX/builtin-is-constant-evaluated.cpp
42–43

Do we get the same result without the attribute?

74

Why call both the std:: function and the builtin here? If we want to test that both work, shouldn't we use && instead of ||?

92

This is incorrect: n should be initialized to true because it occurs in the initializer of a variable that is usable in constant expressions.

123

Can you also test the case where an automatic storage duration reference is initialized to one of two static storage duration variables? In that case, we should pick the 'constant' case because the initializer would be a constant expression.

EricWF marked 6 inline comments as done.Jan 23 2019, 9:07 PM
EricWF added inline comments.
test/CodeGenCXX/builtin-is-constant-evaluated.cpp
42–43

Yes it does. But Ill ad a second test to verify.

74

I think I was trying to ensure std::is_constant_evaluated worked the same as calling the builtin directly.
Using || here was a mistake. Fixed.

92

Ack.

EricWF updated this revision to Diff 183248.Jan 23 2019, 9:56 PM
EricWF marked 2 inline comments as done.
  • Add or correct tests as requested.
  • Fix code gen for initializers of reference types to correctly evaluate the initializer as a constant expression.
rsmith accepted this revision.Apr 22 2019, 5:30 PM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 22 2019, 5:30 PM
EricWF updated this revision to Diff 196368.Apr 23 2019, 6:53 PM
  • Merge with master.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2019, 7:22 PM