Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -798,13 +798,15 @@ bool MemProfUsePass::annotateGlobalVariables(
return false;

if (!DataAccessProf) {
M.addModuleFlag(Module::Warning, "EnableDataAccessProf", 0U);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the function doesn't have any variables that get data access profiles? I assume we have an non-missing but empty DataAccessProfiles section so we wouldn't come in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the function doesn't have any variables that get data access profiles? I assume we have an non-missing but empty DataAccessProfiles section so we wouldn't come in here?

The data access profile payload is generated per binary and parsed as a whole in an optimizer pass. There are no selective parsing logic that filters the payload based on the module IR.

So there are basically three cases

  1. If memprof-annotate-static-data-prefix is false or if the module doesn't have global variables, the module won't have module flag (due to early return at line 797)
  2. If 1) is not met and the payload is empty in the memprof file, the condition at line 800 is true and module flag is zero.
  3. if 2) is not met (i.e., payload is not empty), the module will have module flag as one.

On the module flag user side, it regards case 3 as 'the module is compiled with data access profile' and treats empty section prefix as unknown, and regards the other two cases as 'the module isn't compiled with data access profiles' and override empty section prefix if needed.

I wonder if it's clearer to combine 1) and 2) so the module flag is always present as long as this pass runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, this makes sense.

I wonder if it's clearer to combine 1) and 2) so the module flag is always present as long as this pass runs.

I don't think you want to do that, because a module without any globals would get a flag value of 0, and per Module::Warning semantics, if it is combined with another module via an LTO link the first value would win. I think you want this to emit the same value, or none, for all modules using the same profile.

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