[llvm] [MemProf] Change the STACK_ID record to fixed width values (PR #116448)

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 15 15:21:08 PST 2024


https://github.com/teresajohnson updated https://github.com/llvm/llvm-project/pull/116448

>From cffb35170a710d6e3ac71dc39b645edddb43330b Mon Sep 17 00:00:00 2001
From: Teresa Johnson <tejohnson at google.com>
Date: Fri, 15 Nov 2024 14:50:40 -0800
Subject: [PATCH 1/3] [MemProf] Change the STACK_ID record to fixed width
 values

The stack ids are hashes that are close to 64 bits in size, so emitting
as a pair of 32-bit fixed-width values is more efficient than a VBR.
This reduced the summary bitcode size for a large target by about 1%.

Bump the index version and ensure we can read the old format.
---
 llvm/include/llvm/IR/ModuleSummaryIndex.h     |   2 +-
 llvm/lib/Bitcode/Reader/BitcodeReader.cpp     |  11 ++++++-
 llvm/lib/Bitcode/Writer/BitcodeWriter.cpp     |  27 +++++++++++++-----
 .../X86/Inputs/memprof-old-stackid-summary.bc | Bin 0 -> 2128 bytes
 .../X86/memprof-old-stackid-summary.ll        |  20 +++++++++++++
 5 files changed, 51 insertions(+), 9 deletions(-)
 create mode 100644 llvm/test/ThinLTO/X86/Inputs/memprof-old-stackid-summary.bc
 create mode 100644 llvm/test/ThinLTO/X86/memprof-old-stackid-summary.ll

diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 50def0eaf78867..39c60229aa1d81 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -1463,7 +1463,7 @@ class ModuleSummaryIndex {
   // in the way some record are interpreted, like flags for instance.
   // Note that incrementing this may require changes in both BitcodeReader.cpp
   // and BitcodeWriter.cpp.
-  static constexpr uint64_t BitcodeSummaryVersion = 11;
+  static constexpr uint64_t BitcodeSummaryVersion = 12;
 
   // Regular LTO module name for ASM writer
   static constexpr const char *getRegularLTOModuleName() {
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 9ca76b54a88d9d..3e6abacac27261 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -7997,7 +7997,16 @@ Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(unsigned ID) {
     case bitc::FS_STACK_IDS: { // [n x stackid]
       // Save stack ids in the reader to consult when adding stack ids from the
       // lists in the stack node and alloc node entries.
-      StackIds = ArrayRef<uint64_t>(Record);
+      if (Version <= 11) {
+        StackIds = ArrayRef<uint64_t>(Record);
+        break;
+      }
+      // This is an array of 32-bit fixed-width values, holding each 64-bit
+      // context id as a pair of adjacent (most significant first) 32-bit words.
+      assert(Record.size() % 2 == 0);
+      StackIds.reserve(Record.size() / 2);
+      for (auto R = Record.begin(); R != Record.end(); R += 2)
+        StackIds.push_back(*R << 32 | *(R + 1));
       break;
     }
 
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 5829af39cf5e2c..039e9e865b3449 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -4429,12 +4429,17 @@ void ModuleBitcodeWriterBase::writePerModuleGlobalValueSummary() {
     StackIdAbbv->Add(BitCodeAbbrevOp(bitc::FS_STACK_IDS));
     // numids x stackid
     StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
-    // FIXME: The stack ids are hashes that are close to 64 bits in size, so
-    // emitting as a pair of 32-bit fixed-width values, as we do for context
-    // ids, would be more efficient.
-    StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));
+    // The stack ids are hashes that are close to 64 bits in size, so emitting
+    // as a pair of 32-bit fixed-width values is more efficient than a VBR.
+    StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
     unsigned StackIdAbbvId = Stream.EmitAbbrev(std::move(StackIdAbbv));
-    Stream.EmitRecord(bitc::FS_STACK_IDS, Index->stackIds(), StackIdAbbvId);
+    SmallVector<uint32_t> Vals;
+    Vals.reserve(Index->stackIds().size()*2);
+    for (auto Id : Index->stackIds()) {
+      Vals.push_back(static_cast<uint32_t>(Id >> 32));
+      Vals.push_back(static_cast<uint32_t>(Id));
+    }
+    Stream.EmitRecord(bitc::FS_STACK_IDS, Vals, StackIdAbbvId);
   }
 
   // n x context id
@@ -4624,9 +4629,17 @@ void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
     StackIdAbbv->Add(BitCodeAbbrevOp(bitc::FS_STACK_IDS));
     // numids x stackid
     StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
-    StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));
+    // The stack ids are hashes that are close to 64 bits in size, so emitting
+    // as a pair of 32-bit fixed-width values is more efficient than a VBR.
+    StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
     unsigned StackIdAbbvId = Stream.EmitAbbrev(std::move(StackIdAbbv));
