[CSPGO] Fix lost IRPGOFlag in CSPGO instrumentation

[xur]

Summary

The IRPGOFlag symbol (__llvm_profile_raw_version) is dropped when
identified as non-prevailing for either regular or thin LTO during
the mixed-LTO mode compilation. This happens in the module
where IRPGOFlag is marked as non-prevailing. This variable
is emitted in the final object from the prevailing module.

This is still problematic because we currently query this symbol to coordinate
some actions between PGOInstrumentation pass and InstrProfiling lowering
pass, like whether to do value profiling, weather to do comdat renaming.

TThis problem is bought up by YolandaCY in
https://reviews.llvm.org/D107034
YolandCY reported unresolved symbol linker errors in CSPGO instrumentation build.

D107034 proposed to set a module flag as the fix.

But I think a better way is to let LTO retain IRPGOFlag decl by adding
it to CompilerUsed list and relax the check in isIRPGOFlagSet() when
doing the InstruProfling lowering.

The test case in the patch is from D107034.

[MaskRay]

Thanks for the patch. llvm.compiler.used does look better to me.

---

This happens in the module where IRPGOFlag is mard as non-prevailing

marked

This is still problematic because we currently query this symbol to coordinate some actions B/W PGOInstrumentation and InstroProfiling lowering, like whether to do value profiling, weather to do comdat renaming.

Worth clarifying B/W.

InstruProfling

InstrProfiling

---

I have confirmed that

In regular LTO, the non-prevailing variable is dropped unless its linkage weak_odr/linkonce_odr && -compute-dead=0
In ThinLTO, the variable is changed to available_external linkage by thinLTOResolvePrevailingGUID then dropped by GlobleOpt:deleteIfDead in the backend.

Does it make sense to update thinlto_cspgo_gen.ll as well?

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

469

The form liked by both clang-tidy bugprone-argument-comment and clang-format: /*IsCS=*/true

1640

The form liked by both clang-tidy bugprone-argument-comment and clang-format: /*IsCS=*/true

llvm/test/Transforms/PGOProfile/lto_cspgo_gen.ll

9

Worth a comment that the two variables are intentionally non-prevailing.

27

unneeded and can be dropped

32

unneeded and can be dropped

[MaskRay]

llvm/lib/ProfileData/InstrProf.cpp

1104

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

468–471

Add a comment like:

The variable in a comdat may be discarded by LTO. Ensure the declaration will be retained.

This raises the question: Is llvm.compiler.used allowed to reference a declaration (previously a non-prevailing definition due to comdat selection)? ISTM this should be allowed.

[xur]

In D108581#2961321, @MaskRay wrote:

Thanks for the patch. llvm.compiler.used does look better to me.

---

This happens in the module where IRPGOFlag is mard as non-prevailing

marked

Fixed

This is still problematic because we currently query this symbol to coordinate some actions B/W PGOInstrumentation and InstroProfiling lowering, like whether to do value profiling, weather to do comdat renaming.

Worth clarifying B/W.

InstruProfling

InstrProfiling

Fixed

---

I have confirmed that

In regular LTO, the non-prevailing variable is dropped unless its linkage weak_odr/linkonce_odr && -compute-dead=0
In ThinLTO, the variable is changed to available_external linkage by thinLTOResolvePrevailingGUID then dropped by GlobleOpt:deleteIfDead in the backend.

Thanks for confirming and put the conditions here

Does it make sense to update thinlto_cspgo_gen.ll as well?

That's a good idea. I will change that test too.

[xur]

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

468–471

comment added.

For the question:
At the time of appending to llvm.compiler.used, this is a definition (not a decl). I don't see any problem add it to the list.

[xur]

Thanks to Fangrui for the comments/suggestions.
Integrated his reviews to the patch.

-Rong

[MaskRay]

LGTM.

Apologies to @YolandaCY that I did make a mistake in the other patch.
("ld.lld does comdat selection but the prevailing/non-prevailing decisions do not drop non-prevailing __llvm_profile_raw_version variables from bitcode files.") is wrong.
ThinLTO and regular LTO can indeed drop the non-prevailing variables.

---

I assume that D108432 is still worth fixing. Breaking it could be hard, though.

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

468–471

Agreed. There is prior art: we already use llvm.compiler.used to reference potentially non-prevailing __profd_*,

llvm/test/Transforms/PGOProfile/lto_cspgo_gen.ll

15

