Skip to content

[ProfileData] Clean up validateRecord #95488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2024
+4 −7
Merged

[ProfileData] Clean up validateRecord #95488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions llvm/lib/ProfileData/InstrProfWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

!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);
}
}
Expand Down
Loading