[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void created this revision.
void added a reviewer: rsmith.
Herald added subscribers: cfe-commits, kristina.
Herald added a reviewer: shafik.

A __builtin_constant_p may end up with a constant after inlining. Use
the is.constant intrinsic if it's a variable that's in a context where
it may resolve to a constant, e.g., an argument to a function after
inlining.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 173445.
void added a comment.
Herald added a subscriber: jfb.

Adding ConstantExpr visitor.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

This has https://reviews.llvm.org/D54356 integrated into it. https://reviews.llvm.org/D54356 should be reviewed and submitted first, even though it's out of order.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Ping?


Repository:
rC Clang

https://reviews.llvm.org/D54355

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

rsmith added inline comments.


================
Comment at: include/clang/AST/Expr.h:908-912
+ static ConstantExpr *Create(const ASTContext &Context, Expr *E) {
+ if (ConstantExpr *CE = dyn_cast<ConstantExpr>(E))
+ return CE;
+ return new (Context) ConstantExpr(E);
+ }
----------------
If we're creating two `ConstantExpr` wrappers for the same expression, that seems like a bug in the caller to me. When does this happen?


================
Comment at: lib/AST/Expr.cpp:2915-2916
+ case ConstantExprClass: {
+ const Expr *Exp = cast<ConstantExpr>(this)->getSubExpr();
+ return Exp->isConstantInitializer(Ctx, false, Culprit);
+ }
----------------
Can we just return `true` here? If not, please add a FIXME saying that we should be able to.


================
Comment at: lib/Sema/SemaExpr.cpp:4973-4974

+ if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+ Arg = ConstantExpr::Create(Context, Arg);
+
----------------
We should create this node as part of checking that the argument is an ICE (in `SemaBuiltinConstantArg`).


================
Comment at: lib/Sema/SemaExpr.cpp:14156-14157

+ if (!CurContext->isFunctionOrMethod())
+ E = ConstantExpr::Create(Context, E);
+
----------------
I don't understand why `CurContext` matters here. Can you explain the intent of this check?


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 173922.
void marked an inline comment as done.
void added a comment.

Remove some extraneous checks.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.


================
Comment at: include/clang/AST/Expr.h:908-912
+ static ConstantExpr *Create(const ASTContext &Context, Expr *E) {
+ if (ConstantExpr *CE = dyn_cast<ConstantExpr>(E))
+ return CE;
+ return new (Context) ConstantExpr(E);
+ }
----------------

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
If we're creating two `ConstantExpr` wrappers for the same expression, that seems like a bug in the caller to me. When does this happen?

I saw a place that was trying to do that, but it may no longer be valid. I can remove the check for now.


================
Comment at: lib/AST/Expr.cpp:2915-2916
+ case ConstantExprClass: {
+ const Expr *Exp = cast<ConstantExpr>(this)->getSubExpr();
+ return Exp->isConstantInitializer(Ctx, false, Culprit);
+ }
----------------

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
Can we just return `true` here? If not, please add a FIXME saying that we should be able to.

To be honest, I don't know. Theoretically we should be able to. Let me give it a try and see how it goes.


================
Comment at: lib/Sema/SemaExpr.cpp:14156-14157

+ if (!CurContext->isFunctionOrMethod())
+ E = ConstantExpr::Create(Context, E);
+
----------------

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
I don't understand why `CurContext` matters here. Can you explain the intent of this check?

It may have been an artifact of development. I can remove it.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void marked an inline comment as not done.
void added inline comments.


================
Comment at: lib/AST/Expr.cpp:2915-2916
+ case ConstantExprClass: {
+ const Expr *Exp = cast<ConstantExpr>(this)->getSubExpr();
+ return Exp->isConstantInitializer(Ctx, false, Culprit);
+ }
----------------

Post by Bill Wendling via Phabricator via cfe-commits

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
Can we just return `true` here? If not, please add a FIXME saying that we should be able to.

To be honest, I don't know. Theoretically we should be able to. Let me give it a try and see how it goes.

I tested this and it looks like it could lead to an extra warning like this:

```
File /sandbox/morbo/llvm/llvm-mirror/tools/clang/test/Sema/array-init.c Line 285: cannot initialize array of type 'int [5]' with non-constant array of type 'int [5]'
```

(Similar in `Sema/compound-literal.c`.) I'll add a comment.


================
Comment at: lib/Sema/SemaExpr.cpp:4973-4974

+ if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+ Arg = ConstantExpr::Create(Context, Arg);
+
----------------

Post by Bill Wendling via Phabricator via cfe-commits
We should create this node as part of checking that the argument is an ICE (in `SemaBuiltinConstantArg`).

It turns out that `SemaBuiltinConstantArg` isn't called for `__builtin_constant_p()`. Its argument type is `.`, which...I'm not sure what that means. It looks like a vararg, but that doesn't make sense for the builtin.

Should I change its signature in `Builtins.def`?


Repository:
rC Clang

https://reviews.llvm.org/D54355

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

rsmith added inline comments.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1930-1931
+ if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+ // At -O0, we don't perform inlining, so we don't need to delay the
+ // processing.
+ return RValue::get(ConstantInt::get(Int32Ty, 0));
----------------
Are there cases where a call to an `always_inline` function could change the outcome?


================
Comment at: lib/Sema/SemaExpr.cpp:4973-4974

+ if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+ Arg = ConstantExpr::Create(Context, Arg);
+
----------------

Post by Bill Wendling via Phabricator via cfe-commits

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
We should create this node as part of checking that the argument is an ICE (in `SemaBuiltinConstantArg`).

It turns out that `SemaBuiltinConstantArg` isn't called for `__builtin_constant_p()`. Its argument type is `.`, which...I'm not sure what that means. It looks like a vararg, but that doesn't make sense for the builtin.
Should I change its signature in `Builtins.def`?

It's also marked `t`, which means "signature is meaningless, uses custom type checking".

The argument to `__builtin_constant_p` isn't required to be an ICE, though, so we shouldn't be calling `SemaBuiltinConstantArg` for it / creating a `ConstantExpr` wrapping it, as far as I can tell.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 173970.
void marked an inline comment as not done.
void added a comment.

Updated to address comments.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

I think this is ready now. PTAL.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:1930-1931
+ if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+ // At -O0, we don't perform inlining, so we don't need to delay the
+ // processing.
+ return RValue::get(ConstantInt::get(Int32Ty, 0));
----------------

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
Are there cases where a call to an `always_inline` function could change the outcome?

Yes, but the result of bcp isn't guaranteed to be the same for optimized and unoptimized code. GCC's documentation says that it won't return 1 unless `-O` is used. Their code appears to back this up somewhat:

```
switch (fcode)
{
case BUILT_IN_CONSTANT_P:
{
tree val = fold_builtin_constant_p (arg0);

/* Gimplification will pull the CALL_EXPR for the builtin out of
an if condition. When not optimizing, we'll not CSE it back.
To avoid link error types of regressions, return false now. */
if (!val && !optimize)
val = integer_zero_node;

return val;
}
...
}
```

Our inlining is different of course, and I'm not sure when GCC performs their `always_inline` inlining. It may be before the above code is called. But I think we're within reasonable bounds with respect to the documentation.

(That said, GCC has a tendency to violate its own documentation. So caveat emptor, etc.)


================
Comment at: lib/Sema/SemaExpr.cpp:4973-4974

+ if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+ Arg = ConstantExpr::Create(Context, Arg);
+
----------------

Post by Richard Smith - zygoloid via Phabricator via cfe-commits

Post by Bill Wendling via Phabricator via cfe-commits

Post by Richard Smith - zygoloid via Phabricator via cfe-commits
We should create this node as part of checking that the argument is an ICE (in `SemaBuiltinConstantArg`).

It turns out that `SemaBuiltinConstantArg` isn't called for `__builtin_constant_p()`. Its argument type is `.`, which...I'm not sure what that means. It looks like a vararg, but that doesn't make sense for the builtin.
Should I change its signature in `Builtins.def`?

It's also marked `t`, which means "signature is meaningless, uses custom type checking".
The argument to `__builtin_constant_p` isn't required to be an ICE, though, so we shouldn't be calling `SemaBuiltinConstantArg` for it / creating a `ConstantExpr` wrapping it, as far as I can tell.

Right. I think I was looking at something else. I changed it.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 173974.
void added a comment.

Fix overflow flag.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 173986.
void added a comment.

ImpCast.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Ping? :-)


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Ping? I really don't want this review to go on forever.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 174529.
void added a comment.