Perhaps add the llvm.compiler.used line and explain that the declaration needs to be kept even if non-prevailing.

llvm/test/Transforms/PGOProfile/thinlto_cspgo_gen.ll

20

Perhaps add the llvm.compiler.used line and explain that the declaration needs to be kept even if non-prevailing.

[xur]

Added a few comments to the test cases as suggested by Fangri.

[YolandaCY]

Thank you for the patch. I've verified the IRPGOFlag is kept now for non-prevailing modules in Chromium build. The linker error is also resolved for Windows build.

Glad to learn the usage of llvm.compiler.used, indeed a more proper fix. :)

llvm/include/llvm/ProfileData/InstrProf.h

 } // end namespace RawInstrProf
 
 // Parse MemOP Size range option.
 void getMemOPSizeRangeFromOption(StringRef Str, int64_t &RangeStart,
                                  int64_t &RangeLast);
 
 // Create a COMDAT variable INSTR_PROF_RAW_VERSION_VAR to make the runtime
 // aware this is an ir_level profile so it can set the version flag.
-void createIRLevelProfileFlagVar(Module &M, bool IsCS,
+GlobalVariable *createIRLevelProfileFlagVar(Module &M, bool IsCS,
                                             bool InstrEntryBBEnabled);
 
 // Create the variable for the profile file name.
 void createProfileFileNameVar(Module &M, StringRef InstrProfileOutput);
 
 // Whether to compress function names in profile records, and filenames in
 // code coverage mappings. Used by the Instrumentation library and unit tests.
 extern cl::opt<bool> DoInstrProfNameCompression;
 
 } // end namespace llvm
 #endif // LLVM_PROFILEDATA_INSTRPROF_H

llvm/lib/ProfileData/InstrProf.cpp

bool needsComdatForCounter(const Function &F, const Module &M) {
 
   return true;
 }
 
 // Check if INSTR_PROF_RAW_VERSION_VAR is defined.
 bool isIRPGOFlagSet(const Module *M) {
   auto IRInstrVar =
       M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
-  if (!IRInstrVar || IRInstrVar->isDeclaration() ||
-      IRInstrVar->hasLocalLinkage())
+  if (!IRInstrVar || IRInstrVar->hasLocalLinkage())
     return false;
 
+  // For CSPGO+LTO, this variable might be marked as non-prevailing and we only
+  // have the decl.
+  if (IRInstrVar->isDeclaration())
+    return true;
+
   // Check if the flag is set.
   if (!IRInstrVar->hasInitializer())
     return false;
 
   auto *InitVal = dyn_cast_or_null<ConstantInt>(IRInstrVar->getInitializer());
   if (!InitVal)
     return false;
   return (InitVal->getZExtValue() & VARIANT_MASK_IR_PROF) != 0;
if (!F.hasComdat()) {
     assert(F.getLinkage() == GlobalValue::AvailableExternallyLinkage);
     return true;
   }
   return true;
 }
 
 // Create a COMDAT variable INSTR_PROF_RAW_VERSION_VAR to make the runtime
 // aware this is an ir_level profile so it can set the version flag.
