[PATCH] D15158: [PGO] Instrument only base constructors and destructors.
Serge Pavlov via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 3 05:48:51 PST 2015
Thank you, Justin.
2015-12-03 5:37 GMT+06:00 Justin Bogner <mail at justinbogner.com>:
> 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.
>
Exactly. Fixed.
>
> >
> > -// 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.
>
Fixed.
>
> > + 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());
> > }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151203/48083f36/attachment-0001.html>
More information about the cfe-commits
mailing list