[PATCH] D108581: [CSPGO] Fix lost IRPGOFlag in CSPGO instrumentation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

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 <https://reviews.llvm.org/D108432> is still worth fixing. Breaking it could be hard, though.



================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:468
     createProfileFileNameVar(M, InstrProfileOutput);
-    createIRLevelProfileFlagVar(M, /* IsCS */ true, PGOInstrumentEntry);
+    appendToCompilerUsed(
+        M, createIRLevelProfileFlagVar(M, /* IsCS */ true, PGOInstrumentEntry));
----------------
xur wrote:
> MaskRay wrote:
> > 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.
> 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.
Agreed. There is prior art: we already use llvm.compiler.used to reference potentially non-prevailing `__profd_*`, 


================
Comment at: llvm/test/Transforms/PGOProfile/lto_cspgo_gen.ll:14
+; IRPGOPRE: @__llvm_profile_raw_version = constant i64
+; IRPGOBE: @__llvm_profile_raw_version = external constant i64
+
----------------
Perhaps add the `llvm.compiler.used` line and explain that the declaration needs to be kept even if non-prevailing.


================
Comment at: llvm/test/Transforms/PGOProfile/thinlto_cspgo_gen.ll:20
+; PREVAILING: @__llvm_profile_raw_version = constant i6
+; NOPREVAILING: @__llvm_profile_raw_version = external constant i64
 ; CSGEN: @__profc_
----------------
Perhaps add the `llvm.compiler.used` line and explain that the declaration needs to be kept even if non-prevailing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108581/new/

https://reviews.llvm.org/D108581