[ProfileData] Clean up validateRecord

[kazutakahirata]

validateRecord ensures that all the values are unique except for
IPVK_IndirectCallTarget and IPVK_VTableTarget. The problem is that we
exclude them in the innermost loop.

This patch pulls the loop invariant out of the loop. While I am at
it, this patch migrates a use of getValueForSite to
getValueArrayForSite.

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

validateRecord ensures that all the values are unique except for
IPVK_IndirectCallTarget and IPVK_VTableTarget. The problem is that we
exclude them in the innermost loop.

This patch pulls the loop invariant out of the loop. While I am at
it, this patch migrates a use of getValueForSite to
getValueArrayForSite.

---

Full diff: https://github.com/llvm/llvm-project/pull/95488.diff

1 Files Affected:

• (modified) llvm/lib/ProfileData/InstrProfWriter.cpp (+4-7)

diff --git a/llvm/lib/ProfileData/InstrProfWriter.cpp b/llvm/lib/ProfileData/InstrProfWriter.cpp
index 1a9add109a360..e4e3350070e7d 100644
--- a/llvm/lib/ProfileData/InstrProfWriter.cpp
+++ b/llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -1035,16 +1035,13 @@ static const char *ValueProfKindStr[] = {
 
 Error InstrProfWriter::validateRecord(const InstrProfRecord &Func) {
   for (uint32_t VK = 0; VK <= IPVK_Last; VK++) {
-    uint32_t NS = Func.getNumValueSites(VK);
-    if (!NS)
+    if (VK == IPVK_IndirectCallTarget || VK == IPVK_VTableTarget)
       continue;
+    uint32_t NS = Func.getNumValueSites(VK);
     for (uint32_t S = 0; S < NS; S++) {
-      uint32_t ND = Func.getNumValueDataForSite(VK, S);
-      std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
       DenseSet<uint64_t> SeenValues;
-      for (uint32_t I = 0; I < ND; I++)
-        if ((VK != IPVK_IndirectCallTarget && VK != IPVK_VTableTarget) &&
-            !SeenValues.insert(VD[I].Value).second)
+      for (const auto &V : Func.getValueArrayForSite(VK, S))
+        if (!SeenValues.insert(V.Value).second)
           return make_error<InstrProfError>(instrprof_error::invalid_prof);
     }
   }

[mingmingl-llvm]

lgtm. Would be good to wait for David's feedback.

[mingmingl-llvm]

a4b543a reports errors on duplicated values.

I'm going to be away from keyboard for sometime but curious to dig the rationale before that commit (no need to wait for my dig for this change).

[david-xl]

No producer can generate dups for value profile data, so there is probably no need to specialize these two kinds. Can be cleaned up later.

[mingmingl-llvm]

Before a4b543a, 'validateRecord' allows 'Value' to be zero.

I read the commit message of a4b543a a second time, and think duplicated zeros can come from valid profiles; IndirectCallTarget and VTableTarget value profiles map runtime address to functions' and global variables' GUID. If the function or global variable isn't instrumented (e.g., per IR module opt-out of -fprofile-generate), (unique) runtime address might be mapped to (duplicated) zero (by InstrProfRecord::remapValue).

[kazutakahirata]

I happen to have a raw instrumentation profile lying around for clang (that is, clang as an application). When I generate an indexed profile with validateRecord modified to check all value profile kinds, including IPVK_IndirectCallTarget and IPVK_VTableTarget, then I get an invalid_prof error. So, duplicate values for the two kinds seem to be a real thing. I have yet to investigate where they come from though.

[mingmingl-llvm]

For IPVK_VTableTarget, a heuristic is used to find vtables in instrumented binary (

llvm-project/llvm/include/llvm/Analysis/IndirectCallVisitor.h

Lines 45 to 57 in 2d9b6a0

	// FIXME: Add support in the frontend so LLVM type intrinsics are
	// emitted without LTO. This way, added intrinsics could filter
	// non-vtable instructions and reduce instrumentation overhead.
	// Since a non-vtable profiled address is not within the address
	// range of vtable objects, it's stored as zero in indexed profiles.
	// A pass that looks up symbol with an zero hash will (almost) always
	// find nullptr and skip the actual transformation (e.g., comparison
	// of symbols). So the performance overhead from non-vtable profiled
	// address is negligible if exists at all. Comparing loaded address
	// with symbol address guarantees correctness.
	if (VTablePtr != nullptr && isa<Instruction>(VTablePtr))
	return cast<Instruction>(VTablePtr);
	}

), so runtime address of non-vtables can be collected.

• https://gcc.godbolt.org/z/ddMfrWaYd is an example from rfc.The heuristic can find the IR sequence but it's not vtable.
• InstrProfRecord::remapValue will map such runtime address to zero. It should be rare for symbol GUID to be zero; in the rare case it happens, the profile use in ICP pass gates de-virtualization by comparing object pointers with vtables for correctness.

[david-xl]

No producer can generate dups for value profile data, so there is probably no need to specialize these two kinds. Can be cleaned up later.