Skip to content

Reapply "Use an abbrev to reduce size of VALUE_GUID records in ThinLTO summaries" (#90610) #90692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2024

Conversation

jvoung
Copy link
Contributor

@jvoung jvoung commented May 1, 2024

This reverts commit 2aabfc8.

Add fixes to LLD and Gold tests missed in original change.

…O summaries" (llvm#90610)

This reverts commit 2aabfc8.

Add fixes to LLD and Gold tests missed in original change.
@llvmbot llvmbot added lld lld:MachO lld:ELF lld:COFF LTO Link time optimization (regular/full LTO or ThinLTO) llvm:ir labels May 1, 2024
@llvmbot
Copy link
Member

llvmbot commented May 1, 2024

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lld-coff
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-lto

@llvm/pr-subscribers-lld-elf

Author: Jan Voung (jvoung)

Changes

This reverts commit 2aabfc8.

Add fixes to LLD and Gold tests missed in original change.


Patch is 40.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90692.diff

22 Files Affected:

  • (modified) lld/test/COFF/thinlto-index-only.ll (+3-3)
  • (modified) lld/test/ELF/lto/thinlto-emit-index.ll (+3-3)
  • (modified) lld/test/ELF/lto/thinlto-index-only.ll (+3-3)
  • (modified) lld/test/MachO/thinlto-emit-index.ll (+3-3)
  • (modified) lld/test/MachO/thinlto-index-only.ll (+3-3)
  • (modified) llvm/include/llvm/IR/ModuleSummaryIndex.h (+1-1)
  • (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+7-2)
  • (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+26-4)
  • (modified) llvm/test/Assembler/thinlto-summary.ll (+29-24)
  • (modified) llvm/test/Bitcode/summary_version.ll (+1-1)
  • (modified) llvm/test/Bitcode/thinlto-alias.ll (+2-2)
  • (modified) llvm/test/Bitcode/thinlto-func-summary-vtableref-pgo.ll (+4-4)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-callgraph-partial-sample-profile-summary.ll (+1-1)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-callgraph-pgo.ll (+1-1)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-callgraph-profile-summary.ll (+1-1)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-callgraph-sample-profile-summary.ll (+1-1)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-callgraph.ll (+1-1)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-originalnames.ll (+3-3)
  • (modified) llvm/test/Bitcode/thinlto-function-summary-paramaccess.ll (+38-38)
  • (modified) llvm/test/ThinLTO/X86/distributed_indexes.ll (+8-8)
  • (modified) llvm/test/tools/gold/X86/thinlto.ll (+5-5)
  • (modified) llvm/test/tools/llvm-lto/thinlto.ll (+2-2)
diff --git a/lld/test/COFF/thinlto-index-only.ll b/lld/test/COFF/thinlto-index-only.ll
index 8ef981d6090f12..f99134143e4dc5 100644
--- a/lld/test/COFF/thinlto-index-only.ll
+++ b/lld/test/COFF/thinlto-index-only.ll
@@ -22,8 +22,8 @@
 ; BACKEND1: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND1: <VERSION
 ; BACKEND1: <FLAGS
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-5300342847281564238|-2624081020897602054}}
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-5300342847281564238|-2624081020897602054}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3684000822 op2=3884832250}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3684000822 op2=3884832250}}
 ; BACKEND1: <COMBINED
 ; BACKEND1: <COMBINED
 ; BACKEND1: </GLOBALVAL_SUMMARY_BLOCK
@@ -37,7 +37,7 @@
 ; BACKEND2-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND2-NEXT: <VERSION
 ; BACKEND2-NEXT: <FLAGS
-; BACKEND2-NEXT: <VALUE_GUID op0=1 op1=-5300342847281564238
+; BACKEND2-NEXT: <VALUE_GUID {{.*}} op0=1 op1=3060885059 op2=1207956914
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
diff --git a/lld/test/ELF/lto/thinlto-emit-index.ll b/lld/test/ELF/lto/thinlto-emit-index.ll
index ba2ac2ceb689f5..03dfbc0200e94e 100644
--- a/lld/test/ELF/lto/thinlto-emit-index.ll
+++ b/lld/test/ELF/lto/thinlto-emit-index.ll
@@ -76,8 +76,8 @@
 ; BACKEND1: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND1: <VERSION
 ; BACKEND1: <FLAGS
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
 ; BACKEND1: <COMBINED
 ; BACKEND1: <COMBINED
 ; BACKEND1: </GLOBALVAL_SUMMARY_BLOCK
