From c181dba45921e511ef4010a74f3617375ae1f545 Mon Sep 17 00:00:00 2001 From: Alan Phipps Date: Sun, 12 Nov 2023 15:34:45 -0600 Subject: [PATCH] [InstrProfiling] Always create data variables Fixes a bug introduced by commit f95b2f1acf11 ("Reland [InstrProf][compiler-rt] Enable MC/DC Support in LLVM Source-based Code Coverage (1/3)") The InstrProfiling pass was refactored when introducing support for MC/DC such that the creation of the data variable was abstracted and called only once per function. This assumption is violated if a function is fully inlined into other instrumented callers, which means that data variables will be created for the callers but not for the inlined callee. There should be no assumption with respect to inlining in this pass. The solution (as before the refactor) is to ensure that the data variable can be created whenever the counter region is created either during ::run() or during intrinsic lowering. Also, because the data variable references both the MC/DC bitmaps as well as the counters, the creation of the bitmaps region has to happen first. This is now guaranteed in ::run() prior to the creation of the counter region and the corresponding data variable. --- .../Instrumentation/InstrProfiling.h | 4 +- .../Instrumentation/InstrProfiling.cpp | 23 ++++----- .../InstrProfiling/inline-data-var-create.ll | 47 +++++++++++++++++++ 3 files changed, 58 insertions(+), 16 deletions(-) create mode 100644 llvm/test/Instrumentation/InstrProfiling/inline-data-var-create.ll diff --git a/llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h b/llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h index d8f3e75087ace..c106e1651e804 100644 --- a/llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h +++ b/llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h @@ -51,6 +51,7 @@ class InstrProfiling : public PassInfoMixin { GlobalVariable *RegionCounters = nullptr; GlobalVariable *DataVar = nullptr; GlobalVariable *RegionBitmaps = nullptr; + uint32_t NumBitmapBytes = 0; PerFunctionProfileData() { memset(NumValueSites, 0, sizeof(uint32_t) * (IPVK_Last + 1)); @@ -156,8 +157,7 @@ class InstrProfiling : public PassInfoMixin { InstrProfSectKind IPSK); /// Create INSTR_PROF_DATA variable for counters and bitmaps. - void createDataVariable(InstrProfCntrInstBase *Inc, - InstrProfMCDCBitmapParameters *Update); + void createDataVariable(InstrProfCntrInstBase *Inc); /// Emit the section with compressed function names. void emitNameData(); diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp index 84cc8c601366e..480817a23d2c2 100644 --- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -556,7 +556,6 @@ bool InstrProfiling::run( // target value sites to enter it as field in the profile data variable. for (Function &F : M) { InstrProfCntrInstBase *FirstProfInst = nullptr; - InstrProfMCDCBitmapParameters *FirstProfMCDCParams = nullptr; for (BasicBlock &BB : F) { for (auto I = BB.begin(), E = BB.end(); I != E; I++) { if (auto *Ind = dyn_cast(I)) @@ -565,22 +564,17 @@ bool InstrProfiling::run( if (FirstProfInst == nullptr && (isa(I) || isa(I))) FirstProfInst = dyn_cast(I); - if (FirstProfMCDCParams == nullptr) - FirstProfMCDCParams = dyn_cast(I); + // If the MCDCBitmapParameters intrinsic seen, create the bitmaps. + if (const auto &Params = dyn_cast(I)) + static_cast(getOrCreateRegionBitmaps(Params)); } } } - // If the MCDCBitmapParameters intrinsic was seen, create the bitmaps. - if (FirstProfMCDCParams != nullptr) { - static_cast(getOrCreateRegionBitmaps(FirstProfMCDCParams)); - } - // Use a profile intrinsic to create the region counters and data variable. // Also create the data variable based on the MCDCParams. if (FirstProfInst != nullptr) { static_cast(getOrCreateRegionCounters(FirstProfInst)); - createDataVariable(FirstProfInst, FirstProfMCDCParams); } } @@ -1162,6 +1156,7 @@ InstrProfiling::getOrCreateRegionBitmaps(InstrProfMCDCBitmapInstBase *Inc) { // the corresponding profile section. auto *BitmapPtr = setupProfileSection(Inc, IPSK_bitmap); PD.RegionBitmaps = BitmapPtr; + PD.NumBitmapBytes = Inc->getNumBitmapBytes()->getZExtValue(); return PD.RegionBitmaps; } @@ -1238,11 +1233,13 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfCntrInstBase *Inc) { CompilerUsedVars.push_back(PD.RegionCounters); } + // Create the data variable (if it doesn't already exist). + createDataVariable(Inc); + return PD.RegionCounters; } -void InstrProfiling::createDataVariable(InstrProfCntrInstBase *Inc, - InstrProfMCDCBitmapParameters *Params) { +void InstrProfiling::createDataVariable(InstrProfCntrInstBase *Inc) { // When debug information is correlated to profile data, a data variable // is not needed. if (DebugInfoCorrelate) @@ -1305,9 +1302,7 @@ void InstrProfiling::createDataVariable(InstrProfCntrInstBase *Inc, uint64_t NumCounters = Inc->getNumCounters()->getZExtValue(); auto *CounterPtr = PD.RegionCounters; - uint64_t NumBitmapBytes = 0; - if (Params != nullptr) - NumBitmapBytes = Params->getNumBitmapBytes()->getZExtValue(); + uint64_t NumBitmapBytes = PD.NumBitmapBytes; // Create data variable. auto *IntPtrTy = M->getDataLayout().getIntPtrType(M->getContext()); diff --git a/llvm/test/Instrumentation/InstrProfiling/inline-data-var-create.ll b/llvm/test/Instrumentation/InstrProfiling/inline-data-var-create.ll new file mode 100644 index 0000000000000..7c064f547141f --- /dev/null +++ b/llvm/test/Instrumentation/InstrProfiling/inline-data-var-create.ll @@ -0,0 +1,47 @@ +;; Check that all data variables are created for instrumented functions even +;; when those functions are fully inlined into their instrumented callers prior +;; to the instrprof pass. +; RUN: opt %s -passes='instrprof' -S | FileCheck %s -check-prefix=NOINLINE +; RUN: opt %s -passes='cgscc(inline),instrprof' -S | FileCheck %s -check-prefix=INLINEFIRST +; RUN: opt %s -passes='instrprof,cgscc(inline)' -S | FileCheck %s -check-prefix=INLINEAFTER + +target triple = "x86_64-unknown-linux-gnu" + +; INLINEFIRST: @__profd_foo = private global{{.*}}zeroinitializer, i32 21 +; INLINEFIRST: @__profd_bar = private global{{.*}}zeroinitializer, i32 23 +; INLINEFIRST: @__profd_foobar = private global{{.*}}zeroinitializer, i32 99 + +; INLINEAFTER: @__profd_foobar = private global{{.*}}zeroinitializer, i32 99 +; INLINEAFTER: @__profd_foo = private global{{.*}}zeroinitializer, i32 21 +; INLINEAFTER: @__profd_bar = private global{{.*}}zeroinitializer, i32 23 + +; NOINLINE: @__profd_foobar = private global{{.*}}zeroinitializer, i32 99 +; NOINLINE: @__profd_foo = private global{{.*}}zeroinitializer, i32 21 +; NOINLINE: @__profd_bar = private global{{.*}}zeroinitializer, i32 23 + +declare void @llvm.instrprof.increment(ptr %0, i64 %1, i32 %2, i32 %3) +declare void @llvm.instrprof.mcdc.parameters(ptr %0, i64 %1, i32 %2) +@__profn_foobar = private constant [6 x i8] c"foobar" +@__profn_foo = private constant [3 x i8] c"foo" +@__profn_bar = private constant [3 x i8] c"bar" + +define internal void @foobar() { + call void @llvm.instrprof.increment(ptr @__profn_foobar, i64 123456, i32 32, i32 0) + call void @llvm.instrprof.mcdc.parameters(ptr @__profn_foobar, i64 123456, i32 99) + + ret void +} + +define void @foo() { + call void @llvm.instrprof.increment(ptr @__profn_foo, i64 123456, i32 32, i32 0) + call void @llvm.instrprof.mcdc.parameters(ptr @__profn_foo, i64 123456, i32 21) + call void @foobar() + ret void +} + +define void @bar() { + call void @llvm.instrprof.increment(ptr @__profn_bar, i64 123456, i32 32, i32 0) + call void @llvm.instrprof.mcdc.parameters(ptr @__profn_bar, i64 123456, i32 23) + call void @foobar() + ret void +}