Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
6 changes: 0 additions & 6 deletions llvm/include/llvm/IR/Function.h
Original file line number Diff line number Diff line change
Expand Up @@ -346,12 +346,6 @@ class LLVM_ABI Function : public GlobalObject, public ilist_node<Function> {
/// sample PGO, to enable the same inlines as the profiled optimized binary.
DenseSet<GlobalValue::GUID> getImportGUIDs() const;

/// Set the section prefix for this function.
void setSectionPrefix(StringRef Prefix);

/// Get the section prefix for this function.
std::optional<StringRef> getSectionPrefix() const;

/// hasGC/getGC/setGC/clearGC - The name of the garbage collection algorithm
/// to use during code generation.
bool hasGC() const {
Expand Down
11 changes: 11 additions & 0 deletions llvm/include/llvm/IR/GlobalObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,17 @@ class GlobalObject : public GlobalValue {
/// appropriate default object file section.
void setSection(StringRef S);

/// Set the section prefix for this global object.
void setSectionPrefix(StringRef Prefix);

/// Update the section prefix, unless the existing prefix is the same as
/// `KeepPrefix`.
bool updateSectionPrefix(StringRef Prefix,

Choose a reason for hiding this comment

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

Don't forget to drop this after merging #125757.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

It occurred to me that counters will increase monotonically (before it becomes max), so I asserted a global variable should not have .hot prefix under if (PSI->isColdCount(Iter->second)) { around StaticDataSplitter.cpp L 224.

std::optional<StringRef> KeepPrefix = std::nullopt);

/// Get the section prefix for this global object.
std::optional<StringRef> getSectionPrefix() const;

bool hasComdat() const { return getComdat() != nullptr; }
const Comdat *getComdat() const { return ObjComdat; }
Comdat *getComdat() { return ObjComdat; }
Expand Down
4 changes: 2 additions & 2 deletions llvm/include/llvm/IR/MDBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ class MDBuilder {
MDNode *createFunctionEntryCount(uint64_t Count, bool Synthetic,
const DenseSet<GlobalValue::GUID> *Imports);

/// Return metadata containing the section prefix for a function.
MDNode *createFunctionSectionPrefix(StringRef Prefix);
/// Return metadata containing the section prefix for a global object.
MDNode *createGlobalObjectSectionPrefix(StringRef Prefix);

/// Return metadata containing the pseudo probe descriptor for a function.
MDNode *createPseudoProbeDesc(uint64_t GUID, uint64_t Hash, StringRef FName);
Expand Down
102 changes: 99 additions & 3 deletions llvm/lib/CodeGen/StaticDataSplitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@
// The pass uses branch profile data to assign hotness based section qualifiers
// for the following types of static data:
// - Jump tables
// - Module-internal global variables
// - Constant pools (TODO)
// - Other module-internal data (TODO)
//
// For the original RFC of this pass please see
// https://discourse.llvm.org/t/rfc-profile-guided-static-data-partitioning/83744

#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/Analysis/ProfileSummaryInfo.h"
#include "llvm/CodeGen/MBFIWrapper.h"
Expand All @@ -27,9 +27,12 @@
#include "llvm/CodeGen/MachineFunctionPass.h"
#include "llvm/CodeGen/MachineJumpTableInfo.h"
#include "llvm/CodeGen/Passes.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/Module.h"
#include "llvm/InitializePasses.h"
#include "llvm/Pass.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Target/TargetLoweringObjectFile.h"

using namespace llvm;

Expand All @@ -46,6 +49,20 @@ class StaticDataSplitter : public MachineFunctionPass {
const MachineBlockFrequencyInfo *MBFI = nullptr;
const ProfileSummaryInfo *PSI = nullptr;

// If the global value is a local linkage global variable, return it.
// Otherwise, return nullptr.
const GlobalVariable *getLocalLinkageGlobalVariable(const GlobalValue *GV);

// Returns true if the global variable is in one of {.rodata, .bss, .data,
// .data.rel.ro} sections
bool inStaticDataSection(const GlobalVariable *GV, const TargetMachine &TM);

// Iterate all global variables in the module and update the section prefix
// of the module-internal data.
bool updateGlobalVariableSectionPrefix(MachineFunction &MF);

// Accummulated data profile count across machine functions in the module.
DenseMap<const GlobalVariable *, APInt> DataProfileCounts;
// Update LLVM statistics for a machine function without profiles.
void updateStatsWithoutProfiles(const MachineFunction &MF);
// Update LLVM statistics for a machine function with profiles.
Expand Down Expand Up @@ -88,13 +105,16 @@ bool StaticDataSplitter::runOnMachineFunction(MachineFunction &MF) {

bool Changed = partitionStaticDataWithProfiles(MF);

Changed |= updateGlobalVariableSectionPrefix(MF);

updateStatsWithProfiles(MF);
return Changed;
}

bool StaticDataSplitter::partitionStaticDataWithProfiles(MachineFunction &MF) {
int NumChangedJumpTables = 0;

const TargetMachine &TM = MF.getTarget();
MachineJumpTableInfo *MJTI = MF.getJumpTableInfo();

// Jump table could be used by either terminating instructions or
Expand All @@ -105,6 +125,11 @@ bool StaticDataSplitter::partitionStaticDataWithProfiles(MachineFunction &MF) {
for (const auto &MBB : MF) {
for (const MachineInstr &I : MBB) {
for (const MachineOperand &Op : I.operands()) {
if (!Op.isJTI() && !Op.isGlobal())
continue;

std::optional<uint64_t> Count = MBFI->getBlockProfileCount(&MBB);

if (Op.isJTI()) {
assert(MJTI != nullptr && "Jump table info is not available.");
const int JTI = Op.getIndex();
Expand All @@ -117,18 +142,89 @@ bool StaticDataSplitter::partitionStaticDataWithProfiles(MachineFunction &MF) {
// Hotness is based on source basic block hotness.
// TODO: PSI APIs are about instruction hotness. Introduce API for
// data access hotness.
if (PSI->isColdBlock(&MBB, MBFI))
if (Count && PSI->isColdCount(*Count))
Hotness = MachineFunctionDataHotness::Cold;

if (MJTI->updateJumpTableEntryHotness(JTI, Hotness))
++NumChangedJumpTables;
} else if (Op.isGlobal()) {
// Find global variables with local linkage
const GlobalVariable *GV =
getLocalLinkageGlobalVariable(Op.getGlobal());
if (!GV || !inStaticDataSection(GV, TM))
continue;

// Acccumulate data profile count across machine function
// instructions.
// TODO: Analyze global variable's initializers.
if (Count) {
auto [It, Inserted] =
DataProfileCounts.try_emplace(GV, APInt(128, 0));
It->second += *Count;
}
}
}
}
}
return NumChangedJumpTables > 0;
}

const GlobalVariable *
StaticDataSplitter::getLocalLinkageGlobalVariable(const GlobalValue *GV) {
if (!GV || GV->isDeclarationForLinker())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can "GV->hasLocalLinkage()" cover "GV->isDeclarationForLinker()"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I haven't thought about it before.

It turns out IR verifier requires a declaration to have one of 'valid' decl linkages, and valid decl linkages must be at least external according to isValidDeclarationLinkage.

I removed GV->isDeclarationForLinker() here and added a comment.

return nullptr;

return GV->hasLocalLinkage() ? dyn_cast<GlobalVariable>(GV) : nullptr;
}

bool StaticDataSplitter::inStaticDataSection(const GlobalVariable *GV,
const TargetMachine &TM) {
assert(GV && "Caller guaranteed");

// Skip LLVM reserved symbols.
if (GV->getName().starts_with("llvm."))
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please explain why need to skip LLVM reserved symbols?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The globals with llvm. as a name prefix are usually handled specially, by many (middle-end and back-end) passes (e.g., AsmPrinter emits special LLVM values specially), and they are skipped in this pass mostly out of conservativeness.

I moved this check before calling inStaticDataSection helper though for readability, and added a comment around the check. What do you think about this?

return false;

SectionKind Kind = TargetLoweringObjectFile::getKindForGlobal(GV, TM);
return Kind.isData() || Kind.isReadOnly() || Kind.isReadOnlyWithRel() ||
Kind.isBSS();
}

bool StaticDataSplitter::updateGlobalVariableSectionPrefix(
MachineFunction &MF) {
bool Changed = false;
for (GlobalVariable &GV : MF.getFunction().getParent()->globals()) {
if (GV.isDeclarationForLinker())
continue;
// DataProfileCounts accumulates data profile count across all machine
// function instructions, and it can't model the indirect accesses through
// other global variables' initializers.
// TODO: Analyze the users of module-internal global variables and see
// through the users' initializers. Do not place a global variable into
// unlikely section if any of its users are potentially hot.
auto Iter = DataProfileCounts.find(&GV);
if (Iter == DataProfileCounts.end())
continue;

// StaticDataSplitter is made a machine function pass rather than a module
// pass because (Lazy)MachineBlockFrequencyInfo is a machine-function
// analysis pass and cannot be used for a legacy module pass.
// As a result, we use `DataProfileCounts` to accumulate data
// profile count across machine functions and update global variable section
// prefix once per machine function.
// FIXME: Make StaticDataSplitter a module pass under new pass manager
// framework, and set global variable section prefix once per module after
// analyzing all machine functions.
if (PSI->isColdCount(Iter->second.getZExtValue())) {
Changed |= GV.updateSectionPrefix("unlikely",
Copy link
Contributor

Choose a reason for hiding this comment

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

why not prefix "split" instead of "unlikely" to align with "MachineFunctionSplitter"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that functions have .hot or .unlikely prefixes when they are placed as a whole (in the codegenprepare pass), and have .split as a prefix when basic blocks of a function are further categorized into hot and non-hot ones (in the MFS pass you mentioned).

Here the granularity is a piece of global variable, and .split would be a good name if a global variable itself is split into hot and cold ones. For instance, there should be hot and cold functions in a vtable, but it'd take substantial work to implement an ABI to support the split vtable entries.

std::make_optional(StringRef("hot")));
} else if (PSI->isHotCount(Iter->second.getZExtValue())) {
Changed |= GV.updateSectionPrefix("hot");
}
}
return Changed;
}

void StaticDataSplitter::updateStatsWithProfiles(const MachineFunction &MF) {
if (!AreStatisticsEnabled())
return;
Expand Down
6 changes: 6 additions & 0 deletions llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,7 @@ getELFSectionNameForGlobal(const GlobalObject *GO, SectionKind Kind,
}

bool HasPrefix = false;

if (const auto *F = dyn_cast<Function>(GO)) {
// Jump table hotness takes precedence over its enclosing function's hotness
// if it's known. The function's section prefix is used if jump table entry
Expand All @@ -687,6 +688,11 @@ getELFSectionNameForGlobal(const GlobalObject *GO, SectionKind Kind,
raw_svector_ostream(Name) << '.' << *Prefix;
HasPrefix = true;
}
} else if (const auto *GV = dyn_cast<GlobalVariable>(GO)) {
if (std::optional<StringRef> Prefix = GV->getSectionPrefix()) {
raw_svector_ostream(Name) << '.' << *Prefix;
HasPrefix = true;
}
}

if (UniqueSectionName) {
Expand Down
16 changes: 0 additions & 16 deletions llvm/lib/IR/Function.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1164,22 +1164,6 @@ DenseSet<GlobalValue::GUID> Function::getImportGUIDs() const {
return R;
}

void Function::setSectionPrefix(StringRef Prefix) {
MDBuilder MDB(getContext());
setMetadata(LLVMContext::MD_section_prefix,
MDB.createFunctionSectionPrefix(Prefix));
}

std::optional<StringRef> Function::getSectionPrefix() const {
if (MDNode *MD = getMetadata(LLVMContext::MD_section_prefix)) {
assert(cast<MDString>(MD->getOperand(0))->getString() ==
"function_section_prefix" &&
"Metadata not match");
return cast<MDString>(MD->getOperand(1))->getString();
}
return std::nullopt;
}

bool Function::nullPointerIsDefined() const {
return hasFnAttribute(Attribute::NullPointerIsValid);
}
Expand Down
30 changes: 30 additions & 0 deletions llvm/lib/IR/Globals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "llvm/IR/GlobalAlias.h"
#include "llvm/IR/GlobalValue.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/MDBuilder.h"
#include "llvm/IR/Module.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
Expand Down Expand Up @@ -286,6 +287,35 @@ void GlobalObject::setSection(StringRef S) {
setGlobalObjectFlag(HasSectionHashEntryBit, !S.empty());
}

void GlobalObject::setSectionPrefix(StringRef Prefix) {
MDBuilder MDB(getContext());
setMetadata(LLVMContext::MD_section_prefix,
MDB.createGlobalObjectSectionPrefix(Prefix));
}

bool GlobalObject::updateSectionPrefix(StringRef Prefix,
std::optional<StringRef> KeepPrefix) {
auto SectionPrefix = getSectionPrefix();
if (SectionPrefix && (*SectionPrefix == Prefix ||
(KeepPrefix && *SectionPrefix == *KeepPrefix)))
return false;

setSectionPrefix(Prefix);
return true;
}

std::optional<StringRef> GlobalObject::getSectionPrefix() const {
if (MDNode *MD = getMetadata(LLVMContext::MD_section_prefix)) {
[[maybe_unused]] StringRef MDName =
cast<MDString>(MD->getOperand(0))->getString();
assert((MDName == "section_prefix" ||
(isa<Function>(this) && MDName == "function_section_prefix")) &&
"Metadata not match");
return cast<MDString>(MD->getOperand(1))->getString();
}
return std::nullopt;
}

bool GlobalValue::isNobuiltinFnDef() const {
const Function *F = dyn_cast<Function>(this);
if (!F || F->empty())
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/IR/MDBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ MDNode *MDBuilder::createFunctionEntryCount(
return MDNode::get(Context, Ops);
}

MDNode *MDBuilder::createFunctionSectionPrefix(StringRef Prefix) {
return MDNode::get(
Context, {createString("function_section_prefix"), createString(Prefix)});
MDNode *MDBuilder::createGlobalObjectSectionPrefix(StringRef Prefix) {
return MDNode::get(Context,
{createString("section_prefix"), createString(Prefix)});
}

MDNode *MDBuilder::createRange(const APInt &Lo, const APInt &Hi) {
Expand Down
27 changes: 27 additions & 0 deletions llvm/test/CodeGen/X86/data-section-prefix.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
; RUN: llc -mtriple x86_64-linux-gnu -data-sections %s -o - | FileCheck %s --check-prefix=ELF
; RUN: llc -mtriple x86_64-linux-gnu -unique-section-names=0 -data-sections %s -o - | FileCheck %s --check-prefix=ELF-NOUNIQ

; RUN: llc -mtriple x86_64-windows-msvc -data-sections %s -o - | FileCheck %s --check-prefix=COFF-MSVC

; ELF: .section .data.hot.foo,
; ELF: .section .data.bar,
; ELF: .section .bss.unlikely.baz,
; ELF: .section .bss.quz,

; ELF-NOUNIQ: .section .data.hot.,"aw",@progbits,unique,1
; ELF-NOUNIQ: .section .data,"aw",@progbits,unique,2
; ELF-NOUNIQ: .section .bss.unlikely.,"aw",@nobits,unique,3
; ELF-NOUNIQ: .section .bss,"aw",@nobits,unique,4

; COFF-MSVC: .section .data,"dw",one_only,foo
; COFF-MSVC: .section .data,"dw",one_only,bar
; COFF-MSVC: .section .bss,"bw",one_only,baz
; COFF-MSVC: .section .bss,"bw",one_only,quz

@foo = global i32 1, !section_prefix !0
@bar = global i32 2
@baz = global i32 0, !section_prefix !1
@quz = global i32 0

!0 = !{!"section_prefix", !"hot"}
!1 = !{!"section_prefix", !"unlikely"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if data_section_prefix would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function and global variables are derived classes of GlobalObject, and currently function section prefix metadata has function_section_prefix.

To me, data_section_prefix carries the semantic that a global variable/object is a piece of data, which may become inaccurate. Alternative to setting global object's section prefix, I can keep function's methods as they are in TOT, and implements {set,get,update}SectionPrefix for GlobalVariables (not its superclass GlobalObject) and name it global_variable_section_prefix. How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. I get it.

Loading