-void createIRLevelProfileFlagVar(Module &M, bool IsCS,
+GlobalVariable *createIRLevelProfileFlagVar(Module &M, bool IsCS,
                                             bool InstrEntryBBEnabled) {
   const StringRef VarName(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
   Type *IntTy64 = Type::getInt64Ty(M.getContext());
   uint64_t ProfileVersion = (INSTR_PROF_RAW_VERSION | VARIANT_MASK_IR_PROF);
   if (IsCS)
     ProfileVersion |= VARIANT_MASK_CSIR_PROF;
   if (InstrEntryBBEnabled)
     ProfileVersion |= VARIANT_MASK_INSTR_ENTRY;
   auto IRLevelVersionVariable = new GlobalVariable(
       M, IntTy64, true, GlobalValue::WeakAnyLinkage,
       Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName);
   IRLevelVersionVariable->setVisibility(GlobalValue::DefaultVisibility);
   Triple TT(M.getTargetTriple());
   if (TT.supportsCOMDAT()) {
     IRLevelVersionVariable->setLinkage(GlobalValue::ExternalLinkage);
     IRLevelVersionVariable->setComdat(M.getOrInsertComdat(VarName));
   }
+  return IRLevelVersionVariable;
 }
 
 // Create the variable for the profile file name.
 void createProfileFileNameVar(Module &M, StringRef InstrProfileOutput) {
   if (InstrProfileOutput.empty())
     return;
   Constant *ProfileNameConst =
       ConstantDataArray::getString(M.getContext(), InstrProfileOutput, true);

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

 #include "llvm/Support/DOTGraphTraits.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/GraphWriter.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <memory>
 #include <numeric>
 #include <string>
 #include <unordered_map>
 #include <utility>
PGOInstrumentationGenCreateVarLegacyPass(std::string CSInstrName = "")
       : ModulePass(ID), InstrProfileOutput(CSInstrName) {
     initializePGOInstrumentationGenCreateVarLegacyPassPass(
         *PassRegistry::getPassRegistry());
   }
 
 private:
   bool runOnModule(Module &M) override {
     createProfileFileNameVar(M, InstrProfileOutput);
-    createIRLevelProfileFlagVar(M, /* IsCS */ true, PGOInstrumentEntry);
+    // The variable in a comdat may be discarded by LTO. Ensure the
+    // declaration will be retained.
+    appendToCompilerUsed(
+        M, createIRLevelProfileFlagVar(M, /*IsCS=*/true, PGOInstrumentEntry));
     return false;
   }
   std::string InstrProfileOutput;
 };
 
 } // end anonymous namespace
 
 char PGOInstrumentationGenLegacyPass::ID = 0;

 
 static bool InstrumentAllFunctions(
     Module &M, function_ref<TargetLibraryInfo &(Function &)> LookupTLI,
     function_ref<BranchProbabilityInfo *(Function &)> LookupBPI,
     function_ref<BlockFrequencyInfo *(Function &)> LookupBFI, bool IsCS) {
   // For the context-sensitve instrumentation, we should have a separated pass
   // (before LTO/ThinLTO linking) to create these variables.
   if (!IsCS)
-    createIRLevelProfileFlagVar(M, /* IsCS */ false, PGOInstrumentEntry);
+    createIRLevelProfileFlagVar(M, /*IsCS=*/false, PGOInstrumentEntry);
   std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
   collectComdatMembers(M, ComdatMembers);
 
   for (auto &F : M) {
     if (F.isDeclaration())
       continue;
     if (F.hasFnAttribute(llvm::Attribute::NoProfile))
       continue;
     auto &TLI = LookupTLI(F);
     auto *BPI = LookupBPI(F);
     auto *BFI = LookupBFI(F);
     instrumentOneFunc(F, &M, TLI, BPI, BFI, ComdatMembers, IsCS);
   }
   return true;
 }
 
 PreservedAnalyses
 PGOInstrumentationGenCreateVar::run(Module &M, ModuleAnalysisManager &AM) {
   createProfileFileNameVar(M, CSInstrName);
-  createIRLevelProfileFlagVar(M, /* IsCS */ true, PGOInstrumentEntry);
+  // The variable in a comdat may be discarded by LTO. Ensure the declaration
+  // will be retained.
+  appendToCompilerUsed(
+      M, createIRLevelProfileFlagVar(M, /*IsCS=*/true, PGOInstrumentEntry));
   return PreservedAnalyses::all();
 }
 
 bool PGOInstrumentationGenLegacyPass::runOnModule(Module &M) {
   if (skipModule(M))
     return false;
 
   auto LookupTLI = [this](Function &F) -> TargetLibraryInfo & {

llvm/test/Transforms/PGOProfile/Inputs/thinlto_cspgo_bar_gen.ll

 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"
 
-$__llvm_profile_filename = comdat any
-$__llvm_profile_raw_version = comdat any
-
 @odd = common dso_local global i32 0, align 4
 @even = common dso_local global i32 0, align 4
-@__llvm_profile_filename = constant [25 x i8] c"pass2/default_%m.profraw\00", comdat
-@__llvm_profile_raw_version = constant i64 216172782113783812, comdat
 
 define dso_local void @bar(i32 %n) !prof !29 {
 entry:
   %call = tail call fastcc i32 @cond(i32 %n)
   %tobool = icmp eq i32 %call, 0
   br i1 %tobool, label %if.else, label %if.then, !prof !30
 
 if.then:

llvm/test/Transforms/PGOProfile/lto_cspgo_gen.ll

+; REQUIRES: x86-registered-target
+
+; RUN: opt -passes='thinlto-pre-link<O2>' --cs-profilegen-file=alloc -cspgo-kind=cspgo-instr-gen-pipeline -module-summary %s -o %t.bc
+; RUN: llvm-dis %t.bc -o - | FileCheck %s --check-prefix=IRPGOPRE
+
+;; Symbol __llvm_profile_filename and __llvm_profile_raw_version are non-prevailing here.
+; RUN: llvm-lto2 run -lto-cspgo-profile-file=alloc -lto-cspgo-gen -save-temps -o %t %t.bc \
+; RUN:   -r=%t.bc,f,px \
+; RUN:   -r=%t.bc,__llvm_profile_filename,x \
+; RUN:   -r=%t.bc,__llvm_profile_raw_version,x
+; RUN: llvm-dis %t.0.0.preopt.bc -o - | FileCheck %s --check-prefix=IRPGOBE
+
+;; Before LTO, we should have the __llvm_profile_raw_version definition.
+; IRPGOPRE: @__llvm_profile_raw_version = constant i64
+
+;; Non-prevailing __llvm_profile_raw_version is discarded by LTO. Ensure the
+;; declaration is retained.
+; IRPGOBE: @__llvm_profile_raw_version = external constant i64
+
+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"
+
+$f = comdat any
+
+; Function Attrs: nofree norecurse nosync nounwind readnone uwtable willreturn mustprogress
+define i32 @f() {
+entry:
+  ret i32 1
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"ThinLTO", i32 0}

llvm/test/Transforms/PGOProfile/thinlto_cspgo_gen.ll

 ; REQUIRES: x86-registered-target
 
-; RUN: opt -module-summary %s -o %t1.bc
-; RUN: opt -module-summary %S/Inputs/thinlto_cspgo_bar_gen.ll -o %t2.bc
+; RUN: opt -passes='thinlto-pre-link<O2>' --cs-profilegen-file=alloc -cspgo-kind=cspgo-instr-gen-pipeline -module-summary %s -o %t1.bc
+; RUN: opt -passes='thinlto-pre-link<O2>' --cs-profilegen-file=alloc -cspgo-kind=cspgo-instr-gen-pipeline -module-summary %S/Inputs/thinlto_cspgo_bar_gen.ll -o %t2.bc
 ; RUN: llvm-lto2 run -lto-cspgo-profile-file=alloc -lto-cspgo-gen -save-temps -o %t %t1.bc %t2.bc \
 ; RUN:   -r=%t1.bc,foo,pl \
 ; RUN:   -r=%t1.bc,bar,l \
 ; RUN:   -r=%t1.bc,main,plx \
 ; RUN:   -r=%t1.bc,__llvm_profile_filename,plx \
 ; RUN:   -r=%t1.bc,__llvm_profile_raw_version,plx \
 ; RUN:   -r=%t2.bc,bar,pl \
 ; RUN:   -r=%t2.bc,odd,pl \
 ; RUN:   -r=%t2.bc,even,pl \
 ; RUN:   -r=%t2.bc,__llvm_profile_filename,x \
 ; RUN:   -r=%t2.bc,__llvm_profile_raw_version,x
-; RUN: llvm-dis %t.1.4.opt.bc -o - | FileCheck %s --check-prefix=CSGEN
+; RUN: llvm-dis %t.1.4.opt.bc -o - | FileCheck %s --check-prefixes=CSGEN,PREVAILING
+; RUN: llvm-dis %t.2.4.opt.bc -o - | FileCheck %s --check-prefixes=CSGEN,NOPREVAILING
 
+;; Prevailing __llvm_profile_raw_version is kept by LTO.
+; PREVAILING: @__llvm_profile_raw_version = constant i64
+
+;; Non-prevailing __llvm_profile_raw_version is discarded by LTO. Ensure the
+;; declaration is retained.
+; NOPREVAILING: @__llvm_profile_raw_version = external constant i64
 ; CSGEN: @__profc_
 ; CSGEN: @__profd_
 
 source_filename = "cspgo.c"
 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"
 
-$__llvm_profile_filename = comdat any
-$__llvm_profile_raw_version = comdat any
-@__llvm_profile_filename = constant [25 x i8] c"pass2/default_%m.profraw\00", comdat
-@__llvm_profile_raw_version = constant i64 216172782113783812, comdat
-
 define dso_local void @foo() #0 !prof !29 {
 entry:
   br label %for.body
 
 for.body:
   %i.06 = phi i32 [ 0, %entry ], [ %add1, %for.body ]
   tail call void @bar(i32 %i.06)
   %add = or i32 %i.06, 1