[PATCH] D19527: LTOCodeGenerator: add linkonce to llvm.used when in "MustPreserve" set
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 26 10:44:35 PDT 2016
> On Apr 26, 2016, at 9:55 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>
>> On 2016-Apr-26, at 09:35, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>
>> joker.eph marked 2 inline comments as done.
>> joker.eph added a comment.
>>
>> Thanks for the comments.
>>
>>
>> ================
>> Comment at: lib/LTO/LTOCodeGenerator.cpp:394
>> @@ +393,3 @@
>> + if (ScopeRestrictionsDone)
>> + return;
>> +
>> ----------------
>> Yes, good catch. It turns out that the next variable declaration was unused, and was taken as body for the if block, which is why the code was still compiling...
>>
>>
>>
>> http://reviews.llvm.org/D19527
>
> Any idea why this is only coming up now? It doesn't seem like a new bug...
>
> I think llvm.compiler.used is more appropriate than llvm.used.
You're right: I should have read LangRef again before making this call, my memories was not good enough apparently.
> I'm also not sure adding to the llvm.compiler.used (or llvm.used) is the
> right way to prevent linkonce/linkonce_odr functions from being
> discarded. Should you just change their linkage to weak/weak_odr?
I hesitated, and could find a good rational for one or the other, I'll be happy to switch to weak/weak_odr if you think it is more appropriated.
This will invalidate many of your comments below, but I'll address them anyway:
>
> I.e., this seems like the moral equivalent of the following code in
> gold-plugin.cpp:
> --
> case LDPR_PREVAILING_DEF:
> Keep.push_back(GV);
> // There is a non IR use, so we have to force optimizations to keep this.
> switch (GV->getLinkage()) {
> default:
> break;
> case GlobalValue::LinkOnceAnyLinkage:
> GV->setLinkage(GlobalValue::WeakAnyLinkage);
> break;
> case GlobalValue::LinkOnceODRLinkage:
> GV->setLinkage(GlobalValue::WeakODRLinkage);
> break;
> }
> break;
> --
>
> Should we take the same approach?
>
>> Index: test/tools/lto/hide-linkonce-odr.ll
>> ===================================================================
>> --- test/tools/lto/hide-linkonce-odr.ll
>> +++ test/tools/lto/hide-linkonce-odr.ll
>> @@ -1,9 +1,11 @@
>> ; RUN: llvm-as %s -o %t.o
>> -; RUN: %ld64 -lto_library %llvmshlibdir/libLTO.dylib -dylib -arch x86_64 -macosx_version_min 10.10.0 -lSystem -o %t.dylib %t.o -save-temps -undefined dynamic_lookup
>> +; RUN: %ld64 -lto_library %llvmshlibdir/libLTO.dylib -dylib -arch x86_64 -macosx_version_min 10.10.0 -lSystem -o %t.dylib %t.o -save-temps -undefined dynamic_lookup -exported_symbol _c -exported_symbol _b
>>
>> ; RUN: llvm-dis %t.dylib.lto.opt.bc -o - | FileCheck --check-prefix=IR %s
>> -; check that @a is still a linkonce_odr definition
>> -; IR: define linkonce_odr void @a()
>> +; check that @a is no longer a linkonce_odr definition
>> +; IR-NOT: define linkonce_odr void @a()
>> +; check that @b is appended in llvm.used
>> +; IR: @llvm.used = appending global [1 x i8*] [i8* bitcast (void ()* @b to i8*)], section "llvm.metadata"
>>
>> ; RUN: llvm-nm %t.dylib | FileCheck --check-prefix=NM %s
>> ; check that the linker can hide @a but not @b
>> Index: lib/LTO/LTOCodeGenerator.cpp
>> ===================================================================
>> --- lib/LTO/LTOCodeGenerator.cpp
>> +++ lib/LTO/LTOCodeGenerator.cpp
>> @@ -348,14 +348,74 @@
>> RelocModel, CodeModel::Default, CGOptLevel));
>> }
>>
>> +// If a linkonce global is present in the MustPreserveSymbols, we need to make
>> +// sure we honor this. To force the compiler to not drop it, we add it to the
>> +// "llvm.used" global.
>> +static void preserveDiscardableGV(
>
> This this is preserving all the discardable GVs that should be preserved
> (not just one), I think this should be called "preserveDiscardableGVs".
>
>> + Module &TheModule,
>> + const std::function<bool(const GlobalValue &)> &MustPreserveGV) {
>
> I think you can use llvm::function_ref here since the function lifetime
> isn't an issue.
Great, I didn't know about this helper!
>
> Also, I usually name function objects the same way as other functions
> (starting with a lower-case letter). I don't think our style guidelines
> are clear one way or the other, but I thought I'd mention my tendency.
Oh interesting.
>
>> + DenseSet<Constant *> UsedValuesSet;
>
> You should use a SetVector here. Otherwise the output value of the
> variable will depend on how pointers decide to hash on that particular
> run.
Good point.
>
>> + if (GlobalVariable *LLVMUsed = TheModule.getGlobalVariable("llvm.used")) {
>> + ConstantArray *Inits = cast<ConstantArray>(LLVMUsed->getInitializer());
>> + for (auto &V : Inits->operands())
>> + UsedValuesSet.insert(cast<Constant>(&V));
>> + LLVMUsed->eraseFromParent();
>
> This doesn't seem appropriate: you're *always* replacing the variable,
> even if you're not going to change its contents.
Yes, I should do better. Note that is imitating what already exists in UpdateCompilerUsed() (i.e. we should fix it there as well).
>
>> + }
>> + llvm::Type *i8PTy = llvm::Type::getInt8PtrTy(TheModule.getContext());
>> + auto mayPreserveGlobal = [&](GlobalValue &GV) {
>
> "shouldPreserveGlobal"?
`should` would makes me think that this is a predicate (i.e. that could be used as `if (shouldXYZ()) ...` , while this helper directly adds to the set (under condition).
>
>> + if (!GV.isDiscardableIfUnused() || GV.isDeclaration())
>> + return;
>> + if (!MustPreserveGV(GV))
>> + return;
>> + assert(!GV.hasAvailableExternallyLinkage() && !GV.hasInternalLinkage());
>
> Couldn't it be that (in -flto=thin):
>
> - The function is linkonce.
> - It gets imported to another translation unit as
> available_externally.
>
> Or is that somehow excluded by logic elsewhere?
We're in the LTOCodegenerator, not sure how/why ThinLTO interacts here?
>
>> + UsedValuesSet.insert(ConstantExpr::getBitCast(&GV, i8PTy));
>> + };
>> + for (auto &GV : TheModule)
>> + mayPreserveGlobal(GV);
>> + for (auto &GV : TheModule.globals())
>> + mayPreserveGlobal(GV);
>> + for (auto &GV : TheModule.aliases())
>> + mayPreserveGlobal(GV);
>> +
>> + if (UsedValuesSet.empty())
>> + return;
>> + std::vector<Constant *> UsedValuesList(UsedValuesSet.begin(),
>> + UsedValuesSet.end());
>> +
>> + llvm::ArrayType *ATy = llvm::ArrayType::get(i8PTy, UsedValuesList.size());
>> + auto *LLVMUsed = new llvm::GlobalVariable(
>> + TheModule, ATy, false, llvm::GlobalValue::AppendingLinkage,
>> + llvm::ConstantArray::get(ATy, UsedValuesList), "llvm.used");
>> + LLVMUsed->setSection("llvm.metadata");
>> +}
>> +
>> void LTOCodeGenerator::applyScopeRestrictions() {
>> - if (ScopeRestrictionsDone || !ShouldInternalize)
>> + if (ScopeRestrictionsDone)
>> + return;
>> +
>> + // Declare a callback for the internalize pass that will ask for every
>> + // candidate GlobalValue if it can be internalized or not.
>> + SmallString<64> MangledName;
>> + auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
>> + // Need to mangle the GV as the "MustPreserveSymbols" StringSet is filled
>> + // with the linker supplied name, which on Darwin includes a leading
>> + // underscore.
>> + MangledName.clear();
>> + MangledName.reserve(GV.getName().size() + 1);
>> + Mangler::getNameWithPrefix(MangledName, GV.getName(),
>> + MergedModule->getDataLayout());
>> + return MustPreserveSymbols.count(MangledName);
>> + };
>> +
>> + // Preserve linkonce value on linker request
>> + preserveDiscardableGV(*MergedModule, MustPreserveGV);
>> +
>> + if (!ShouldInternalize)
>> return;
>>
>> if (ShouldRestoreGlobalsLinkage) {
>> // Record the linkage type of non-local symbols so they can be restored
>> - // prior
>> - // to module splitting.
>> + // prior to module splitting.
>> auto RecordLinkage = [&](const GlobalValue &GV) {
>> if (!GV.hasAvailableExternallyLinkage() && !GV.hasLocalLinkage() &&
>> GV.hasName())
>> @@ -373,21 +433,6 @@
>> // symbols referenced from asm
>> UpdateCompilerUsed(*MergedModule, *TargetMach, AsmUndefinedRefs);
>
> ^ If we stick with the "used" way of doing it, should this get merged
> with this code somehow?
I thought about it, but didn't like it (because I don't want it for ThinLTO or other users), but now I see how it could be merged: with an optional callback for the client.
--
Mehdi
>
>>
>> - // Declare a callback for the internalize pass that will ask for every
>> - // candidate GlobalValue if it can be internalized or not.
>> - Mangler Mangler;
>> - SmallString<64> MangledName;
>> - auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
>> - // Need to mangle the GV as the "MustPreserveSymbols" StringSet is filled
>> - // with the linker supplied name, which on Darwin includes a leading
>> - // underscore.
>> - MangledName.clear();
>> - MangledName.reserve(GV.getName().size() + 1);
>> - Mangler::getNameWithPrefix(MangledName, GV.getName(),
>> - MergedModule->getDataLayout());
>> - return MustPreserveSymbols.count(MangledName);
>> - };
>> -
>> internalizeModule(*MergedModule, MustPreserveGV);
>>
>> ScopeRestrictionsDone = true;
>>
>
More information about the llvm-commits
mailing list