[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb54294e2c959: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter… (authored by nickdesaulniers). Repository: rG LLVM

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 543036. nickdesaulniers added a comment. - go back to Eli's sad (and boring) old way of doing things^U:wq Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/Expr.cpp:3444 if (CE->getCastKind() == CK_NoOp || CE->getCastKind() == CK_LValueToRValue || CE->getCastKind() == CK_ToUnion || efriedma wrote: > nickdesaulniers wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/Expr.cpp:3444 if (CE->getCastKind() == CK_NoOp || CE->getCastKind() == CK_LValueToRValue || CE->getCastKind() == CK_ToUnion || nickdesaulniers wrote: > efriedma wrote: > > efriedma

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/Expr.cpp:3444 if (CE->getCastKind() == CK_NoOp || CE->getCastKind() == CK_LValueToRValue || CE->getCastKind() == CK_ToUnion || efriedma wrote: > efriedma wrote: > > An

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added a comment. In D151587#4523571 , @efriedma wrote: >> restore E->getStorageDuration() == SD_Static check to fix libcxx regressions > > Makes sense... but it would be nice to add a testcase for whatever construct > was triggering

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added a comment. > restore E->getStorageDuration() == SD_Static check to fix libcxx regressions Makes sense... but it would be nice to add a testcase for whatever construct was triggering this. Comment at: clang/lib/AST/Expr.cpp:3462-3468

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542990. nickdesaulniers added a comment. - restore E->getStorageDuration() == SD_Static check to fix libcxx regressions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8384-8385 const MaterializeTemporaryExpr *E) { + if (Info.EvalMode == EvalInfo::EM_ConstantFold) +return false; + nickdesaulniers wrote: > removing these two lines fixes

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8384-8385 const MaterializeTemporaryExpr *E) { + if (Info.EvalMode == EvalInfo::EM_ConstantFold) +return false; + removing these two lines fixes the following

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers planned changes to this revision. nickdesaulniers added a comment. Cool, looks like the libc++ presubmit tests have already spotted some unintended changes. Will root cause+fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/Expr.cpp:3462-3468 ->isConstantInitializer(Ctx, false, Culprit); case CXXDefaultArgExprClass: return cast(this)->getExpr() ->isConstantInitializer(Ctx, false, Culprit); case

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/Expr.cpp:3462-3468 ->isConstantInitializer(Ctx, false, Culprit); case CXXDefaultArgExprClass: return cast(this)->getExpr() ->isConstantInitializer(Ctx, false, Culprit); case

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542607. nickdesaulniers marked an inline comment as done. nickdesaulniers added a comment. - force ForRef to false for CK_LValueToRValue CastExprs, as per @efriedma Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/Expr.cpp:3462-3468 ->isConstantInitializer(Ctx, false, Culprit); case CXXDefaultArgExprClass: return cast(this)->getExpr() ->isConstantInitializer(Ctx, false, Culprit); case

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added a comment. This seems to be close to a good state. Comment at: clang/lib/AST/Expr.cpp:3444 if (CE->getCastKind() == CK_NoOp || CE->getCastKind() == CK_LValueToRValue || CE->getCastKind() == CK_ToUnion || An

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542603. nickdesaulniers added a comment. - refactor common tail from IsForRef check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587 Files:

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542600. nickdesaulniers edited the summary of this revision. nickdesaulniers added a comment. - add more context to the commit description Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542594. nickdesaulniers added a comment. - reorder the check for IsForRef relative to the switch on statement class - add @efriedma's suggestions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added a comment. In D151587#4519700 , @efriedma wrote: >> It seems like perhaps we want to call EvaluateAsLValue in >> Expr::isConstantInitializer AFTER checking the statement class. Otherwise >> we're basically reimplementing that big

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added a comment. > Indeed, but I wonder why only do this when IsForRef == true (narrator: it's > not)? A MaterializeTemporaryExpr is an lvalue; it only makes sense in lvalue contexts. > Also, why for MaterializeTemporaryExpr manually force the ForRef parameter to > false? The

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added a comment. In D151587#4516772 , @efriedma wrote: > This seems to work: > > diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp > index 25d3535e..98b1e4d 100644 > --- a/clang/lib/AST/Expr.cpp > +++

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added a comment. This seems to work: diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 25d3535e..98b1e4d 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -3319,6 +3319,10 @@ bool Expr::isConstantInitializer(ASTContext , bool IsForRef,

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; efriedma wrote: > efriedma wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; efriedma wrote: > nickdesaulniers wrote: > > efriedma

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; nickdesaulniers wrote: > efriedma wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; efriedma wrote: > nickdesaulniers wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; nickdesaulniers wrote: > nickdesaulniers wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542127. nickdesaulniers added a comment. - reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587 Files: clang/lib/AST/ExprConstant.cpp

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 542125. nickdesaulniers added a comment. - add @efriedma's test case - check if the extending decl is not a reference, rather than for X or L values Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; nickdesaulniers wrote: > efriedma wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; efriedma wrote: > nickdesaulniers wrote: > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; nickdesaulniers wrote: > efriedma wrote: > > Checking

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; efriedma wrote: > Checking isLValue() doesn't

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl:60 -// CHECK: @fold_priv ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(5) addrspacecast (ptr null to ptr addrspace(5)), align 4 +// CHECK: @fold_priv ={{.*}}

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

arsenm added inline comments. Comment at: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl:60 -// CHECK: @fold_priv ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(5) addrspacecast (ptr null to ptr addrspace(5)), align 4 +// CHECK: @fold_priv ={{.*}} local_unnamed_addr

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; Checking isLValue() doesn't make sense; consider: ```

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 534090. nickdesaulniers added a comment. - test for RValue and EvalMode in LValueExprEvaluator::VisitMaterializeTemporaryExpr rather than Visit MTE's in VisitImplicitValueInitExpr. - reformat Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + efriedma wrote: > nickdesaulniers wrote: > > efriedma wrote: > > > nickdesaulniers wrote: > > > > efriedma wrote: > > > > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + nickdesaulniers wrote: > efriedma wrote: > > nickdesaulniers wrote: > > > efriedma wrote: > > > > efriedma wrote: > > > > > This

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + efriedma wrote: > nickdesaulniers wrote: > > efriedma wrote: > > > efriedma wrote: > > > > This needs a comment explaining

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:917 + bool VisitArraySubscriptExpr(const ArraySubscriptExpr *A) { +return Visit(A->getBase()) || Visit(A->getIdx()); + } Should only need to visit base, not idx. Repository:

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + nickdesaulniers wrote: > efriedma wrote: > > efriedma wrote: > > > This needs a comment explaining why we're bailing out here. > >

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 530110. nickdesaulniers added a comment. - move added test case CHECK lines closer Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587 Files:

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 530109. nickdesaulniers marked 2 inline comments as done. nickdesaulniers added a comment. - remove fixme, restore isReferenceType Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 530107. nickdesaulniers added a comment. - resurrect HasAnyMaterializeTemporaryExpr, add @efriedma's test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151587/new/ https://reviews.llvm.org/D151587

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + efriedma wrote: > efriedma wrote: > > This needs a comment explaining why we're bailing out here. > We might need to do a

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + efriedma wrote: > This needs a comment explaining why we're bailing out here. We might need to do a recursive visit still, to

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + This needs a comment explaining why we're bailing out here. Comment at:

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

nickdesaulniers updated this revision to Diff 529732. nickdesaulniers retitled this revision from "[clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first" to "[clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first".