[InstrProfiling] Always create data variables #72069
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes a bug introduced by commit f95b2f1 ("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.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-pgo Author: Alan Phipps (evodius96) ChangesFixes a bug introduced by 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 from ::run(). Because ::run() only iterated over functions there were not fully inlined, and because it only created the data variable for the first intrinsic that it saw, data variables corresponding to functions fully inlined into other instrumented callers would end up without a data variable, resulting in loss of coverage information. This patch does the following: 1.) Move the call of createDataVariable() to getOrCreateRegionCounters() so that the creation of the data variable will happen indirectly either from ::new() or during profile intrinsic lowering when it is needed. This effectively restores the behavior prior to the refactor and ensures that all data variables are created when needed (and not duplicated). 2.) Process all MC/DC bitmap parameter intrinsics in ::run() prior to calling getOrCreateRegionCounters(). This ensures bitmap regions are created for each function including functions that are fully inlined. It also ensures that the bitmap region is created for each function prior to the creation of the data variable because it is referenced by the data variable. Again, duplication is prevented if the same parameter intrinsic is inlined into multiple functions. 3.) No longer pass the MC/DC intrinsic to createDataVariable(). This decouples the creation of the data variable from a specific MC/DC intrinsic. Instead, with #2 above, store the number of bitmap bytes required in the PerFunctionProfileData in the ProfileDataMap along with the function's CounterRegion and BitmapRegion variables. This ties the bitmap information directly to the function to which it belongs, and the data variable created for that function can reference that. Full diff: https://github.com/llvm/llvm-project/pull/72069.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h b/llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h
index d8f3e75087ace6f..c106e1651e8045f 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<InstrProfiling> {
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<InstrProfiling> {
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 84cc8c601366e62..480817a23d2c208 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<InstrProfValueProfileInst>(I))
@@ -565,22 +564,17 @@ bool InstrProfiling::run(
if (FirstProfInst == nullptr &&
(isa<InstrProfIncrementInst>(I) || isa<InstrProfCoverInst>(I)))
FirstProfInst = dyn_cast<InstrProfCntrInstBase>(I);
- if (FirstProfMCDCParams == nullptr)
- FirstProfMCDCParams = dyn_cast<InstrProfMCDCBitmapParameters>(I);
+ // If the MCDCBitmapParameters intrinsic seen, create the bitmaps.
+ if (const auto &Params = dyn_cast<InstrProfMCDCBitmapParameters>(I))
+ static_cast<void>(getOrCreateRegionBitmaps(Params));
}
}
}
- // If the MCDCBitmapParameters intrinsic was seen, create the bitmaps.
- if (FirstProfMCDCParams != nullptr) {
- static_cast<void>(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<void>(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 000000000000000..7c064f547141f3e
--- /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
+}
|
ZequanWu
approved these changes
Nov 14, 2023
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
… functions (llvm#72069) Fixes a bug introduced by commit f95b2f1 ("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 from ::run(). Because ::run() only iterated over functions there were not fully inlined, and because it only created the data variable for the first intrinsic that it saw, data variables corresponding to functions fully inlined into other instrumented callers would end up without a data variable, resulting in loss of coverage information. This patch does the following: 1.) Move the call of createDataVariable() to getOrCreateRegionCounters() so that the creation of the data variable will happen indirectly either from ::new() or during profile intrinsic lowering when it is needed. This effectively restores the behavior prior to the refactor and ensures that all data variables are created when needed (and not duplicated). 2.) Process all MC/DC bitmap parameter intrinsics in ::run() prior to calling getOrCreateRegionCounters(). This ensures bitmap regions are created for each function including functions that are fully inlined. It also ensures that the bitmap region is created for each function prior to the creation of the data variable because it is referenced by the data variable. Again, duplication is prevented if the same parameter intrinsic is inlined into multiple functions. 3.) No longer pass the MC/DC intrinsic to createDataVariable(). This decouples the creation of the data variable from a specific MC/DC intrinsic. Instead, with llvm#2 above, store the number of bitmap bytes required in the PerFunctionProfileData in the ProfileDataMap along with the function's CounterRegion and BitmapRegion variables. This ties the bitmap information directly to the function to which it belongs, and the data variable created for that function can reference that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug introduced by
commit f95b2f1 ("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 from ::run(). Because ::run() only iterated over functions there were not fully inlined, and because it only created the data variable for the first intrinsic that it saw, data variables corresponding to functions fully inlined into other instrumented callers would end up without a data variable, resulting in loss of coverage information. This patch does the following:
1.) Move the call of createDataVariable() to getOrCreateRegionCounters() so that the creation of the data variable will happen indirectly either from ::new() or during profile intrinsic lowering when it is needed. This effectively restores the behavior prior to the refactor and ensures that all data variables are created when needed (and not duplicated).
2.) Process all MC/DC bitmap parameter intrinsics in ::run() prior to calling getOrCreateRegionCounters(). This ensures bitmap regions are created for each function including functions that are fully inlined. It also ensures that the bitmap region is created for each function prior to the creation of the data variable because it is referenced by the data variable. Again, duplication is prevented if the same parameter intrinsic is inlined into multiple functions.
3.) No longer pass the MC/DC intrinsic to createDataVariable(). This decouples the creation of the data variable from a specific MC/DC intrinsic. Instead, with #2 above, store the number of bitmap bytes required in the PerFunctionProfileData in the ProfileDataMap along with the function's CounterRegion and BitmapRegion variables. This ties the bitmap information directly to the function to which it belongs, and the data variable created for that function can reference that.