@@ -90,7 +90,7 @@
 ; BACKEND2-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND2-NEXT: <VERSION
 ; BACKEND2-NEXT: <FLAGS
-; BACKEND2-NEXT: <VALUE_GUID op0=1 op1=-5300342847281564238
+; BACKEND2-NEXT: <VALUE_GUID {{.*}} op0=1 op1=3060885059 op2=1207956914
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
diff --git a/lld/test/ELF/lto/thinlto-index-only.ll b/lld/test/ELF/lto/thinlto-index-only.ll
index abf58ce5ea4113..da60af80a00414 100644
--- a/lld/test/ELF/lto/thinlto-index-only.ll
+++ b/lld/test/ELF/lto/thinlto-index-only.ll
@@ -103,8 +103,8 @@
 ; BACKEND1: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND1: <VERSION
 ; BACKEND1: <FLAGS
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
 ; BACKEND1: <COMBINED
 ; BACKEND1: <COMBINED
 ; BACKEND1: </GLOBALVAL_SUMMARY_BLOCK
@@ -117,7 +117,7 @@
 ; BACKEND2-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND2-NEXT: <VERSION
 ; BACKEND2-NEXT: <FLAGS
-; BACKEND2-NEXT: <VALUE_GUID op0=1 op1=-5300342847281564238
+; BACKEND2-NEXT: <VALUE_GUID {{.*}} op0=1 op1=3060885059 op2=1207956914
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
diff --git a/lld/test/MachO/thinlto-emit-index.ll b/lld/test/MachO/thinlto-emit-index.ll
index 6f8d552f1435d7..7a3332ab8c93ba 100644
--- a/lld/test/MachO/thinlto-emit-index.ll
+++ b/lld/test/MachO/thinlto-emit-index.ll
@@ -76,8 +76,8 @@
 ; BACKEND1: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND1: <VERSION
 ; BACKEND1: <FLAGS
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
 ; BACKEND1: <COMBINED
 ; BACKEND1: <COMBINED
 ; BACKEND1: </GLOBALVAL_SUMMARY_BLOCK
@@ -90,7 +90,7 @@
 ; BACKEND2-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND2-NEXT: <VERSION
 ; BACKEND2-NEXT: <FLAGS
-; BACKEND2-NEXT: <VALUE_GUID op0=1 op1=-5300342847281564238
+; BACKEND2-NEXT: <VALUE_GUID {{.*}} op0=1 op1=3060885059 op2=1207956914
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
diff --git a/lld/test/MachO/thinlto-index-only.ll b/lld/test/MachO/thinlto-index-only.ll
index a97cd126ad5b2e..4844e715f49275 100644
--- a/lld/test/MachO/thinlto-index-only.ll
+++ b/lld/test/MachO/thinlto-index-only.ll
@@ -75,8 +75,8 @@
 ; BACKEND1: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND1: <VERSION
 ; BACKEND1: <FLAGS
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
-; BACKEND1: <VALUE_GUID op0={{1|2}} op1={{-3706093650706652785|-5300342847281564238}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
+; BACKEND1: <VALUE_GUID {{.*}} op0={{1|2}} {{op1=3060885059 op2=1207956914|op1=3432075125 op2=3712786831}}
 ; BACKEND1: <COMBINED
 ; BACKEND1: <COMBINED
 ; BACKEND1: </GLOBALVAL_SUMMARY_BLOCK
@@ -89,7 +89,7 @@
 ; BACKEND2-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND2-NEXT: <VERSION
 ; BACKEND2-NEXT: <FLAGS
-; BACKEND2-NEXT: <VALUE_GUID op0=1 op1=-5300342847281564238
+; BACKEND2-NEXT: <VALUE_GUID {{.*}} op0=1 op1=3060885059 op2=1207956914
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 5d137d4b3553cf..fa2a7b42c9aab3 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -1423,7 +1423,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 = 9;
+  static constexpr uint64_t BitcodeSummaryVersion = 10;
 
   // 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 0b7fcd88418894..a0779f955cf28d 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -7513,9 +7513,14 @@ Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(unsigned ID) {
       TheIndex.setFlags(Record[0]);
       break;
     }
