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

jyknight added a comment. That example cannot be expected to ever evaluate the expression as "1" -- it doesn't in GCC, nor should it in Clang. An asm constraint of "n" or "i" (but not, e.g., "nr") must require a constant expression, and evaluating the argument as a constant expression

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

void added a comment. In D54355#1328786 , @jyknight wrote: > In D54355#1328557 , @void wrote: > > > The issue is that "`n`" is expecting an immediate value, but the result of > > the ternary operator isn't

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

jyknight added a comment. In D54355#1328557 , @void wrote: > The issue is that "`n`" is expecting an immediate value, but the result of > the ternary operator isn't calculated by the front-end, because it doesn't > "know" that the evaluation of

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

void added a comment. The issue is that "`n`" is expecting an immediate value, but the result of the ternary operator isn't calculated by the front-end, because it doesn't "know" that the evaluation of `__builtin_constant_p` shouldn't be delayed (it being compiled at `-O0`). I suspect that

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

jyknight added a comment. In D54355#1327237 , @craig.topper wrote: > Here's the test case that we have https://reviews.llvm.org/P8123 gcc seems > to accept it at O0 Smaller test case: extern unsigned long long __sdt_unsp; void foo() {

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

craig.topper added a comment. Here's the test case that we have https://reviews.llvm.org/P8123 gcc seems to accept it at O0 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54355/new/ https://reviews.llvm.org/D54355

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

void added a comment. In D54355#1326494 , @craig.topper wrote: > 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

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

craig.topper added a comment. 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

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

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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

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

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

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?

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

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.

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

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:

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

nickdesaulniers added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1285 case Expr::ConstantExprClass: case Stmt::ExprWithCleanupsClass: nickdesaulniers wrote: > `LLVM_FALLTHROUGH;` ? nvm, only needed when there's code THEN

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

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

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

void added a comment. Should be okay now. PTAL. :-) Repository: rC Clang https://reviews.llvm.org/D54355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

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

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

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

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

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

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

void added a comment. Ping? I really don't want this review to go on forever. Repository: rC Clang https://reviews.llvm.org/D54355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

void added a comment. Ping? :-) Repository: rC Clang https://reviews.llvm.org/D54355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

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

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

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

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

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

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

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

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

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(this)->getSubExpr(); +return Exp->isConstantInitializer(Ctx, false, Culprit); + } void

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

void added inline comments. Comment at: include/clang/AST/Expr.h:908-912 + static ConstantExpr *Create(const ASTContext , Expr *E) { +if (ConstantExpr *CE = dyn_cast(E)) + return CE; +return new (Context) ConstantExpr(E); + } rsmith wrote: > If

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

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

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

rsmith added inline comments. Comment at: include/clang/AST/Expr.h:908-912 + static ConstantExpr *Create(const ASTContext , Expr *E) { +if (ConstantExpr *CE = dyn_cast(E)) + return CE; +return new (Context) ConstantExpr(E); + } If we're creating

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

void added a comment. Ping? Repository: rC Clang https://reviews.llvm.org/D54355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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 ___

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

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

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

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