[PATCH] D55500: [Builtins] Implement __builtin_is_constant_evaluated for use in C++2a

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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



================
Comment at: test/CodeGenCXX/builtin-is-constant-evaluated.cpp:41-42
+
+// CHECK-STATIC-DAG: @p = global i32 26,
+CONSTINIT int p = f(); // f().m == 13; initialized to 26
+
----------------
Do we get the same result without the attribute?


================
Comment at: test/CodeGenCXX/builtin-is-constant-evaluated.cpp:73
+  // CHECK-ARR: %x1 = alloca [101 x i8],
+  char x1[std::is_constant_evaluated() || __builtin_is_constant_evaluated() ? 101 : 1];
+
----------------
Why call both the std:: function and the builtin here? If we want to test that both work, shouldn't we use `&&` instead of `||`?


================
Comment at: test/CodeGenCXX/builtin-is-constant-evaluated.cpp:91
+bool test_constant_initialized_local(int k) {
+  // CHECK-FOLD: store i8 0, i8* %n,
+  // CHECK-FOLD: store volatile i8* %n, i8** %p,
----------------
This is incorrect: `n` should be initialized to `true` because it occurs in the initializer of a variable that is usable in constant expressions.


================
Comment at: test/CodeGenCXX/builtin-is-constant-evaluated.cpp:122
+  // CHECK-FOLD: store i32* %i_non_constant, i32** %r,
+  const int &r = __builtin_is_constant_evaluated() ? i_constant : i_non_constant;
+}
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55500/new/

https://reviews.llvm.org/D55500