From 4f9b93abc8a8d41d7c89b3e6babeb48621b4d109 Mon Sep 17 00:00:00 2001 From: Vedant Kumar Date: Sat, 25 Feb 2017 06:35:45 +0000 Subject: [PATCH] Retry: [profiling] Fix profile counter increment when emitting selects (PR32019) 2nd attempt: the first was in r296231, but it had a use after lifetime bug. Clang has logic to lower certain conditional expressions directly into llvm select instructions. However, it does not emit the correct profile counter increment as it does this: it emits an unconditional increment of the counter for the 'then branch', even if the value selected is from the 'else branch' (this is PR32019). That means, given the following snippet, we would report that "0" is selected twice, and that "1" is never selected: int f1(int x) { return x ? 0 : 1; ^2 ^0 } f1(0); f1(1); Fix the problem by using the instrprof_increment_step intrinsic to do the proper increment. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@296245 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGExprScalar.cpp | 6 ++++-- lib/CodeGen/CodeGenFunction.h | 7 ++++--- lib/CodeGen/CodeGenPGO.cpp | 20 ++++++++++++++------ lib/CodeGen/CodeGenPGO.h | 3 ++- test/Profile/c-ternary.c | 15 +++++++++++++++ 5 files changed, 39 insertions(+), 12 deletions(-) create mode 100644 test/Profile/c-ternary.c diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp index 529642cb83..4326783382 100644 --- a/lib/CodeGen/CGExprScalar.cpp +++ b/lib/CodeGen/CGExprScalar.cpp @@ -3414,9 +3414,11 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) { // safe to evaluate the LHS and RHS unconditionally. if (isCheapEnoughToEvaluateUnconditionally(lhsExpr, CGF) && isCheapEnoughToEvaluateUnconditionally(rhsExpr, CGF)) { - CGF.incrementProfileCounter(E); - llvm::Value *CondV = CGF.EvaluateExprAsBool(condExpr); + llvm::Value *StepV = Builder.CreateZExtOrBitCast(CondV, CGF.Int64Ty); + + CGF.incrementProfileCounter(E, StepV); + llvm::Value *LHS = Visit(lhsExpr); llvm::Value *RHS = Visit(rhsExpr); if (!LHS) { diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index 72105802b4..e011c1167f 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -1127,10 +1127,11 @@ private: uint64_t LoopCount); public: - /// Increment the profiler's counter for the given statement. - void incrementProfileCounter(const Stmt *S) { + /// Increment the profiler's counter for the given statement by \p StepV. + /// If \p StepV is null, the default increment is 1. + void incrementProfileCounter(const Stmt *S, llvm::Value *StepV = nullptr) { if (CGM.getCodeGenOpts().hasProfileClangInstr()) - PGO.emitCounterIncrement(Builder, S); + PGO.emitCounterIncrement(Builder, S, StepV); PGO.setCurrentStmt(S); } diff --git a/lib/CodeGen/CodeGenPGO.cpp b/lib/CodeGen/CodeGenPGO.cpp index 530ee9b4e9..90711b5479 100644 --- a/lib/CodeGen/CodeGenPGO.cpp +++ b/lib/CodeGen/CodeGenPGO.cpp @@ -739,7 +739,8 @@ CodeGenPGO::applyFunctionAttributes(llvm::IndexedInstrProfReader *PGOReader, Fn->setEntryCount(FunctionCount); } -void CodeGenPGO::emitCounterIncrement(CGBuilderTy &Builder, const Stmt *S) { +void CodeGenPGO::emitCounterIncrement(CGBuilderTy &Builder, const Stmt *S, + llvm::Value *StepV) { if (!CGM.getCodeGenOpts().hasProfileClangInstr() || !RegionCounterMap) return; if (!Builder.GetInsertBlock()) @@ -747,11 +748,18 @@ void CodeGenPGO::emitCounterIncrement(CGBuilderTy &Builder, const Stmt *S) { unsigned Counter = (*RegionCounterMap)[S]; auto *I8PtrTy = llvm::Type::getInt8PtrTy(CGM.getLLVMContext()); - Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::instrprof_increment), - {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy), - Builder.getInt64(FunctionHash), - Builder.getInt32(NumRegionCounters), - Builder.getInt32(Counter)}); + + llvm::Value *Args[] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy), + Builder.getInt64(FunctionHash), + Builder.getInt32(NumRegionCounters), + Builder.getInt32(Counter), StepV}; + if (!StepV) + Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::instrprof_increment), + makeArrayRef(Args, 4)); + else + Builder.CreateCall( + CGM.getIntrinsic(llvm::Intrinsic::instrprof_increment_step), + makeArrayRef(Args)); } // This method either inserts a call to the profile run-time during diff --git a/lib/CodeGen/CodeGenPGO.h b/lib/CodeGen/CodeGenPGO.h index 4f229cde63..0026df570b 100644 --- a/lib/CodeGen/CodeGenPGO.h +++ b/lib/CodeGen/CodeGenPGO.h @@ -105,7 +105,8 @@ private: void emitCounterRegionMapping(const Decl *D); public: - void emitCounterIncrement(CGBuilderTy &Builder, const Stmt *S); + void emitCounterIncrement(CGBuilderTy &Builder, const Stmt *S, + llvm::Value *StepV); /// Return the region count for the counter at the given index. uint64_t getRegionCount(const Stmt *S) { diff --git a/test/Profile/c-ternary.c b/test/Profile/c-ternary.c new file mode 100644 index 0000000000..09e8329235 --- /dev/null +++ b/test/Profile/c-ternary.c @@ -0,0 +1,15 @@ +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11.0 -x c %s -o - -emit-llvm -fprofile-instrument=clang | FileCheck %s + +// PR32019: Clang can lower some ternary operator expressions to select +// instructions. Make sure we only increment the profile counter for the +// condition when the condition evaluates to true. +// CHECK-LABEL: define i32 @f1 +int f1(int x) { +// CHECK: [[TOBOOL:%.*]] = icmp ne i32 %1, 0 +// CHECK-NEXT: [[STEP:%.*]] = zext i1 [[TOBOOL]] to i64 +// CHECK-NEXT: [[COUNTER:%.*]] = load i64, i64* getelementptr inbounds ([2 x i64], [2 x i64]* @__profc_f1, i64 0, i64 1) +// CHECK-NEXT: add i64 [[COUNTER]], [[STEP]] +// CHECK: [[COND:%.*]] = select i1 [[TOBOOL]], i32 0, i32 1 + return x ? 0 : 1; +// CHECK: ret i32 [[COND]] +} -- 2.40.0