-    Stream.EmitRecord(bitc::FS_STACK_IDS, StackIds, StackIdAbbvId);
+    SmallVector<uint32_t> Vals;
+    Vals.reserve(StackIds.size()*2);
+    for (auto Id : StackIds) {
+      Vals.push_back(static_cast<uint32_t>(Id >> 32));
+      Vals.push_back(static_cast<uint32_t>(Id));
+    }
+    Stream.EmitRecord(bitc::FS_STACK_IDS, Vals, StackIdAbbvId);
   }
 
   // Abbrev for FS_COMBINED_PROFILE.
diff --git a/llvm/test/ThinLTO/X86/Inputs/memprof-old-stackid-summary.bc b/llvm/test/ThinLTO/X86/Inputs/memprof-old-stackid-summary.bc
new file mode 100644
index 0000000000000000000000000000000000000000..78b134cb44216e9dd72e1c140830d50fd83825bc
GIT binary patch
literal 2128
zcmX|CeM}q46(6(UHQ?+n33su>db<lGMe30*OJeNUR>8~Vd^)1 at B^QaRjbiiRgz{yK
z0Yk2>ja{vySSOQOI&@7Xb4jmh`wzL}k5o-7U at 7$xU+Sc%3MmNL<s+ox6569xs-CWW
zYm%GQyf^b^X5V|i_uh<MoLxO$j!+Ros7=dPpZe|}K0Ni`nKvs^)y*oQ2yt437E}n8
z7I3Hl_UNJGp{j|dRN?DtN-$qlMYM;DG<v4+N?TC-YJvS~sl9^|%1s*G8zq7eb$A!J
zs^<MtjB2`C(2>&IDrd!$QT7v+AVgY;_ypG+pBn$5KriEWU(yS)*kQ_W!k<FuG7CCE
zSEZ+r`p7>133VDsd|=+`C at DT7E*9B^BO2`?RZWw+U{f>edKZBY2l#hpG0U?eOEPS{
ziI246BLV4>S2DUJzgHTYm5e5Q<fgRmLnqOjA;y3r(&xp!NqmsS$9UYAQSY$$2$dh_
zg?k%(Nqo2wA9LaUjQTOq{Iu6a1Q0$(;l3c=n<WBCe8BbmAZ+gCO9xmykSv>Dr at G&J
z=>5;bmG}Q5uMNGwBK-0nogZEKlsT;mgLhvy9*MtpQ^Bw22&o&}!j1Q{2nAoEK&*i-
zFq2w+SxY=teB`yoLb0tlhMac~t{Mk)A2<B;I|$v<A(VlA^_n2PkfS~2DqB=EMdX-0
zh>r&F!N~#bCL%wlh}|GjueCGtbB23Uw1+RsZwptXF<VNs#V}hs5c7NSOB#ICA$h&{
zm=_xkNM2Ubr^|@#IFaMYKAv3LA$FWZJ+)w1<>+P3Fz;qO{AJ6Xuq7j!XE5uXur-2N
z(mUnDF8tC(yy2qc0~(aPamnk#himZRXGQSak)U*5BMs;z-=s7;8G7Z(4)G*Q{A-rj
z=CQWa6umx0r}K2hO)qiuLddY>R{u3Fnp2{AGEZ;Iri5sUVwQybZBY8<xaBj<GA(QW
z)FpXaC7(C!zlaAQ2XvA at De3Q>A)g at fpH5;uPOLSOIhNQ4FW4Hx${aJ_#5~~Wl at PO%
zrPp(YzpEIJ?k&@_c;Z^vl6F=<cf=))1n|J*TP?eBQbB<E0kh<$w>ZP<oZ&|*MlqN(
z?6}o$|0HZm%a&=;27r7yZ(SEHQJBiHKbC-}(g at h2!N+QF-(=|fYZS3POKdoaFZimt
z2Tja+?%;CHunGn^^p@$cZ6;h1kxentoR+P(=gcV?e6*$=GXokNtQr6STP6L&MPl1U
zex8LiCO3F;gC+m%B>E^~jVECGMOOVdORPJIXMEM)w%zo6$grF{xXl?hIl}^HfCz4J
z-|(<Mx7-nJw=r`fY)-e{f#`#?V_>I48u3c{yN`&cJekW9j~mIyNzfvzutBIbopgQ@
zQbj at 7pMg+@B2DJ34A9viQt8Hpq+w~wkZodC=V(uLJ5S`4_>%CG4PNBJLpprWA)QZ3
z?*!rOt%S-NldTEalo3xrI;8<It%G~X?q<N$dvk`<p>j{l1)g}yl25bhr%rNL`4zE|
zRd2FnE(6Y9ZwwD##78xf{`8^SvIX++hG<FyhL|}Xwx%&FfSHiBukFiB(eo-M4~1dT
z%{&M(ODZ(V?%i`f#=(kFt-jbNIM}LCY+~YmbCRm*u+|CocjMf39rZHW5B2tHpS!K6
z^NhEr%ir?$n`isF{0LQPBK&xx7RroIIb7pHTl>e|gI$-Ze$)P~!MgTR%g-tfL$xCG
zE1<Q&OG|t;Pz(52t9KIcSD-Uz<nc{<acbXIlTxbS`(BQ{d~D4{JaR(nlIuzWlaHLl
z?sPDOkJjWXD<DBuD0$WDy(XJxRF)fITS6}Ah9ad`a`XyIue%kx6=LSyG$4 at LO9uCx
zirfVN0nZ>&8r=&@(VtBaf at LT)8B)Rm83^}73bQD---nAy4IEEfZ-gz^pa3yjGHiun
zjLYz3AUBH;JRj&lVUtzi%KLF3GQLc~U0tOvMB^@&s-UpE{CjGbwglFdHpL=z2>A8F
zyG|8bU8oP)lkxkq=IqYp-<_hDwm;qa=9*Mo{y_;<*&NlBRW`@sHxEQoRHZ$lDXXg9
zK%8*Wk%o6Ds^d6PGr?8=qKLDfbU5|_N$smVc at rxLVhF6K6qC>Z=)-^u1XUS9?<qrc
zrR29qyXu**Zf&1A2Wj}@v$@aufB!@LtHikkqpWv=Lx+Ly#}YM)ugW0>3julybVb$*
zU1<te6h)N at G!t|PJ90kfd4({3u at 1xheuXRT#k#`(2Mty4$&~qvuE;eYSK2wS%>W#1
z+fwFjZl9;WU*M0zccIJAnvZpMws{(aI=A348SD9DbwWcu?>S!Aahz|cGu0X0Mw7AA
z%?C`z^TuPfLw((S{Ud#~Jzaf6f!YgwL%p57USGebcF^x`@9yfT^>hU~JC64BD7yX+
DUEs5-

literal 0
HcmV?d00001

diff --git a/llvm/test/ThinLTO/X86/memprof-old-stackid-summary.ll b/llvm/test/ThinLTO/X86/memprof-old-stackid-summary.ll
new file mode 100644
index 00000000000000..10048f8674a080
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/memprof-old-stackid-summary.ll
@@ -0,0 +1,20 @@
+;; Check that we can read the old STACK_ID summary format that encoded the id as
+;; a VBR8 instead of as a pair of 32-bit fixed-width values.
+;;
+;; The old bitcode was generated by the older compiler from `opt -thinlto-bc`
+;; on the following LLVM assembly:
+;;
+;; target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+;; target triple = "x86_64-unknown-linux-gnu"
+;;
+;; define void @bar() {
+;;   call void @foo(), !callsite !0
+;;   ret void
+;; }
+;;
+;; declare void @foo()
+;;
+;; !0 = !{i64 9086428284934609951}
+
+; RUN: llvm-dis %S/Inputs/memprof-old-stackid-summary.bc -o - | FileCheck %s
+; CHECK: stackIds: (9086428284934609951)

>From bc213f537d6a1aced13e7f72841ea2a8f25032fc Mon Sep 17 00:00:00 2001
From: Teresa Johnson <tejohnson at google.com>
Date: Fri, 15 Nov 2024 14:58:59 -0800
Subject: [PATCH 2/3] Fix clang format

---
 llvm/lib/Bitcode/Writer/BitcodeWriter.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 039e9e865b3449..24a4c2e8303d5a 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -4434,7 +4434,7 @@ void ModuleBitcodeWriterBase::writePerModuleGlobalValueSummary() {
     StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
     unsigned StackIdAbbvId = Stream.EmitAbbrev(std::move(StackIdAbbv));
     SmallVector<uint32_t> Vals;
-    Vals.reserve(Index->stackIds().size()*2);
+    Vals.reserve(Index->stackIds().size() * 2);
     for (auto Id : Index->stackIds()) {
       Vals.push_back(static_cast<uint32_t>(Id >> 32));
       Vals.push_back(static_cast<uint32_t>(Id));
@@ -4634,7 +4634,7 @@ void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
     StackIdAbbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
     unsigned StackIdAbbvId = Stream.EmitAbbrev(std::move(StackIdAbbv));
     SmallVector<uint32_t> Vals;
-    Vals.reserve(StackIds.size()*2);
+    Vals.reserve(StackIds.size() * 2);
     for (auto Id : StackIds) {
       Vals.push_back(static_cast<uint32_t>(Id >> 32));
       Vals.push_back(static_cast<uint32_t>(Id));

>From 3a5e12b1a4fa771217a3348c3b83816cf5669aec Mon Sep 17 00:00:00 2001
From: Teresa Johnson <tejohnson at google.com>
Date: Fri, 15 Nov 2024 15:20:51 -0800
Subject: [PATCH 3/3] Fix summary version test

---
 llvm/test/Bitcode/summary_version.ll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/llvm/test/Bitcode/summary_version.ll b/llvm/test/Bitcode/summary_version.ll
index c8d36f812c208a..c95c145a08788b 100644
--- a/llvm/test/Bitcode/summary_version.ll
+++ b/llvm/test/Bitcode/summary_version.ll
@@ -2,7 +2,7 @@
 ; RUN: opt  -module-summary  %s -o - | llvm-bcanalyzer -dump | FileCheck %s
 
 ; CHECK: <GLOBALVAL_SUMMARY_BLOCK
-; CHECK: <VERSION op0=11/>
+; CHECK: <VERSION op0=12/>
 
 
 



More information about the llvm-commits mailing list