-    case bitc::FS_VALUE_GUID: { // [valueid, refguid]
+    case bitc::FS_VALUE_GUID: { // [valueid, refguid_upper32, refguid_lower32]
       uint64_t ValueID = Record[0];
-      GlobalValue::GUID RefGUID = Record[1];
+      GlobalValue::GUID RefGUID;
+      if (Version >= 10) {
+        RefGUID = Record[1] << 32 | Record[2];
+      } else {
+        RefGUID = Record[1];
+      }
       ValueIdToValueInfoMap[ValueID] = std::make_tuple(
           TheIndex.getOrInsertValueInfo(RefGUID), RefGUID, RefGUID);
       break;
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 6d01e3b4d82189..1aaf160e91ca18 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -4299,9 +4299,20 @@ void ModuleBitcodeWriterBase::writePerModuleGlobalValueSummary() {
     return;
   }
 
+  auto Abbv = std::make_shared<BitCodeAbbrev>();
+  Abbv->Add(BitCodeAbbrevOp(bitc::FS_VALUE_GUID));
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));
+  // GUIDS often use up most of 64-bits, so encode as two Fixed 32.
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
+  unsigned ValueGuidAbbrev = Stream.EmitAbbrev(std::move(Abbv));
+
   for (const auto &GVI : valueIds()) {
     Stream.EmitRecord(bitc::FS_VALUE_GUID,
-                      ArrayRef<uint64_t>{GVI.second, GVI.first});
+                      ArrayRef<uint32_t>{GVI.second,
+                                         static_cast<uint32_t>(GVI.first >> 32),
+                                         static_cast<uint32_t>(GVI.first)},
+                      ValueGuidAbbrev);
   }
 
   if (!Index->stackIds().empty()) {
@@ -4315,7 +4326,7 @@ void ModuleBitcodeWriterBase::writePerModuleGlobalValueSummary() {
   }
 
   // Abbrev for FS_PERMODULE_PROFILE.
-  auto Abbv = std::make_shared<BitCodeAbbrev>();
+  Abbv = std::make_shared<BitCodeAbbrev>();
   Abbv->Add(BitCodeAbbrevOp(bitc::FS_PERMODULE_PROFILE));
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));   // valueid
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));   // flags
@@ -4467,9 +4478,20 @@ void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
   // Write the index flags.
   Stream.EmitRecord(bitc::FS_FLAGS, ArrayRef<uint64_t>{Index.getFlags()});
 
+  auto Abbv = std::make_shared<BitCodeAbbrev>();
+  Abbv->Add(BitCodeAbbrevOp(bitc::FS_VALUE_GUID));
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));
+  // GUIDS often use up most of 64-bits, so encode as two Fixed 32.
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 32));
+  unsigned ValueGuidAbbrev = Stream.EmitAbbrev(std::move(Abbv));
+
   for (const auto &GVI : valueIds()) {
     Stream.EmitRecord(bitc::FS_VALUE_GUID,
-                      ArrayRef<uint64_t>{GVI.second, GVI.first});
+                      ArrayRef<uint32_t>{GVI.second,
+                                         static_cast<uint32_t>(GVI.first >> 32),
+                                         static_cast<uint32_t>(GVI.first)},
+                      ValueGuidAbbrev);
   }
 
   if (!StackIdIndices.empty()) {
@@ -4488,7 +4510,7 @@ void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
   }
 
   // Abbrev for FS_COMBINED_PROFILE.
-  auto Abbv = std::make_shared<BitCodeAbbrev>();
+  Abbv = std::make_shared<BitCodeAbbrev>();
   Abbv->Add(BitCodeAbbrevOp(bitc::FS_COMBINED_PROFILE));
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));   // valueid
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));   // modid
diff --git a/llvm/test/Assembler/thinlto-summary.ll b/llvm/test/Assembler/thinlto-summary.ll
index 05dad2c7acad46..e0d866da0d8a22 100644
--- a/llvm/test/Assembler/thinlto-summary.ll
+++ b/llvm/test/Assembler/thinlto-summary.ll
@@ -46,28 +46,32 @@
 ^18 = gv: (guid: 17, summaries: (alias: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1), aliasee: ^14)))
 
 ; Test all types of TypeIdInfo on function summaries.
