[PATCH] D15158: [PGO] Instrument only base constructors and destructors.
Justin Bogner via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 2 15:37:15 PST 2015
Serge Pavlov <sepavloff at gmail.com> writes:
> sepavloff created this revision.
> sepavloff added a reviewer: bogner.
> sepavloff added subscribers: cfe-commits, silvas.
>
> Constructors and destructors may be represented by several functions
> in IR. Only the base structors correspond to source code, others
> are small pieces of code and eventually call the base variant. In
> this case instrumentation of non-base structors has little sense,
> this fix remove it. Now profile data of a declaration correspond to
> exactly one function in IR, it agrees with current logic of profile
> data loading.
>
> This change fixes PR24996.
This looks like the right thing to do. A couple of comments on the patch
below.
>
> http://reviews.llvm.org/D15158
>
> Files:
> lib/CodeGen/CGBlocks.cpp
> lib/CodeGen/CGObjC.cpp
> lib/CodeGen/CGStmt.cpp
> lib/CodeGen/CGStmtOpenMP.cpp
> lib/CodeGen/CodeGenFunction.cpp
> lib/CodeGen/CodeGenPGO.cpp
> lib/CodeGen/CodeGenPGO.h
> test/Profile/cxx-structors.cpp
> test/Profile/cxx-virtual-destructor-calls.cpp
>
> Index: test/Profile/cxx-virtual-destructor-calls.cpp
> ===================================================================
> --- test/Profile/cxx-virtual-destructor-calls.cpp
> +++ test/Profile/cxx-virtual-destructor-calls.cpp
> @@ -13,18 +13,25 @@
> virtual ~B();
> };
>
> -// Complete dtor
> -// CHECK: @__llvm_profile_name__ZN1BD1Ev = private constant [9 x i8] c"_ZN1BD1Ev"
> +// Base dtor
> +// CHECK: @__llvm_profile_name__ZN1BD2Ev = private constant [9 x i8] c"_ZN1BD2Ev"
>
> -// Deleting dtor
> -// CHECK: @__llvm_profile_name__ZN1BD0Ev = private constant [9 x i8] c"_ZN1BD0Ev"
> +// Complete dtor must not be instrumented
> +// @__llvm_profile_name__ZN1BD1Ev = private constant [9 x i8] c"_ZN1BD1Ev"
I guess these should be CHECK-NOT instead of just comments.
>
> -// Complete dtor counters and profile data
> -// CHECK: @__llvm_profile_counters__ZN1BD1Ev = private global [1 x i64] zeroinitializer
> -// CHECK: @__llvm_profile_data__ZN1BD1Ev =
> +// Deleting dtor must not be instrumented
> +// @__llvm_profile_name__ZN1BD0Ev = private constant [9 x i8] c"_ZN1BD0Ev"
>
> -// Deleting dtor counters and profile data
> -// CHECK: @__llvm_profile_counters__ZN1BD0Ev = private global [1 x i64] zeroinitializer
> -// CHECK: @__llvm_profile_data__ZN1BD0Ev =
> +// Base dtor counters and profile data
> +// CHECK: @__llvm_profile_counters__ZN1BD2Ev = private global [1 x i64] zeroinitializer
> +// CHECK: @__llvm_profile_data__ZN1BD2Ev =
> +
> +// Complete dtor counters and profile data must absent
> +// @__llvm_profile_counters__ZN1BD1Ev = private global [1 x i64] zeroinitializer
> +// @__llvm_profile_data__ZN1BD1Ev =
> +
> +// Deleting dtor counters and profile data must absent
> +// @__llvm_profile_counters__ZN1BD0Ev = private global [1 x i64] zeroinitializer
> +// @__llvm_profile_data__ZN1BD0Ev =
>
> B::~B() { }
> Index: test/Profile/cxx-structors.cpp
> ===================================================================
> --- /dev/null
> +++ test/Profile/cxx-structors.cpp
> @@ -0,0 +1,32 @@
> +// Tests for instrumentation of C++ constructors and destructors.
> +//
> +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11.0 -x c++ %s -o - -emit-llvm -fprofile-instr-generate | FileCheck %s
> +
> +struct Foo {
> + Foo() {}
> + Foo(int) {}
> + ~Foo() {}
> +};
> +
> +struct Bar : public Foo {
> + Bar() {}
> + Bar(int x) : Foo(x) {}
> + ~Bar();
> +};
> +
> +Foo foo;
> +Foo foo2(1);
> +Bar bar;
> +
> +// Profile data for complete constructors and destructors must absent.
> +
> +// CHECK-NOT: @__llvm_profile_name__ZN3FooC1Ev
> +// CHECK-NOT: @__llvm_profile_name__ZN3FooC1Ei
> +// CHECK-NOT: @__llvm_profile_name__ZN3FooD1Ev
> +// CHECK-NOT: @__llvm_profile_name__ZN3BarC1Ev
> +// CHECK-NOT: @__llvm_profile_name__ZN3BarD1Ev
> +// CHECK-NOT: @__llvm_profile_counters__ZN3FooD1Ev
> +// CHECK-NOT: @__llvm_profile_data__ZN3FooD1Ev
> +
> +int main() {
> +}
> Index: lib/CodeGen/CodeGenPGO.h
> ===================================================================
> --- lib/CodeGen/CodeGenPGO.h
> +++ lib/CodeGen/CodeGenPGO.h
> @@ -78,13 +78,11 @@
> setCurrentRegionCount(*Count);
> }
>
> - /// Check if we need to emit coverage mapping for a given declaration
> - void checkGlobalDecl(GlobalDecl GD);
> /// Assign counters to regions and configure them for PGO of a given
> /// function. Does nothing if instrumentation is not enabled and either
> /// generates global variables or associates PGO data with each of the
> /// counters depending on whether we are generating or using instrumentation.
> - void assignRegionCounters(const Decl *D, llvm::Function *Fn);
> + void assignRegionCounters(GlobalDecl GD, llvm::Function *Fn);
> /// Emit a coverage mapping range with a counter zero
> /// for an unused declaration.
> void emitEmptyCounterMapping(const Decl *D, StringRef FuncName,
> Index: lib/CodeGen/CodeGenPGO.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenPGO.cpp
> +++ lib/CodeGen/CodeGenPGO.cpp
> @@ -602,27 +602,25 @@
> return endian::read<uint64_t, little, unaligned>(Result);
> }
>
> -void CodeGenPGO::checkGlobalDecl(GlobalDecl GD) {
> - // Make sure we only emit coverage mapping for one constructor/destructor.
> - // Clang emits several functions for the constructor and the destructor of
> - // a class. Every function is instrumented, but we only want to provide
> - // coverage for one of them. Because of that we only emit the coverage mapping
> - // for the base constructor/destructor.
> - if ((isa<CXXConstructorDecl>(GD.getDecl()) &&
> - GD.getCtorType() != Ctor_Base) ||
> - (isa<CXXDestructorDecl>(GD.getDecl()) &&
> - GD.getDtorType() != Dtor_Base)) {
> - SkipCoverageMapping = true;
> - }
> -}
> -
> -void CodeGenPGO::assignRegionCounters(const Decl *D, llvm::Function *Fn) {
> +void CodeGenPGO::assignRegionCounters(GlobalDecl GD, llvm::Function *Fn) {
> + const Decl *D = GD.getDecl();
> bool InstrumentRegions = CGM.getCodeGenOpts().ProfileInstrGenerate;
> llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();
> if (!InstrumentRegions && !PGOReader)
> return;
> if (D->isImplicit())
> return;
> + // Constructors and destructors may be represented by several functions in IR.
> + // If so, instrument only base variant, others are implemented by delegation
> + // to the base one, it would be counted twice otherwise.
> + if (CGM.getTarget().getCXXABI().hasConstructorVariants() &&
> + ((isa<CXXConstructorDecl>(GD.getDecl()) &&
> + GD.getCtorType() != Ctor_Base) ||
> + (isa<CXXDestructorDecl>(GD.getDecl()) &&
> + GD.getDtorType() != Dtor_Base))) {
> + SkipCoverageMapping = true;
Is SkipCoverageMapping even necessary anymore? Since we return here, we
shouldn't hit emitCounterRegionMapping at all, I think.
> + return;
> + }
> CGM.ClearUnusedCoverageMapping(D);
> setFuncName(Fn);
>
> Index: lib/CodeGen/CodeGenFunction.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenFunction.cpp
> +++ lib/CodeGen/CodeGenFunction.cpp
> @@ -946,8 +946,7 @@
> StartFunction(GD, ResTy, Fn, FnInfo, Args, Loc, BodyRange.getBegin());
>
> // Generate the body of the function.
> - PGO.checkGlobalDecl(GD);
> - PGO.assignRegionCounters(GD.getDecl(), CurFn);
> + PGO.assignRegionCounters(GD, CurFn);
> if (isa<CXXDestructorDecl>(FD))
> EmitDestructorBody(Args);
> else if (isa<CXXConstructorDecl>(FD))
> Index: lib/CodeGen/CGStmtOpenMP.cpp
> ===================================================================
> --- lib/CodeGen/CGStmtOpenMP.cpp
> +++ lib/CodeGen/CGStmtOpenMP.cpp
> @@ -141,7 +141,7 @@
> ++Cnt, ++I;
> }
>
> - PGO.assignRegionCounters(CD, F);
> + PGO.assignRegionCounters(GlobalDecl(CD), F);
> CapturedStmtInfo->EmitBody(*this, CD->getBody());
> FinishFunction(CD->getBodyRBrace());
>
> Index: lib/CodeGen/CGStmt.cpp
> ===================================================================
> --- lib/CodeGen/CGStmt.cpp
> +++ lib/CodeGen/CGStmt.cpp
> @@ -2167,7 +2167,7 @@
> CXXThisValue = EmitLoadOfLValue(ThisLValue, Loc).getScalarVal();
> }
>
> - PGO.assignRegionCounters(CD, F);
> + PGO.assignRegionCounters(GlobalDecl(CD), F);
> CapturedStmtInfo->EmitBody(*this, CD->getBody());
> FinishFunction(CD->getBodyRBrace());
>
> Index: lib/CodeGen/CGObjC.cpp
> ===================================================================
> --- lib/CodeGen/CGObjC.cpp
> +++ lib/CodeGen/CGObjC.cpp
> @@ -557,7 +557,7 @@
> /// its pointer, name, and types registered in the class struture.
> void CodeGenFunction::GenerateObjCMethod(const ObjCMethodDecl *OMD) {
> StartObjCMethod(OMD, OMD->getClassInterface());
> - PGO.assignRegionCounters(OMD, CurFn);
> + PGO.assignRegionCounters(GlobalDecl(OMD), CurFn);
> assert(isa<CompoundStmt>(OMD->getBody()));
> incrementProfileCounter(OMD->getBody());
> EmitCompoundStmtWithoutScope(*cast<CompoundStmt>(OMD->getBody()));
> Index: lib/CodeGen/CGBlocks.cpp
> ===================================================================
> --- lib/CodeGen/CGBlocks.cpp
> +++ lib/CodeGen/CGBlocks.cpp
> @@ -1241,7 +1241,7 @@
> if (IsLambdaConversionToBlock)
> EmitLambdaBlockInvokeBody();
> else {
> - PGO.assignRegionCounters(blockDecl, fn);
> + PGO.assignRegionCounters(GlobalDecl(blockDecl), fn);
> incrementProfileCounter(blockDecl->getBody());
> EmitStmt(blockDecl->getBody());
> }
More information about the cfe-commits
mailing list