Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
18 changes: 18 additions & 0 deletions llvm/include/llvm/Analysis/StaticDataProfileInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,24 @@

namespace llvm {

namespace memprof {
// Represents the eligibility status of a global variable for section prefix
// annotation. Other than AnnotationOk, each enum value indicates a specific
// reason for ineligibility.
enum class AnnotationKind : uint8_t {
AnnotationOK,
DeclForLinker,
ExplicitSection,
ReservedName,
};
/// Returns the annotation kind of the global variable \p GV.
AnnotationKind getAnnotationKind(const GlobalVariable &GV);

/// Returns true if the annotation kind of the global variable \p GV is
/// AnnotationOK.
bool IsAnnotationOK(const GlobalVariable &GV);
} // namespace memprof

/// A class that holds the constants that represent static data and their
/// profile information and provides methods to operate on them.
class StaticDataProfileInfo {
Expand Down
37 changes: 37 additions & 0 deletions llvm/lib/Analysis/StaticDataProfileInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,43 @@
#include "llvm/ProfileData/InstrProf.h"

using namespace llvm;

namespace llvm {
namespace memprof {
// Returns true iff the global variable has custom section either by
// __attribute__((section("name")))
// (https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate)
// or #pragma clang section directives
// (https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section).
static bool hasExplicitSectionName(const GlobalVariable &GVar) {
if (GVar.hasSection())
return true;

auto Attrs = GVar.getAttributes();
if (Attrs.hasAttribute("bss-section") || Attrs.hasAttribute("data-section") ||
Attrs.hasAttribute("relro-section") ||
Attrs.hasAttribute("rodata-section"))
return true;
return false;
}

AnnotationKind getAnnotationKind(const GlobalVariable &GV) {
if (GV.isDeclarationForLinker())
return AnnotationKind::DeclForLinker;
StringRef Name = GV.getName();
if (Name.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.

This check is new afaict - so is this NFC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry, you are correct.

This is from another similar call site [1] and this PR means to cover that one. Updated the PR.

[1]

// Skip 'llvm.'-prefixed global variables conservatively because they are
// often handled specially, and skip those not in static data
// sections.
if (!GV || GV->getName().starts_with("llvm.") ||
!inStaticDataSection(*GV, TM))

return AnnotationKind::ReservedName;
if (hasExplicitSectionName(GV))
return AnnotationKind::ExplicitSection;
return AnnotationKind::AnnotationOK;
}

bool IsAnnotationOK(const GlobalVariable &GV) {
return getAnnotationKind(GV) == AnnotationKind::AnnotationOK;
}
} // namespace memprof
} // namespace llvm

void StaticDataProfileInfo::addConstantProfileCount(
const Constant *C, std::optional<uint64_t> Count) {
if (!Count) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/StaticDataAnnotator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ bool StaticDataAnnotator::runOnModule(Module &M) {

bool Changed = false;
for (auto &GV : M.globals()) {
if (GV.isDeclarationForLinker())
if (!llvm::memprof::IsAnnotationOK(GV))
Copy link
Contributor

Choose a reason for hiding this comment

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

This helper function includes more checks than just isDeclarationForLinker - so is this change really NFC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I agree.

I think this PR is more of 'NFCI' (non functional change intended) or no user-visible change intended. I removed the 'NFC' tag so (unfamiliar) readers won't get (unintentionally) fooled..

Copy link
Contributor

Choose a reason for hiding this comment

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

But are the effects of this change and the other one I mentioned earlier not observable by tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But are the effects of this change and the other one I mentioned earlier not observable by tests?

Yeah, the test coverage is limited to [1] before. Improved the test coverage now, PTAL, thanks!

[1]

; LOG: Global variable var3 has explicit section name. Skip annotating.
; LOG: Global variable var4 has explicit section name. Skip annotating.

continue;

// The implementation below assumes prior passes don't set section prefixes,
Expand Down
32 changes: 10 additions & 22 deletions llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Analysis/MemoryProfileInfo.h"
#include "llvm/Analysis/OptimizationRemarkEmitter.h"
#include "llvm/Analysis/StaticDataProfileInfo.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/Function.h"
Expand Down Expand Up @@ -775,36 +776,21 @@ PreservedAnalyses MemProfUsePass::run(Module &M, ModuleAnalysisManager &AM) {
return PreservedAnalyses::none();
}

// Returns true iff the global variable has custom section either by
// __attribute__((section("name")))
// (https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate)
// or #pragma clang section directives
// (https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section).
static bool hasExplicitSectionName(const GlobalVariable &GVar) {
if (GVar.hasSection())
return true;

auto Attrs = GVar.getAttributes();
if (Attrs.hasAttribute("bss-section") || Attrs.hasAttribute("data-section") ||
Attrs.hasAttribute("relro-section") ||
Attrs.hasAttribute("rodata-section"))
return true;
return false;
}

bool MemProfUsePass::annotateGlobalVariables(
Module &M, const memprof::DataAccessProfData *DataAccessProf) {
if (!AnnotateStaticDataSectionPrefix || M.globals().empty())
return false;

if (!DataAccessProf) {
M.addModuleFlag(Module::Warning, "EnableDataAccessProf", 0U);
M.getContext().diagnose(DiagnosticInfoPGOProfile(
MemoryProfileFileName.data(),
StringRef("Data access profiles not found in memprof. Ignore "
"-memprof-annotate-static-data-prefix."),
DS_Warning));
return false;
}
M.addModuleFlag(Module::Warning, "EnableDataAccessProf", 1U);

bool Changed = false;
// Iterate all global variables in the module and annotate them based on
Expand All @@ -815,13 +801,16 @@ bool MemProfUsePass::annotateGlobalVariables(
for (GlobalVariable &GVar : M.globals()) {
assert(!GVar.getSectionPrefix().has_value() &&
"GVar shouldn't have section prefix yet");
if (GVar.isDeclarationForLinker())
continue;

if (hasExplicitSectionName(GVar)) {
auto Kind = llvm::memprof::getAnnotationKind(GVar);
switch (Kind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the switch now only handles 2 cases, this would be clearer written as an if condition like:

if (Kind != llvm::memprof::AnnotationKind::AnnotationOK) {
   HandleUnsupportedAnnotationKinds(GVar, Kind);
   continue;
}

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'll resolve this one shortly after.

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 in 34b47c9

case llvm::memprof::AnnotationKind::AnnotationOK:
break;
case llvm::memprof::AnnotationKind::ExplicitSection:
++NumOfMemProfExplicitSectionGlobalVars;
LLVM_DEBUG(dbgs() << "Global variable " << GVar.getName()
<< " has explicit section name. Skip annotating.\n");
[[fallthrough]];
default:
continue;
}

Expand All @@ -831,7 +820,6 @@ bool MemProfUsePass::annotateGlobalVariables(
// TODO: Track string content hash in the profiles and compute it inside the
// compiler to categeorize the hotness string literals.
if (Name.starts_with(".str")) {

LLVM_DEBUG(dbgs() << "Skip annotating string literal " << Name << "\n");
continue;
}
Expand Down
64 changes: 46 additions & 18 deletions llvm/test/Transforms/PGOProfile/data-access-profile.ll
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,28 @@

; RUN: rm -rf %t && split-file %s %t && cd %t

;; Read a text profile and merge it into indexed profile.
;; Read text profiles and merge them into indexed profiles.
; RUN: llvm-profdata merge --memprof-version=4 memprof.yaml -o memprof.profdata
; RUN: llvm-profdata merge --memprof-version=4 memprof-no-dap.yaml -o memprof-no-dap.profdata

;; Run optimizer pass on an IR module without IR functions, and test that global
;; variables in the module could be annotated (i.e., no early return),
; RUN: opt -passes='memprof-use<profile-filename=memprof.profdata>' -memprof-annotate-static-data-prefix \
; RUN: -debug-only=memprof -stats -S funcless-module.ll -o - 2>&1 | FileCheck %s --check-prefixes=LOG,PREFIX,STAT
; RUN: -debug-only=memprof -stats -S funcless-module.ll -o - 2>&1 | FileCheck %s --check-prefixes=LOG,IR,STAT

;; Run optimizer pass on the IR, and check the section prefix.
; RUN: opt -passes='memprof-use<profile-filename=memprof.profdata>' -memprof-annotate-static-data-prefix \
; RUN: -debug-only=memprof -stats -S input.ll -o - 2>&1 | FileCheck %s --check-prefixes=LOG,PREFIX,STAT
; RUN: -debug-only=memprof -stats -S input.ll -o - 2>&1 | FileCheck %s --check-prefixes=LOG,IR,STAT

;; Run optimizer pass without explicitly setting -memprof-annotate-static-data-prefix.
;; The output text IR shouldn't have `section_prefix`
;; Run memprof without providing memprof data. Test that IR has module flag
;; `EnableDataAccessProf` as 0.
; RUN: opt -passes='memprof-use<profile-filename=memprof-no-dap.profdata>' -memprof-annotate-static-data-prefix \
; RUN: -debug-only=memprof -stats -S input.ll -o - 2>&1 | FileCheck %s --check-prefix=FLAG

;; Run memprof without explicitly setting -memprof-annotate-static-data-prefix.
;; The output text IR shouldn't have `section_prefix` or EnableDataAccessProf module flag.
; RUN: opt -passes='memprof-use<profile-filename=memprof.profdata>' \
; RUN: -debug-only=memprof -stats -S input.ll -o - | FileCheck %s --implicit-check-not="section_prefix"
; RUN: -debug-only=memprof -stats -S input.ll -o - | FileCheck %s --check-prefix=FLAGLESS --implicit-check-not="section_prefix"

; LOG: Skip annotating string literal .str
; LOG: Global variable var1 is annotated as hot
Expand All @@ -29,29 +35,33 @@
; LOG: Global variable var4 has explicit section name. Skip annotating.

;; String literals are not annotated.
; PREFIX: @.str = unnamed_addr constant [5 x i8] c"abcde"
; PREFIX-NOT: section_prefix
; PREFIX: @var1 = global i32 123, !section_prefix !0
; IR: @.str = unnamed_addr constant [5 x i8] c"abcde"
; IR-NOT: section_prefix
; IR: @var1 = global i32 123, !section_prefix !0

;; @var.llvm.125 will be canonicalized to @var2 for profile look-up.
; PREFIX-NEXT: @var2.llvm.125 = global i64 0, !section_prefix !0
; IR-NEXT: @var2.llvm.125 = global i64 0, !section_prefix !0

;; @bar is not seen in hot symbol or known symbol set, so it won't get a section
;; prefix. Test this by testing that there is no section_prefix between @bar and
;; @foo.
; PREFIX-NEXT: @bar = global i16 3
; PREFIX-NOT: !section_prefix
; IR-NEXT: @bar = global i16 3
; IR-NOT: !section_prefix

;; @foo is unlikely.
; PREFIX-NEXT: @foo = global i8 2, !section_prefix !1
; IR-NEXT: @foo = global i8 2, !section_prefix !1

; IR-NEXT: @var3 = constant [2 x i32] [i32 12345, i32 6789], section "sec1"
; IR-NEXT: @var4 = constant [1 x i64] [i64 98765] #0

; PREFIX-NEXT: @var3 = constant [2 x i32] [i32 12345, i32 6789], section "sec1"
; PREFIX-NEXT: @var4 = constant [1 x i64] [i64 98765] #0
; IR: attributes #0 = { "rodata-section"="sec2" }

; PREFIX: attributes #0 = { "rodata-section"="sec2" }
; IR: !0 = !{!"section_prefix", !"hot"}
; IR-NEXT: !1 = !{!"section_prefix", !"unlikely"}
; IR-NEXT: !2 = !{i32 2, !"EnableDataAccessProf", i32 1}

; PREFIX: !0 = !{!"section_prefix", !"hot"}
; PREFIX-NEXT: !1 = !{!"section_prefix", !"unlikely"}
; FLAG: !{i32 2, !"EnableDataAccessProf", i32 0}
; FLAGLESS-NOT: EnableDataAccessProf

; STAT: 1 memprof - Number of global vars annotated with 'unlikely' section prefix.
; STAT: 2 memprof - Number of global vars with user-specified section (not annotated).
Expand All @@ -72,6 +82,24 @@ DataAccessProfiles:
- foo
KnownColdStrHashes: [ 999, 1001 ]
...
;--- memprof-no-dap.yaml
---
# A memprof file with without data access profiles. The heap records are simplified
# to pass profile parsing and don't need to match the IR.
HeapProfileRecords:
- GUID: 0xdeadbeef12345678
AllocSites:
- Callstack:
- { Function: 0x1111111111111111, LineOffset: 11, Column: 10, IsInlineFrame: true }
MemInfoBlock:
AllocCount: 111
TotalSize: 222
TotalLifetime: 333
TotalLifetimeAccessDensity: 444
CallSites:
- Frames:
- { Function: 0x5555555555555555, LineOffset: 55, Column: 50, IsInlineFrame: true }
...
;--- input.ll

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
Expand Down