-^19 = gv: (guid: 18, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 4, typeIdInfo: (typeTests: (^25, ^27)))))
-^20 = gv: (guid: 19, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 8, typeIdInfo: (typeTestAssumeVCalls: (vFuncId: (^28, offset: 16))))))
-^21 = gv: (guid: 20, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 5, typeIdInfo: (typeCheckedLoadVCalls: (vFuncId: (^26, offset: 16))))))
-^22 = gv: (guid: 21, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 15, typeIdInfo: (typeTestAssumeConstVCalls: ((vFuncId: (^28, offset: 16), args: (42)), (vFuncId: (^28, offset: 24)))))))
-^23 = gv: (guid: 22, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 5, typeIdInfo: (typeCheckedLoadConstVCalls: ((vFuncId: (^29, offset: 16), args: (42)))))))
+^19 = gv: (guid: 18, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 4, typeIdInfo: (typeTests: (^26, ^28)))))
+^20 = gv: (guid: 19, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 8, typeIdInfo: (typeTestAssumeVCalls: (vFuncId: (^29, offset: 16))))))
+^21 = gv: (guid: 20, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 5, typeIdInfo: (typeCheckedLoadVCalls: (vFuncId: (^27, offset: 16))))))
+^22 = gv: (guid: 21, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 15, typeIdInfo: (typeTestAssumeConstVCalls: ((vFuncId: (^29, offset: 16), args: (42)), (vFuncId: (^29, offset: 24)))))))
+^23 = gv: (guid: 22, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 5, typeIdInfo: (typeCheckedLoadConstVCalls: ((vFuncId: (^30, offset: 16), args: (42)))))))
 
 ; Function summary with an import type of declaration
 ^24 = gv: (guid: 23, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, importType: declaration), insts: 5)))
 
+; GUID that are 64-bit
+
+^25 = gv: (guid: 9123456789101112131, summaries: (function: (module: ^0, flags: (linkage: internal, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, importType: definition), insts: 1)))
+
 ; Test TypeId summaries:
 
-^25 = typeid: (name: "_ZTS1C", summary: (typeTestRes: (kind: single, sizeM1BitWidth: 0)))
+^26 = typeid: (name: "_ZTS1C", summary: (typeTestRes: (kind: single, sizeM1BitWidth: 0)))
 ; Test TypeId with other optional fields (alignLog2/sizeM1/bitMask/inlineBits)
-^26 = typeid: (name: "_ZTS1B", summary: (typeTestRes: (kind: inline, sizeM1BitWidth: 0, alignLog2: 1, sizeM1: 2, bitMask: 3, inlineBits: 4)))
+^27 = typeid: (name: "_ZTS1B", summary: (typeTestRes: (kind: inline, sizeM1BitWidth: 0, alignLog2: 1, sizeM1: 2, bitMask: 3, inlineBits: 4)))
 ; Test the AllOnes resolution, and all kinds of WholeProgramDevirtResolution
 ; types, including all optional resolution by argument kinds.
-^27 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi")), (offset: 16, wpdRes: (kind: indir, resByArg: (args: (1, 2), byArg: (kind: indir, byte: 2, bit: 3), args: (3), byArg: (kind: uniformRetVal, info: 1), args: (4), byArg: (kind: uniqueRetVal, info: 1), args: (5), byArg: (kind: virtualConstProp)))))))
+^28 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes, sizeM1BitWidth: 7), wpdResolutions: ((offset: 0, wpdRes: (kind: branchFunnel)), (offset: 8, wpdRes: (kind: singleImpl, singleImplName: "_ZN1A1nEi")), (offset: 16, wpdRes: (kind: indir, resByArg: (args: (1, 2), byArg: (kind: indir, byte: 2, bit: 3), args: (3), byArg: (kind: uniformRetVal, info: 1), args: (4), byArg: (kind: uniqueRetVal, info: 1), args: (5), byArg: (kind: virtualConstProp)))))))
 ; Test the other kinds of type test resoultions
