[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

aaupov · 2024-05-07T02:09:23Z

Switch from FuncBranchData intermediate maps (Intra/InterIndex)
to aggregated Data, same as one used by DataReader:

llvm-project/bolt/lib/Profile/DataReader.cpp

Lines 385 to 389 in e62ce1f

    
           for (const BranchInfo &BI : FBD->Data) { 
        
             if (BI.From.Name != BI.To.Name) 
        
               continue; 
        
             if (!recordBranch(BF, BI.From.Offset, BI.To.Offset, BI.Branches,

This aligns the order of the output between YAMLProfileWriter and
writeBATYAML.

Test Plan: updated bolt-address-translation-yaml.test

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

llvmbot · 2024-05-07T02:09:50Z

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

This reverts commit 224e4cc.

Sorting invalidated data indices that are used to lookup frequency
information for branch offsets or calls in NamesToBranches map.

Test Plan: updated bolt-address-translation-yaml.test

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

4 Files Affected:

(modified) bolt/lib/Profile/DataAggregator.cpp (-6)
(added) bolt/test/X86/Inputs/blarge_new_bat_order.preagg.txt (+2)
(modified) bolt/test/X86/bolt-address-translation-yaml.test (+11)
(modified) bolt/test/runtime/X86/fdata-escape-chars.ll (+1-1)

diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp
index bd96a7865009a9..4f18c402cc3a0e 100644
--- a/bolt/lib/Profile/DataAggregator.cpp
+++ b/bolt/lib/Profile/DataAggregator.cpp
@@ -641,12 +641,6 @@ void DataAggregator::processProfile(BinaryContext &BC) {
       BF.markProfiled(Flags);
   }
 
-  for (auto &FuncBranches : NamesToBranches)
-    llvm::stable_sort(FuncBranches.second.Data);
-
-  for (auto &MemEvents : NamesToMemEvents)
-    llvm::stable_sort(MemEvents.second.Data);
-
   // Release intermediate storage.
   clear(BranchLBRs);
   clear(FallthroughLBRs);
diff --git a/bolt/test/X86/Inputs/blarge_new_bat_order.preagg.txt b/bolt/test/X86/Inputs/blarge_new_bat_order.preagg.txt
new file mode 100644
index 00000000000000..e4e1f170343c62
--- /dev/null
+++ b/bolt/test/X86/Inputs/blarge_new_bat_order.preagg.txt
@@ -0,0 +1,2 @@
+B 800154 401050 20 0
+F 800159 800193 7
diff --git a/bolt/test/X86/bolt-address-translation-yaml.test b/bolt/test/X86/bolt-address-translation-yaml.test
index f67cc6361c9ef8..c7927f92c9dd95 100644
--- a/bolt/test/X86/bolt-address-translation-yaml.test
+++ b/bolt/test/X86/bolt-address-translation-yaml.test
@@ -15,6 +15,17 @@ BRANCHENTRY-YAML-CHECK:    - name: SolveCubic
 BRANCHENTRY-YAML-CHECK:      bid: 0
 BRANCHENTRY-YAML-CHECK:      hash: 0x700F19D24600000
 BRANCHENTRY-YAML-CHECK-NEXT: succ: [ { bid: 7, cnt: 1 }
+# Check that the order is correct between BAT YAML and FDATA->YAML.
+RUN: perf2bolt %t.out --pa -p %p/Inputs/blarge_new_bat_order.preagg.txt \
+RUN:   -w %t.yaml -o %t.fdata
+RUN: llvm-bolt %t.exe -data %t.fdata -w %t.yaml-fdata -o %t.null
+RUN: FileCheck --input-file %t.yaml --check-prefix ORDER-YAML-CHECK %s
+RUN: FileCheck --input-file %t.yaml-fdata --check-prefix ORDER-YAML-CHECK %s
+ORDER-YAML-CHECK:    - name: SolveCubic
+ORDER-YAML-CHECK:      bid: 3
+ORDER-YAML-CHECK:      hash: 0xDDA1DC5F69F900AC
+ORDER-YAML-CHECK-NEXT: calls: [ { off: 0x26, fid: [[#]], cnt: 20 } ]
+ORDER-YAML-CHECK-NEXT: succ: [ { bid: 5, cnt: 7 }
 # Large profile test
 RUN: perf2bolt %t.out --pa -p %p/Inputs/blarge_new_bat.preagg.txt -w %t.yaml -o %t.fdata \
 RUN:   2>&1 | FileCheck --check-prefix READ-BAT-CHECK %s
diff --git a/bolt/test/runtime/X86/fdata-escape-chars.ll b/bolt/test/runtime/X86/fdata-escape-chars.ll
index 4ea781ad184be1..a5be4e4eb13479 100644
--- a/bolt/test/runtime/X86/fdata-escape-chars.ll
+++ b/bolt/test/runtime/X86/fdata-escape-chars.ll
@@ -88,8 +88,8 @@ define internal void @static_symb_backslash_b() #0 {
 ; INSTR_CHECK: {{([[:xdigit:]]+)}}:   callq   "symb whitespace" # Count: 1
 
 ; PREAGR_FDATA_CHECK: 1 main 0 1 static\ symb\ backslash\\/1 0 0 1
-; PREAGR_FDATA_CHECK: 1 main 0 1 symb\ backslash\\ 0 0 2
 ; PREAGR_FDATA_CHECK: 1 main 0 1 symb\ whitespace 0 0 1
+; PREAGR_FDATA_CHECK: 1 main 0 1 symb\ backslash\\ 0 0 2
 ; PREAGR_FDATA_CHECK: 1 static\ symb\ backslash\\/1 0 1 symb\ whitespace 0 0 1
 ; PREAGR_FDATA_CHECK: 1 symb\ backslash\\ 0 1 symb\ whitespace 0 0 2

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

ayermolo · 2024-05-13T17:40:49Z

bolt/lib/Profile/DataAggregator.cpp

+          // Internal branch
+          const unsigned SuccIndex = getBlock(BI.To.Offset).second;
+          auto &SI = YamlBB.Successors.emplace_back(SuccessorInfo{SuccIndex});
+          SI.Count = BI.Branches;


Why can't this be part of constructor?

It can, but I decided to keep it this way for two reasons:

type impedance (int vs uint) between BI and SI requiring a cast

making it explicit where Branches and Mispreds are assigned (vs implicitly in constructor parameter order).

Type impedance means that if Count and Mispreds are passed to the constructor there needs to be an explicit cast (as the constructor doesn't accept uints). Whereas if the values are assigned to, there's an implicit cast.

For example, an explicit cast in the constructor due to type impedance:

llvm-project/bolt/lib/Profile/DataAggregator.cpp

Lines 1296 to 1298 in 31a203f

return AggregatedLBREntry{From.get(), To.get(),

static_cast<uint64_t>(Frequency.get()), Mispreds,

Type};

maksfb

Looks good on my end.

maksfb · 2024-05-13T19:14:45Z

bolt/lib/Profile/DataAggregator.cpp

-                                        yaml::bolt::CallSiteInfo &B) {
-          return A.Offset < B.Offset;
-        });
+      for (const llvm::bolt::BranchInfo &BI : Branches.Data) {


nit: no need for llvm::bolt::.

Unfortunately we need a FQ name:
llvm::bolt::BranchInfo:

llvm-project/bolt/include/bolt/Profile/DataReader.h

Line 78 in 5944579

struct BranchInfo {

llvm::bolt::DataAggregator::BranchInfo:

llvm-project/bolt/include/bolt/Profile/DataAggregator.h

Line 125 in 5944579

struct BranchInfo {

I thought about renaming the latter to TakenInfo to match its counterpart FTInfo:

llvm-project/bolt/include/bolt/Profile/DataAggregator.h

Line 120 in 5944579

struct FTInfo {

WDYT?

Dropped in a follow-up diff #92017.

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

Switch from FuncBranchData intermediate maps (Intra/InterIndex) to aggregated Data, same as one used by DataReader: https://github.com/llvm/llvm-project/blob/e62ce1f8842cca36eb14126d79dcca0a85bf6d36/bolt/lib/Profile/DataReader.cpp#L385-L389 This aligns the order of the output between YAMLProfileWriter and writeBATYAML. Test Plan: updated bolt-address-translation-yaml.test Reviewers: rafaelauler, dcci, ayermolo, maksfb Reviewed By: ayermolo, maksfb Pull Request: llvm#91289

aaupov added 2 commits May 6, 2024 19:09

[𝘀𝗽𝗿] changes to main this commit is based on

6e4239c

Created using spr 1.3.4 [skip ci]

[𝘀𝗽𝗿] initial version

049c1e3

Created using spr 1.3.4

aaupov requested review from maksfb, rafaelauler, ayermolo and dcci as code owners May 7, 2024 02:09

llvmbot added the BOLT label May 7, 2024

aaupov added 2 commits May 10, 2024 18:55

[𝘀𝗽𝗿] changes introduced through rebase

eecff96

Created using spr 1.3.4 [skip ci]

Avoid the revert

b8425a4

Created using spr 1.3.4

aaupov changed the title ~~Revert "[BOLT] Sort BranchData in DataAggregator"~~ [BOLT] Use FuncBranchData Data in writeBATYAML May 11, 2024

aaupov mentioned this pull request May 11, 2024

[BOLT][NFC] Unify Intra/InterIndex handling in writeBATYAML #91278

Closed

aaupov changed the title ~~[BOLT] Use FuncBranchData Data in writeBATYAML~~ [BOLT] Use aggregated FuncBranchData in writeBATYAML May 11, 2024

ayermolo reviewed May 13, 2024

View reviewed changes

maksfb approved these changes May 13, 2024

View reviewed changes

ayermolo approved these changes May 13, 2024

View reviewed changes

aaupov added 2 commits May 13, 2024 14:22

[𝘀𝗽𝗿] changes introduced through rebase

4370534

Created using spr 1.3.4 [skip ci]

rebase

2b8664c

Created using spr 1.3.4

aaupov changed the base branch from users/aaupov/spr/main.revert-bolt-sort-branchdata-in-dataaggregator to main May 13, 2024 21:23

aaupov merged commit 4ecf2ca into main May 13, 2024
4 of 5 checks passed

aaupov deleted the users/aaupov/spr/revert-bolt-sort-branchdata-in-dataaggregator branch May 13, 2024 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

aaupov commented May 7, 2024 •

edited

llvmbot commented May 7, 2024

ayermolo May 13, 2024

aaupov May 13, 2024

aaupov May 13, 2024

aaupov May 13, 2024

maksfb left a comment

maksfb May 13, 2024

aaupov May 13, 2024 •

edited

aaupov May 13, 2024 •

edited

	for (const BranchInfo &BI : FBD->Data) {
	if (BI.From.Name != BI.To.Name)
	continue;

	if (!recordBranch(BF, BI.From.Offset, BI.To.Offset, BI.Branches,

	return AggregatedLBREntry{From.get(), To.get(),
	static_cast<uint64_t>(Frequency.get()), Mispreds,
	Type};

[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

Conversation

aaupov commented May 7, 2024 • edited

llvmbot commented May 7, 2024

ayermolo May 13, 2024

Choose a reason for hiding this comment

aaupov May 13, 2024

Choose a reason for hiding this comment

aaupov May 13, 2024

Choose a reason for hiding this comment

aaupov May 13, 2024

Choose a reason for hiding this comment

maksfb left a comment

Choose a reason for hiding this comment

maksfb May 13, 2024

Choose a reason for hiding this comment

aaupov May 13, 2024 • edited

Choose a reason for hiding this comment

aaupov May 13, 2024 • edited

Choose a reason for hiding this comment

aaupov commented May 7, 2024 •

edited

aaupov May 13, 2024 •

edited

aaupov May 13, 2024 •

edited