Don't re-wrap a ConstExpr.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 174551.
void added a comment.

No function pointers


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

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

rsmith added a comment.

Looks good other than the ODR violation issue.



================
Comment at: include/clang/AST/Expr.h:37-39
+namespace {
+struct EvalInfo;
+}
----------------
It's not appropriate to declare internal-linkage types in headers. This will create ODR violations, and appears to be unnecessary -- the extra `EvaluateAsRValue` overload you declare below is not callable outside of `ExprConstant.cpp`, so can instead be declared as an internal linkage function in that file.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 174692.
void added a comment.

Remove ODR violation.


Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Should be okay now. PTAL. :-)


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1285

case Expr::ConstantExprClass:
case Stmt::ExprWithCleanupsClass:
----------------
`LLVM_FALLTHROUGH;` ?


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1285

case Expr::ConstantExprClass:
case Stmt::ExprWithCleanupsClass:
----------------

Post by Nick Desaulniers via Phabricator via cfe-commits
`LLVM_FALLTHROUGH;` ?

nvm, only needed when there's code THEN fallthrough.


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347294: Use is.constant intrinsic for __builtin_constant_p (authored by void, committed by ).

Changed prior to commit:
https://reviews.llvm.org/D54355?vs=174692&id=174731#toc

Repository:
rC Clang

