Thank you, Justin. 2015-12-03 5:37 GMT+06:00 Justin Bogner <m...@justinbogner.com>:
> Serge Pavlov <sepavl...@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()); > > } >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
