[PGO] Change the ProfileSummary metadata merge behavior

[jakev]

Summary

The behavior when merging different ProfileSummary metadata really shouldn't be to throw an Error. This prevents users from doing LTO builds including IR files with older/different profile data applied.

Even if people think changing the behavior to Warning isn't the right approach, it would be good to move away from intentional failure.

[eraman]

All the modules are expected to have the same profile summary and changing the behavior to a warning is not the right thing to do. If we want to support a use case where we LTO files with different profile data, perhaps we should have a way to disable profile summary for those builds? That seems better than picking the first summary we see (as would be the case with a warning)

[jakev]

In D22030#475831, @eraman wrote:

All the modules are expected to have the same profile summary and changing the behavior to a warning is not the right thing to do. If we want to support a use case where we LTO files with different profile data, perhaps we should have a way to disable profile summary for those builds? That seems better than picking the first summary we see (as would be the case with a warning)

Yeah that seems reasonable. I'm not sure what the right way to do this is, but dropping the ProfileSummary metadata and giving up on any further merging when we encounter a conflict seems like the right idea. What do you think is the correct path forward here?

[eraman]

I am trying to understand the use case here. Do you need to support a case where you compile different modules with different profiles and LTO them together? Isn't merging the profiles and using the merged profile an option there?

[jakev]

In D22030#478216, @eraman wrote:

I am trying to understand the use case here. Do you need to support a case where you compile different modules with different profiles and LTO them together? Isn't merging the profiles and using the merged profile an option there?

Apologies for the delay.

After some discussion about this with silvas offline, it seems that leaving the current behavior might be the easiest way to loudly inform users of the profile metadata mismatch. My initial concern was that, as a matter of principle, builds shouldn't fail due to stale profile information. LTO or otherwise. Since this is apparently a non-issue, I'll abandon the patch. Sorry for kicking up dust.

lib/IR/Module.cpp

return static_cast<PIELevel::Level>(
       cast<ConstantInt>(Val->getValue())->getZExtValue());
 }
 
 void Module::setPIELevel(PIELevel::Level PL) {
   addModuleFlag(ModFlagBehavior::Error, "PIE Level", PL);
 }
 
 void Module::setProfileSummary(Metadata *M) {
-  addModuleFlag(ModFlagBehavior::Error, "ProfileSummary", M);
+  addModuleFlag(ModFlagBehavior::Warning, "ProfileSummary", M);
 }
 
 Metadata *Module::getProfileSummary() {
   return getModuleFlag("ProfileSummary");
 }
 
 GlobalVariable *llvm::collectUsedGlobalVariables(
     const Module &M, SmallPtrSetImpl<GlobalValue *> &Set, bool CompilerUsed) {

test/Transforms/PGOProfile/PR28219.ll

 ; Test that we annotate entire program's summary and not just this module's
 ; RUN: llvm-profdata merge %S/Inputs/PR28219.proftext -o %t.profdata
 ; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata -S | FileCheck %s
 
 define i32 @bar() {
 entry:
   ret i32 1
 }
-; CHECK-DAG: {{![0-9]+}} = !{i32 1, !"ProfileSummary", {{![0-9]+}}}
+; CHECK-DAG: {{![0-9]+}} = !{i32 2, !"ProfileSummary", {{![0-9]+}}}
 ; CHECK-DAG: {{![0-9]+}} = !{!"NumFunctions", i64 2}
 ; CHECK-DAG: {{![0-9]+}} = !{!"MaxFunctionCount", i64 3}
 

test/Transforms/PGOProfile/branch1.ll

; GEN: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @__profn_test_br_1, i32 0, i32 0), i64 25571299074, i32 2, i32 1)
   br label %if.end
 
 if.end:
 ; GEN: if.end:
 ; GEN: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([9 x i8], [9 x i8]* @__profn_test_br_1, i32 0, i32 0), i64 25571299074, i32 2, i32 0)
   %retv = phi i32 [ %add, %if.then ], [ %i, %entry ]
   ret i32 %retv
 }
-; USE-DAG: {{![0-9]+}} = !{i32 1, !"ProfileSummary", {{![0-9]+}}}
+; USE-DAG: {{![0-9]+}} = !{i32 2, !"ProfileSummary", {{![0-9]+}}}
 ; USE-DAG: {{![0-9]+}} = !{!"DetailedSummary", {{![0-9]+}}}
 ; USE-DAG: ![[FUNC_ENTRY_COUNT]] = !{!"function_entry_count", i64 3}

test/Transforms/SampleProfile/summary.ll

 ; Test that we annotate entire program's summary to IR.
 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/summary.prof -S | FileCheck %s
 
 define i32 @bar() #0 !dbg !1 {
 entry:
   ret i32 1, !dbg !2
 }
 
-; CHECK-DAG: {{![0-9]+}} = !{i32 1, !"ProfileSummary", {{![0-9]+}}}
+; CHECK-DAG: {{![0-9]+}} = !{i32 2, !"ProfileSummary", {{![0-9]+}}}
 ; CHECK-DAG: {{![0-9]+}} = !{!"NumFunctions", i64 2}
 ; CHECK-DAG: {{![0-9]+}} = !{!"MaxFunctionCount", i64 3}
 
 !1 = distinct !DISubprogram(name: "bar")
 !2 = !DILocation(line: 2, scope: !2)