-^28 = typeid: (name: "_ZTS1D", summary: (typeTestRes: (kind: byteArray, sizeM1BitWidth: 0)))
-^29 = typeid: (name: "_ZTS1E", summary: (typeTestRes: (kind: unsat, sizeM1BitWidth: 0)))
-^30 = flags: 8
-^31 = blockcount: 1888
+^29 = typeid: (name: "_ZTS1D", summary: (typeTestRes: (kind: byteArray, sizeM1BitWidth: 0)))
+^30 = typeid: (name: "_ZTS1E", summary: (typeTestRes: (kind: unsat, sizeM1BitWidth: 0)))
+^31 = flags: 8
+^32 = blockcount: 1888
 
 ; Make sure we get back from llvm-dis essentially what we put in via llvm-as.
 ; CHECK: ^0 = module: (path: "thinlto-summary1.o", hash: (1369602428, 2747878711, 259090915, 2507395659, 1141468049))
@@ -91,19 +95,20 @@
 ; CHECK: ^16 = gv: (guid: 15, summaries: (function: (module: ^1, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 1, noUnwind: 1, mayThrow: 1, hasUnknownCall: 1, mustBeUnreachable: 0))))
 ; CHECK: ^17 = gv: (guid: 16, summaries: (function: (module: ^1, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 1, funcFlags: (readNone: 0, readOnly: 1, noRecurse: 0, returnDoesNotAlias: 1, noInline: 0, alwaysInline: 0, noUnwind: 0, mayThrow: 0, hasUnknownCall: 0, mustBeUnreachable: 1), calls: ((callee: ^15)))))
 ; CHECK: ^18 = gv: (guid: 17, summaries: (alias: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0, importType: definition), aliasee: ^14)))
-; CHECK: ^19 = gv: (guid: 18, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 4, typeIdInfo: (typeTests: (^25, ^27)))))
-; CHECK: ^20 = gv: (guid: 19, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 8, typeIdInfo: (typeTestAssumeVCalls: (vFuncId: (^28, offset: 16))))))
-; CHECK: ^21 = gv: (guid: 20, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 5, typeIdInfo: (typeCheckedLoadVCalls: (vFuncId: (^26, offset: 16))))))
-; CHECK: ^22 = gv: (guid: 21, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 15, typeIdInfo: (typeTestAssumeConstVCalls: ((vFuncId: (^28, offset: 16), args: (42)), (vFuncId: (^28, offset: 24)))))))
-; CHECK: ^23 = gv: (guid: 22, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 5, typeIdInfo: (typeCheckedLoadConstVCalls: ((vFuncId: (^29, offset: 16), args: (42)))))))
+; CHECK: ^19 = gv: (guid: 18, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 4, typeIdInfo: (typeTests: (^26, ^28)))))
+; CHECK: ^20 = gv: (guid: 19, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 8, typeIdInfo: (typeTestAssumeVCalls: (vFuncId: (^29, offset: 16))))))
+; CHECK: ^21 = gv: (guid: 20, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 5, typeIdInfo: (typeCheckedLoadVCalls: (vFuncId: (^27, offset: 16))))))
+; CHECK: ^22 = gv: (guid: 21, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 15, typeIdInfo: (typeTestAssumeConstVCalls: ((vFuncId: (^29, offset: 16), args: (42)), (vFuncId: (^29, offset: 24)))))))
+; CHECK: ^23 = gv: (guid: 22, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: definition), insts: 5, typeIdInfo: (typeCheckedLoadConstVCalls: ((vFuncId: (^30, offset: 16), args: (42)))))))
 ; CHECK: ^24 = gv: (guid: 23, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0, importType: declaration), insts: 5)))
