[PATCH] D103988: SampleFDO] place the discriminator flag variable into the used list.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp:171
+  appendToUsed(*M, {new GlobalVariable(*M, Type::getInt1Ty(Context), true,
+                                       GlobalValue::WeakODRLinkage,
+                                       ConstantInt::getTrue(Context),
----------------
hoy wrote:
> xur wrote:
> > hoy wrote:
> > > There's a `CommonLinkage` type and from its description, it looks like command variables should not be GC-ed but they can only have a zero value. Not sure if `-Wl,--export-dynamic-symbol` is needed with the common linkage type.
> > > 
> > > common
> > > “common” linkage is most similar to “weak” linkage, but they are used for tentative definitions in C, such as “int X;” at global scope. Symbols with “common” linkage are merged in the same way as weak symbols, and they may not be deleted if unreferenced. common symbols may not have an explicit section, must have a zero initializer, and may not be marked ‘constant’. Functions and aliases may not have common linkage.
> > Hongtao, thanks for pointing this out. I just tried and seemed "common" linkage worked -- I don't need to put the variable into used list and we don't need --export-dynamic-symbol to suppress the  GC. 
> > 
> > Using common linkage is a better solution here. I'll update the patch shortly.
> LGTM, thanks for trying this out.
Looks like similar things like `createIRLevelProfileFlagVar` and `createProfileFileNameVar` are using ExternalLinkage which would also suppress GC. (there's a check against supportsCOMDAT too). Is there a convention for these? 


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

https://reviews.llvm.org/D103988