https://reviews.llvm.org/D54355

Files:
include/clang/AST/Expr.h
lib/AST/ASTImporter.cpp
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/builtin-functions.cpp
test/Sema/builtins.c
test/SemaCXX/compound-literal.cpp

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

It seems this patch caused the SystemZ build bots to fail, they're now all running into assertion failures:

ICE cannot be evaluated!
UNREACHABLE executed at /home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/lib/AST/ExprConstant.cpp:11442!

See e.g. http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/20645/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Abuiltins-systemz-zvector-error.c


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: lib/AST/ExprConstant.cpp:11391

llvm_unreachable("Invalid StmtClass!");
}
----------------
@void , I assume this unreachable is the one reported by @uweigand ? Does the above switch need a case statement added?


Repository:
rC Clang

https://reviews.llvm.org/D54355

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

https://reviews.llvm.org/rC347417 makes `constexpr string_view service = "HELLO WORD SERVICE"` (P0426) broken with libstdc++

% cat a.cc
constexpr bool __constant_string_p(const char *__s) {
while (__builtin_constant_p(*__s) && *__s)
__s++;
return __builtin_constant_p(*__s);
}

constexpr bool n = __constant_string_p("a");



% fclang++ -std=c++17 -fsyntax-only a.cc
a.cc:1:16: error: constexpr function never produces a constant expression [-Winvalid-constexpr]
constexpr bool __constant_string_p(const char *__s) {
^
a.cc:2:10: note: subexpression not valid in a constant expression
while (__builtin_constant_p(*__s) && *__s)
^
a.cc:7:16: error: constexpr variable 'n' must be initialized by a constant expression
constexpr bool n = __constant_string_p("a");
^ ~~~~~~~~~~~~~~~~~~~~~~~~
a.cc:2:10: note: subexpression not valid in a constant expression
while (__builtin_constant_p(*__s) && *__s)
^
a.cc:7:20: note: in call to '__constant_string_p(&"a"[0])'
constexpr bool n = __constant_string_p("a");
^
2 errors generated.

Fourth time should be a charm...


Repository:
rC Clang

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

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Doh! I'll take a look at it.


Repository:
rC Clang

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

https://reviews.llvm.org/D54355

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Fixed in r347531.


Repository:
rC Clang

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

https://reviews.llvm.org/D54355

[Craig Topper via Phabricator via cfe-commits]

This post might be inappropriate. Click to display it.

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Post by Craig Topper via Phabricator via cfe-commits
I'm seeing a case where an inline assembly 'n' constraint is behaving differently in -O0 now. Previously we would evaluate the __builtin_constant_p as part of the EvaluateAsInt call in CodeGenFunction::EmitAsmInput, but I think we're not doing that now. This causes us to fail later because the constraint wasn't satisfied since we were in -O0 and thus we didn't optimize the rest of the expression. The test case we have is a nasty mix of __builtin_constant_p, __builtin_choose_expr, and __builtin_classify_type. I can see about sharing it if needed.

`__builtin_constant_p()` is known to have different semantics at `-O0` than it does during optimizations. That said, we shouldn't be intentionally generating incorrect code. Could you test it against gcc to see if it has the same semantics? Otherwise, please add a testcase. :-)


Repository:
rC Clang

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

https://reviews.llvm.org/D54355