-; CHECK: ^25 = typeid: (name: "_ZTS1C", summary: (typeTestRes: (kind: single, sizeM1BitWidth: 0))) ; guid = 1884921850105019584
-; CHECK: ^26 = typeid: (name: "_ZTS1B", summary: (typeTestRes: (kind: inline, sizeM1BitWidth: 0, alignLog2: 1, sizeM1: 2, bitMask: 3, inlineBits: 4))) ; guid = 6203814149063363976
-; CHECK: ^27 = typeid: (name: "_ZTS1A", summary: (typeTestRes: (kind: allOnes,...
[truncated]

@jvoung
Copy link
Contributor Author

jvoung commented May 1, 2024

Hi @teresajohnson, one more try now with a few more test updates. Thanks!

@jvoung
Copy link
Contributor Author

jvoung commented May 1, 2024

Would you mind helping merge again (still missing commit permissions)? Thank you!

@teresajohnson teresajohnson merged commit 28869a7 into llvm:main May 1, 2024
11 checks passed
@teresajohnson
Copy link
Contributor

Would you mind helping merge again (still missing commit permissions)? Thank you!

Sure, done!

@kamaub
Copy link
Contributor

kamaub commented May 2, 2024

It appears this change has exposed an issue with the test case, compiler-rt/test/profile/instrprof-gc-sections.c, failing on clang-ppc64le-linux-multistage, first build41260, and clang-ppc64le-linux-test-suite, first build27968. Neither bot builds or links with lld, all the tested binaries are linked with ld on purpose as we already had lld buildbots yet the // REQUIRES: lld-available check/guard passes anyway after detecting an lld linker available elsewhere on the host machine. I think the correct behavior would be for it to only search the build directory that the test case is suppose to test.

A solution is not known yet, I wanted to bring this up here for discussion so we could work towards one.

@jvoung
Copy link
Contributor Author

jvoung commented May 3, 2024

Hi Kamau, thanks for looking into that!

Not sure of a great solution yet either, but some notes from an initial scan:

  • lld-available depends on config.has_lld link
  • not totally sure what controls has_lld but looks like it could depend on COMPILER_RT_HAS_LLD_PYBOOL link
  • COMPILER_RT_HAS_LLD seems to be set around here if LINKER_IS_LLD or LLVM_TOOL_LLD_BUILD. I assume you don't have LLVM_TOOL_LLD_BUILD
  • LINKER_IS_LLD can be set at least here

Looking at the last location, perhaps setting LLVM_USE_LINKER and/or CMAKE_LINKER could help control that? I'm having a bit of trouble reproducing, so I'm not sure if those are the best solutions yet, but thought I'd share for discussion.

@chenzheng1030
Copy link
Collaborator

While we are still working on solution for the PPC failures, and seems it requires some time, could we revert this patch first to unblock the PPC build bots? Thanks very much. @jvoung

@jvoung
Copy link
Contributor Author

jvoung commented May 6, 2024

Hi @chenzheng1030 , sure see #91194 . Thank you for looking into the bots!

chenzheng1030 pushed a commit that referenced this pull request May 6, 2024
…n ThinLTO summaries" (#90610)" (#91194)

Reverts #90692

Breaking PPC buildbots. The bots are not meant to test LLD, but are
running a test that is using an old version of LLD without the change
(so is incompatible). Revert until a fix is found.
@chenzheng1030
Copy link
Collaborator

Hi @chenzheng1030 , sure see #91194 . Thank you for looking into the bots!

I merged the revert PR. Thanks very much for unblock PPC buildbots. If you need anything from us to resolve this LIT failure, please let us know.

@kamaub
Copy link
Contributor

kamaub commented May 6, 2024

I believe the culprit commit is [test][sanitizer] Check LINKER_IS_LLD to detect LLD.

I took d751e40 as my top of trunk and was able to reproduce locally by adding clang.16.0.1/bin/lld to my $PATH variable then building with gcc as clang-ppc64le-linux-multistage does:

cmake-3.28.2/bin/cmake -G Ninja ../llvm-project/llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=True '-DLLVM_LIT_ARGS='"'"'-v'"'"'' -DCMAKE_INSTALL_PREFIX=../stage1.install -DLLVM_ENABLE_ASSERTIONS=ON -DBUILD_SHARED_LIBS=ON -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache '-DLLVM_ENABLE_PROJECTS=clang-tools-extra;llvm;clang' -DLLVM_ENABLE_RUNTIMES=compiler-rt && ninja -j 96 && ninja -j 96 check-all

Using this I was able to reproduce FAIL: Profile-powerpc64le :: instrprof-gc-sections.c (81665 of 84128) and then after:

$ git revert 2344a72
Auto-merging compiler-rt/CMakeLists.txt
[main bcffe4472175] Revert "[test][sanitizer] Check LINKER_IS_LLD  to detect LLD"
 1 file changed, 6 insertions(+), 8 deletions(-)

it changed to UNSUPPORTED: Profile-powerpc64le :: instrprof-gc-sections.c (81472 of 84128)
I think this is the expected behavior and it also matches the behavior what Hans reported on the same changeset.

@vitalybuka can you please take a look. The reproducing condition is to observe that lld test cases are being ran and tested when lld is not build or set to be linked with (ppc links with ld by default). For some reason your change ends up searching for lld general availability which would mean it test the wrong lld and gives a false positive/negative.

@jvoung
Copy link
Contributor Author

jvoung commented May 15, 2024

Thanks @kamaub for looking into this more, and for instructions on how to reproduce (I can repro now).

Another thought is if it makes sense to change the config.lto_supported detection in the Linux+LLD case to check that LLD is built and thus understands the latest changes like the version, and not a possibly old copy of LLD. For example, in the Linux+Gold case it checks os.path.exists(os.path.join(config.llvm_shlib_dir, "LLVMgold.so")), or in the Windows case it checks os.path.exists(os.path.join(config.llvm_tools_dir, "lld-link.exe")). Can we then check os.path.exists(os.path.join(config.llvm_tools_dir, "lld")) ?

@MaskRay you mentioned something like this is in 746b5fa

Would making the lto_supported stricter make sense, or do you have a recommendation here?

Then the test should probably check if feature lto is available (since it adds flags -flto). There is some interesting fallback here where it checks lto_supported and may choose not to add -flto, but currently the test unconditionally adds -flto. Alternatively (instead adding REQUIRES), remove the -flto and let the lto_flags += config.lto_flags add it.

jvoung added a commit to jvoung/llvm-project that referenced this pull request May 16, 2024
(1) clang_lto_profgen will add `-flto` (or `-flto=thin`) when `lto_supported`:
- https://github.com/llvm/llvm-project/blob/ec36145f58d2cf93d86bc4e3be617ad7d7d8ace7/compiler-rt/test/profile/lit.cfg.py#L67
- https://github.com/llvm/llvm-project/blob/f0b3654701bde1cf7821d60698b42383edaff9f3/compiler-rt/test/lit.common.cfg.py#L775
so we may not need to add `-flto` explicitly (otherwise there is some
confusing about which is used)

(2) two tests use `clang_lto_profgen`, but only one requires `lto`.
It seems like both should require `lto` since it intends to test
`clang_lto_profgen`.

Item (2) may partially help with llvm#90692 (comment)
At the moment, some bots see LLD, but is using an old copy of LLD which
will break if we increase the `ModuleSummaryIndex::BitCodeSummaryVersion`
since the old copy of LLD does not understand the new version.
@jvoung jvoung deleted the valueguid_fixed_retry branch June 7, 2024 12:59
vitalybuka pushed a commit that referenced this pull request Jun 7, 2024
Otherwise, older copies of LLD may not understand the latest bitcode
versions (for example, if we increase
`ModuleSummaryIndex::BitCodeSummaryVersion`)

Related to
#90692 (comment)
jvoung added a commit to jvoung/llvm-project that referenced this pull request Aug 27, 2024
…O summaries (llvm#90692)

This reverts commit d71771d.
Retrying now that it seems increasing BitcodeSummaryVersion
worked in llvm#98382
jvoung added a commit that referenced this pull request Aug 27, 2024
…O summaries (#106165)

This retries #90692 which was reverted previously due to issues with
lld-available being set, even if the copy of lld is not built from
source.

This does not change any code compared to #90692 to address the
lld-available issue.
The main change w.r.t, lld-available is xfailing tests in PR #99056
(until a longer term fix is available).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lld:COFF lld:ELF lld:MachO lld llvm:ir LTO Link time optimization (regular/full LTO or ThinLTO)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants