[ThinLTO] Don't emit original GUID for locals to distributed indexes

[tejohnson]

Summary

In ThinLTO for locals we normally compute the GUID from the name after
prepending the source path to get a unique global id. SamplePGO indirect
call profiles contain the target GUID without this uniquification,
however (unless compiling with -funique-internal-linkage-names).
Therefore, the index contains the original GUID of the local symbols
(without module path prepended to uniquify), in order to correctly
handle the call edges added for these indirect call profile targets
with SamplePGO.

We were emitting these to the combined index when writing it out as
bitcode, which is unnecessary and causes overhead when writing out the
indexes for distributed backends. The only use of the original GUID name
is in the thin link. Suppress it in that case. This reduced the thin
link time for a large distributed build by about 7%, and the aggregate
size of the serialized indexes by over 2%.

Continue to print it when writing out the full index, since that is just
used for debugging and testing.

Update a distributed thinlto index test to contain a local and ensure
that we don't get a COMBINED_ORIGINAL_NAME record.

[wmi]

llvm/test/ThinLTO/X86/distributed_indexes.ll

65

The test is to ensure we don't get COMBINED_ORIGINAL_NAME record. Why the test added BACKEND1-NEXT instead of BACKEND1-NEXT-NOT?

[tejohnson]

llvm/test/ThinLTO/X86/distributed_indexes.ll

65

"-NEXT-NOT" isn't a supported FileCheck suffix. But it doesn't matter, because the -NEXT are very specific and will fail if we get a COMBINED_ORIGINAL_NAME interspersed among them. I confirmed this test fails without the fix, as the COMBINED_ORIGINAL_NAME is in the middle of the "<COMBINED " sequence specified by BACKEND1-NEXT and causes those to fail.

[wmi]

llvm/test/ThinLTO/X86/distributed_indexes.ll

65

Oh, got it. There is a space after <COMBINED so <COMBINED {{.*}} won't match with COMBINED_ORIGINAL_NAME. Thanks.

[mehdi_amini]

The test seems to break on some bots: https://lab.llvm.org/buildbot/#/builders/61/builds/15150

[tejohnson]

In D110296#3019529, @mehdi_amini wrote:

The test seems to break on some bots: https://lab.llvm.org/buildbot/#/builders/61/builds/15150

Yep, sorry about that, should be fixed by 7da4ee2df088d39c7ca6531d80172af7d973bb67

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
 
   // Set that will be populated during call to writeFunctionTypeMetadataRecords
   // with the type ids referenced by this index file.
   std::set<GlobalValue::GUID> ReferencedTypeIds;
 
   // For local linkage, we also emit the original name separately
   // immediately after the record.
   auto MaybeEmitOriginalName = [&](GlobalValueSummary &S) {
-    if (!GlobalValue::isLocalLinkage(S.linkage()))
+    // We don't need to emit the original name if we are writing the index for
+    // distributed backends (in which case ModuleToSummariesForIndex is
+    // non-null). The original name is only needed during the thin link, since
+    // for SamplePGO the indirect call targets for local functions have
+    // have the original name annotated in profile.
+    // Continue to emit it when writing out the entire combined index, which is
+    // used in testing the thin link via llvm-lto.
+    if (ModuleToSummariesForIndex || !GlobalValue::isLocalLinkage(S.linkage()))
       return;
     NameVals.push_back(S.getOriginalName());
     Stream.EmitRecord(bitc::FS_COMBINED_ORIGINAL_NAME, NameVals);
     NameVals.clear();
   };
 
   std::set<GlobalValue::GUID> DefOrUseGUIDs;
   forEachSummary([&](GVInfo I, bool IsAliasee) {

llvm/test/ThinLTO/X86/distributed_indexes.ll

 ; BACKEND1-NEXT: <ENTRY {{.*}} record string = '{{.*}}distributed_indexes.ll.tmp{{.*}}.bc'
 ; BACKEND1-NEXT: </MODULE_STRTAB_BLOCK
 ; BACKEND1-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND1-NEXT: <VERSION
 ; BACKEND1-DAG: <VALUE_GUID op0={{.*}} op1=-5751648690987223394
 ; BACKEND1-DAG: <VALUE_GUID op0={{.*}} op1=-5300342847281564238
 ; BACKEND1-DAG: <VALUE_GUID op0={{.*}} op1=-3706093650706652785
 ; BACKEND1-DAG: <VALUE_GUID op0={{.*}} op1=-1039159065113703048
-; BACKEND1-DAG: <COMBINED {{.*}} op1=0
-; BACKEND1-DAG: <COMBINED {{.*}} op1=1
-; BACKEND1-DAG: <COMBINED_ALIAS {{.*}} op1=1
-; BACKEND1-NEXT: <BLOCK_COUNT op0=3/>
+; BACKEND1-DAG: <VALUE_GUID op0={{.*}} op1=-126160295109682753
+; BACKEND1-NEXT: <COMBINED {{.*}} op1=0
+; BACKEND1-NEXT: <COMBINED {{.*}} op1=0
+; BACKEND1-NEXT: <COMBINED {{.*}} op1=1
+; BACKEND1-NEXT: <COMBINED_ALIAS {{.*}} op1=1
+; BACKEND1-NEXT: <BLOCK_COUNT op0=4/>
 ; BACKEND1-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
 ; The backend index for Input/distributed_indexes.ll contains summaries from
 ; itself only, as it does not import anything.
 ; BACKEND2: <MODULE_STRTAB_BLOCK
 ; BACKEND2-NEXT: <ENTRY {{.*}} record string = '{{.*}}distributed_indexes.ll.tmp2.bc'
 ; BACKEND2-NEXT: </MODULE_STRTAB_BLOCK
 ; BACKEND2-NEXT: <GLOBALVAL_SUMMARY_BLOCK
 ; BACKEND2-NEXT: <VERSION
 ; BACKEND2-DAG: <VALUE_GUID op0={{.*}} op1=-5751648690987223394/>
 ; BACKEND2-DAG: <VALUE_GUID op0={{.*}} op1=-5300342847281564238/>
 ; BACKEND2-DAG: <VALUE_GUID op0={{.*}} op1=-1039159065113703048/>
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: <COMBINED
 ; BACKEND2-NEXT: <COMBINED_ALIAS
-; BACKEND2-NEXT: <BLOCK_COUNT op0=3/>
+; BACKEND2-NEXT: <BLOCK_COUNT op0=4/>
 ; BACKEND2-NEXT: </GLOBALVAL_SUMMARY_BLOCK
 
 ; Make sure that when the alias is imported as a copy of the aliasee, but the
 ; aliasee is not being imported by itself, that we can still print the summary.
 ; The aliasee should be "null".
 ; RUN: llvm-dis %t1.bc.thinlto.bc -o - | FileCheck %s --check-prefix=DIS
 ; DIS: aliasee: null
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 
 declare void @g(...)
 declare void @analias(...)
 
 define void @f() {
 entry:
   call void (...) @g()
   call void (...) @analias()
   ret void
 }
+
+; Ensure we don't get a COMBINED_ORIGINAL_NAME record in the distributed index.
+; The BACKEND1-NEXT checks would fail if we did.
+define internal void @x() {
+  ret void
+}