Reverted in r332886; I added a testcase for the miscompile below to test/CodeGenCXX/reference-init.cpp
On 21 May 2018 at 13:28, Richard Smith <rich...@metafoo.co.uk> wrote: > On 21 May 2018 at 13:22, Richard Smith <rich...@metafoo.co.uk> wrote: > >> On 21 May 2018 at 09:09, Serge Pavlov via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: sepavloff >>> Date: Mon May 21 09:09:54 2018 >>> New Revision: 332847 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=332847&view=rev >>> Log: >>> [CodeGen] Recognize more cases of zero initialization >>> >>> If a variable has an initializer, codegen tries to build its value. If >>> the variable is large in size, building its value requires substantial >>> resources. It causes strange behavior from user viewpoint: compilation >>> of huge zero initialized arrays like: >>> >>> char data_1[2147483648u] = { 0 }; >>> >>> consumes enormous amount of time and memory. >>> >>> With this change codegen tries to determine if variable initializer is >>> equivalent to zero initializer. In this case variable value is not >>> constructed. >>> >>> This change fixes PR18978. >>> >>> Differential Revision: https://reviews.llvm.org/D46241 >>> >>> Removed: >>> cfe/trunk/test/SemaCXX/large-array-init.cpp >>> Modified: >>> cfe/trunk/include/clang/AST/Expr.h >>> cfe/trunk/lib/AST/ExprConstant.cpp >>> cfe/trunk/lib/CodeGen/CGExprConstant.cpp >>> cfe/trunk/test/CodeGen/const-init.c >>> cfe/trunk/test/CodeGen/designated-initializers.c >>> cfe/trunk/test/CodeGen/union-init2.c >>> cfe/trunk/test/CodeGenCXX/cxx11-initializer-aggregate.cpp >>> cfe/trunk/test/CodeGenCXX/cxx1z-initializer-aggregate.cpp >>> >>> Modified: cfe/trunk/include/clang/AST/Expr.h >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >>> AST/Expr.h?rev=332847&r1=332846&r2=332847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/include/clang/AST/Expr.h (original) >>> +++ cfe/trunk/include/clang/AST/Expr.h Mon May 21 09:09:54 2018 >>> @@ -537,6 +537,13 @@ public: >>> bool isConstantInitializer(ASTContext &Ctx, bool ForRef, >>> const Expr **Culprit = nullptr) const; >>> >>> + enum SideEffectsKind { >>> + SE_NoSideEffects, ///< Strictly evaluate the expression. >>> + SE_AllowUndefinedBehavior, ///< Allow UB that we can give a value, >>> but not >>> + ///< arbitrary unmodeled side effects. >>> + SE_AllowSideEffects ///< Allow any unmodeled side effect. >>> + }; >>> + >>> /// EvalStatus is a struct with detailed info about an evaluation in >>> progress. >>> struct EvalStatus { >>> /// Whether the evaluated expression has side effects. >>> @@ -565,6 +572,11 @@ public: >>> bool hasSideEffects() const { >>> return HasSideEffects; >>> } >>> + >>> + bool hasUnacceptableSideEffect(SideEffectsKind SEK) { >>> + return (SEK < SE_AllowSideEffects && HasSideEffects) || >>> + (SEK < SE_AllowUndefinedBehavior && HasUndefinedBehavior); >>> + } >>> }; >>> >>> /// EvalResult is a struct with detailed info about an evaluated >>> expression. >>> @@ -591,13 +603,6 @@ public: >>> /// side-effects. >>> bool EvaluateAsBooleanCondition(bool &Result, const ASTContext &Ctx) >>> const; >>> >>> - enum SideEffectsKind { >>> - SE_NoSideEffects, ///< Strictly evaluate the expression. >>> - SE_AllowUndefinedBehavior, ///< Allow UB that we can give a value, >>> but not >>> - ///< arbitrary unmodeled side effects. >>> - SE_AllowSideEffects ///< Allow any unmodeled side effect. >>> - }; >>> - >>> /// EvaluateAsInt - Return true if this is a constant which we can >>> fold and >>> /// convert to an integer, using any crazy technique that we want to. >>> bool EvaluateAsInt(llvm::APSInt &Result, const ASTContext &Ctx, >>> >>> Modified: cfe/trunk/lib/AST/ExprConstant.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprCo >>> nstant.cpp?rev=332847&r1=332846&r2=332847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/lib/AST/ExprConstant.cpp (original) >>> +++ cfe/trunk/lib/AST/ExprConstant.cpp Mon May 21 09:09:54 2018 >>> @@ -10312,12 +10312,6 @@ bool Expr::EvaluateAsBooleanCondition(bo >>> HandleConversionToBool(Scratch.Val, Result); >>> } >>> >>> -static bool hasUnacceptableSideEffect(Expr::EvalStatus &Result, >>> - Expr::SideEffectsKind SEK) { >>> - return (SEK < Expr::SE_AllowSideEffects && Result.HasSideEffects) || >>> - (SEK < Expr::SE_AllowUndefinedBehavior && >>> Result.HasUndefinedBehavior); >>> -} >>> - >>> bool Expr::EvaluateAsInt(APSInt &Result, const ASTContext &Ctx, >>> SideEffectsKind AllowSideEffects) const { >>> if (!getType()->isIntegralOrEnumerationType()) >>> @@ -10325,7 +10319,7 @@ bool Expr::EvaluateAsInt(APSInt &Result, >>> >>> EvalResult ExprResult; >>> if (!EvaluateAsRValue(ExprResult, Ctx) || !ExprResult.Val.isInt() || >>> - hasUnacceptableSideEffect(ExprResult, AllowSideEffects)) >>> + ExprResult.hasUnacceptableSideEffect(AllowSideEffects)) >>> return false; >>> >>> Result = ExprResult.Val.getInt(); >>> @@ -10339,7 +10333,7 @@ bool Expr::EvaluateAsFloat(APFloat &Resu >>> >>> EvalResult ExprResult; >>> if (!EvaluateAsRValue(ExprResult, Ctx) || !ExprResult.Val.isFloat() || >>> - hasUnacceptableSideEffect(ExprResult, AllowSideEffects)) >>> + ExprResult.hasUnacceptableSideEffect(AllowSideEffects)) >>> return false; >>> >>> Result = ExprResult.Val.getFloat(); >>> @@ -10417,7 +10411,7 @@ bool Expr::EvaluateAsInitializer(APValue >>> bool Expr::isEvaluatable(const ASTContext &Ctx, SideEffectsKind SEK) >>> const { >>> EvalResult Result; >>> return EvaluateAsRValue(Result, Ctx) && >>> - !hasUnacceptableSideEffect(Result, SEK); >>> + !Result.hasUnacceptableSideEffect(SEK); >>> } >>> >>> APSInt Expr::EvaluateKnownConstInt(const ASTContext &Ctx, >>> >>> Modified: cfe/trunk/lib/CodeGen/CGExprConstant.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CG >>> ExprConstant.cpp?rev=332847&r1=332846&r2=332847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/lib/CodeGen/CGExprConstant.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp Mon May 21 09:09:54 2018 >>> @@ -1392,20 +1392,40 @@ static QualType getNonMemoryType(CodeGen >>> return type; >>> } >>> >>> +/// Checks if the specified initializer is equivalent to zero >>> initialization. >>> +static bool isZeroInitializer(ConstantEmitter &CE, const Expr *Init) { >>> + if (auto *E = dyn_cast_or_null<CXXConstructExpr>(Init)) { >>> + CXXConstructorDecl *CD = E->getConstructor(); >>> + return CD->isDefaultConstructor() && CD->isTrivial(); >>> + } >>> + >>> + if (auto *IL = dyn_cast_or_null<InitListExpr>(Init)) { >>> + for (auto I : IL->inits()) >>> + if (!isZeroInitializer(CE, I)) >>> + return false; >>> + if (const Expr *Filler = IL->getArrayFiller()) >>> + return isZeroInitializer(CE, Filler); >>> + return true; >>> + } >>> + >>> + QualType InitTy = Init->getType(); >>> + if (InitTy->isIntegralOrEnumerationType() || >>> InitTy->isPointerType()) { >>> + Expr::EvalResult Result; >>> + if (Init->EvaluateAsRValue(Result, CE.CGM.getContext()) && >>> + !Result.hasUnacceptableSideEffect(Expr::SE_NoSideEffects)) >>> >> >> As I mentioned on the review thread, this is wrong, and you need to call >> D->evaluateValue() here instead. >> > > Actually, evaluateValue() isn't quite right here either, because we may > have drilled into the initializer already. Here's a simple example of code > you miscompile: > > int f() { static const int &&r = {0}; return r; } > > Prior to your patch this would be properly initialized; after your patch, > due to the incorrect call to EvaluateAsRValue, we "zero-initialize" the > reference. > > Your patch will also result in our re-evaluating the initializers of > variables that we've already evaluated. > > >> + return (Result.Val.isInt() && Result.Val.getInt().isNullValue()) >>> || >>> + (Result.Val.isLValue() && Result.Val.isNullPointer()); >>> + } >>> + >>> + return false; >>> +} >>> + >>> llvm::Constant *ConstantEmitter::tryEmitPrivateForVarInit(const >>> VarDecl &D) { >>> // Make a quick check if variable can be default NULL initialized >>> // and avoid going through rest of code which may do, for c++11, >>> // initialization of memory to all NULLs. >>> - if (!D.hasLocalStorage()) { >>> - QualType Ty = CGM.getContext().getBaseElementType(D.getType()); >>> - if (Ty->isRecordType()) >>> - if (const CXXConstructExpr *E = >>> - dyn_cast_or_null<CXXConstructExpr>(D.getInit())) { >>> - const CXXConstructorDecl *CD = E->getConstructor(); >>> - if (CD->isTrivial() && CD->isDefaultConstructor()) >>> - return CGM.EmitNullConstant(D.getType()); >>> - } >>> - } >>> + if (!D.hasLocalStorage() && isZeroInitializer(*this, D.getInit())) >>> + return CGM.EmitNullConstant(D.getType()); >>> >>> QualType destType = D.getType(); >>> >>> >>> Modified: cfe/trunk/test/CodeGen/const-init.c >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/c >>> onst-init.c?rev=332847&r1=332846&r2=332847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/CodeGen/const-init.c (original) >>> +++ cfe/trunk/test/CodeGen/const-init.c Mon May 21 09:09:54 2018 >>> @@ -167,7 +167,7 @@ void g30() { >>> int : 1; >>> int x; >>> } a = {}; >>> - // CHECK: @g30.a = internal global %struct.anon.1 <{ i8 undef, i32 0 >>> }>, align 1 >>> + // CHECK: @g30.a = internal global %struct.anon.1 zeroinitializer, >>> align 1 >>> #pragma pack() >>> } >>> >>> >>> Modified: cfe/trunk/test/CodeGen/designated-initializers.c >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/d >>> esignated-initializers.c?rev=332847&r1=332846&r2=332847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/CodeGen/designated-initializers.c (original) >>> +++ cfe/trunk/test/CodeGen/designated-initializers.c Mon May 21 >>> 09:09:54 2018 >>> @@ -8,7 +8,7 @@ struct foo { >>> // CHECK: @u = global %union.anon zeroinitializer >>> union { int i; float f; } u = { }; >>> >>> -// CHECK: @u2 = global { i32, [4 x i8] } { i32 0, [4 x i8] undef } >>> +// CHECK: @u2 = global %union.anon.0 zeroinitializer >>> union { int i; double f; } u2 = { }; >>> >>> // CHECK: @u3 = global %union.anon.1 zeroinitializer >>> >>> Modified: cfe/trunk/test/CodeGen/union-init2.c >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/u >>> nion-init2.c?rev=332847&r1=332846&r2=332847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/CodeGen/union-init2.c (original) >>> +++ cfe/trunk/test/CodeGen/union-init2.c Mon May 21 09:09:54 2018 >>> @@ -5,7 +5,7 @@ >>> union x {long long b;union x* a;} r = {.a = &r}; >>> >>> >>> -// CHECK: global { [3 x i8], [5 x i8] } { [3 x i8] zeroinitializer, [5 >>> x i8] undef } >>> +// CHECK: global %union.z zeroinitializer >>> union z { >>> char a[3]; >>> long long b; >>> >>> Modified: cfe/trunk/test/CodeGenCXX/cxx11-initializer-aggregate.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCX >>> X/cxx11-initializer-aggregate.cpp?rev=332847&r1=332846&r2=33 >>> 2847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/CodeGenCXX/cxx11-initializer-aggregate.cpp (original) >>> +++ cfe/trunk/test/CodeGenCXX/cxx11-initializer-aggregate.cpp Mon May >>> 21 09:09:54 2018 >>> @@ -51,3 +51,30 @@ namespace NonTrivialInit { >>> // meaningful. >>> B b[30] = {}; >>> } >>> + >>> +namespace ZeroInit { >>> + enum { Zero, One }; >>> + constexpr int zero() { return 0; } >>> + constexpr int *null() { return nullptr; } >>> + struct Filler { >>> + int x; >>> + Filler(); >>> + }; >>> + struct S1 { >>> + int x; >>> + }; >>> + >>> + // These declarations, if implemented elementwise, require huge >>> + // amout of memory and compiler time. >>> + unsigned char data_1[1024 * 1024 * 1024 * 2u] = { 0 }; >>> + unsigned char data_2[1024 * 1024 * 1024 * 2u] = { Zero }; >>> + unsigned char data_3[1024][1024][1024] = {{{0}}}; >>> + unsigned char data_4[1024 * 1024 * 1024 * 2u] = { zero() }; >>> + int *data_5[1024 * 1024 * 512] = { nullptr }; >>> + int *data_6[1024 * 1024 * 512] = { null() }; >>> + struct S1 data_7[1024 * 1024 * 512] = {{0}}; >>> + >>> + // This variable must be initialized elementwise. >>> + Filler data_e1[1024] = {}; >>> + // CHECK: getelementptr inbounds {{.*}} @_ZN8ZeroInit7data_e1E >>> +} >>> >>> Modified: cfe/trunk/test/CodeGenCXX/cxx1z-initializer-aggregate.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCX >>> X/cxx1z-initializer-aggregate.cpp?rev=332847&r1=332846&r2=33 >>> 2847&view=diff >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/CodeGenCXX/cxx1z-initializer-aggregate.cpp (original) >>> +++ cfe/trunk/test/CodeGenCXX/cxx1z-initializer-aggregate.cpp Mon May >>> 21 09:09:54 2018 >>> @@ -17,14 +17,14 @@ namespace Constant { >>> >>> C c1 = {}; >>> C c2 = {1}; >>> - // CHECK: @_ZN8Constant2c1E = global { i8 } zeroinitializer, align 1 >>> + // CHECK: @_ZN8Constant2c1E = global %"struct.Constant::C" >>> zeroinitializer, align 1 >>> // CHECK: @_ZN8Constant2c2E = global { i8 } { i8 1 }, align 1 >>> >>> // Test packing bases into tail padding. >>> D d1 = {}; >>> D d2 = {1, 2, 3}; >>> D d3 = {1}; >>> - // CHECK: @_ZN8Constant2d1E = global { i32, i8, i8 } zeroinitializer, >>> align 4 >>> + // CHECK: @_ZN8Constant2d1E = global %"struct.Constant::D" >>> zeroinitializer, align 4 >>> // CHECK: @_ZN8Constant2d2E = global { i32, i8, i8 } { i32 1, i8 2, >>> i8 3 }, align 4 >>> // CHECK: @_ZN8Constant2d3E = global { i32, i8, i8 } { i32 1, i8 0, >>> i8 0 }, align 4 >>> >>> >>> Removed: cfe/trunk/test/SemaCXX/large-array-init.cpp >>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/l >>> arge-array-init.cpp?rev=332846&view=auto >>> ============================================================ >>> ================== >>> --- cfe/trunk/test/SemaCXX/large-array-init.cpp (original) >>> +++ cfe/trunk/test/SemaCXX/large-array-init.cpp (removed) >>> @@ -1,10 +0,0 @@ >>> -// RUN: %clang_cc1 -S -o %t.ll -mllvm -debug-only=exprconstant %s 2>&1 >>> | \ >>> -// RUN: FileCheck %s >>> -// REQUIRES: asserts >>> - >>> -struct S { int i; }; >>> - >>> -static struct S arr[100000000] = {{ 0 }}; >>> -// CHECK: The number of elements to initialize: 1. >>> - >>> -struct S *foo() { return arr; } >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
