Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
llvm/lib/Linker/IRMover.cpp
Show All 13 Lines | |||||
#include "llvm/ADT/Triple.h" | #include "llvm/ADT/Triple.h" | ||||
#include "llvm/IR/AutoUpgrade.h" | #include "llvm/IR/AutoUpgrade.h" | ||||
#include "llvm/IR/Constants.h" | #include "llvm/IR/Constants.h" | ||||
#include "llvm/IR/DebugInfoMetadata.h" | #include "llvm/IR/DebugInfoMetadata.h" | ||||
#include "llvm/IR/DiagnosticPrinter.h" | #include "llvm/IR/DiagnosticPrinter.h" | ||||
#include "llvm/IR/Function.h" | #include "llvm/IR/Function.h" | ||||
#include "llvm/IR/GVMaterializer.h" | #include "llvm/IR/GVMaterializer.h" | ||||
#include "llvm/IR/GlobalValue.h" | #include "llvm/IR/GlobalValue.h" | ||||
#include "llvm/IR/Instruction.h" | |||||
#include "llvm/IR/Instructions.h" | |||||
#include "llvm/IR/Intrinsics.h" | #include "llvm/IR/Intrinsics.h" | ||||
#include "llvm/IR/Module.h" | #include "llvm/IR/Module.h" | ||||
#include "llvm/IR/PseudoProbe.h" | #include "llvm/IR/PseudoProbe.h" | ||||
#include "llvm/IR/TypeFinder.h" | #include "llvm/IR/TypeFinder.h" | ||||
#include "llvm/Object/ModuleSymbolTable.h" | #include "llvm/Object/ModuleSymbolTable.h" | ||||
#include "llvm/Support/Error.h" | #include "llvm/Support/Error.h" | ||||
#include "llvm/Support/Path.h" | #include "llvm/Support/Path.h" | ||||
#include "llvm/Transforms/Utils/ValueMapper.h" | #include "llvm/Transforms/Utils/ValueMapper.h" | ||||
▲ Show 20 Lines • Show All 490 Lines • ▼ Show 20 Lines | class IRLinker { | ||||
/// Hence, the need to move this out of the materialization call chain. | /// Hence, the need to move this out of the materialization call chain. | ||||
void flushRAUWWorklist(); | void flushRAUWWorklist(); | ||||
/// When importing for ThinLTO, prevent importing of types listed on | /// When importing for ThinLTO, prevent importing of types listed on | ||||
/// the DICompileUnit that we don't need a copy of in the importing | /// the DICompileUnit that we don't need a copy of in the importing | ||||
/// module. | /// module. | ||||
void prepareCompileUnitsForImport(); | void prepareCompileUnitsForImport(); | ||||
void linkNamedMDNodes(); | void linkNamedMDNodes(); | ||||
tejohnson: Probably make the comment more generic, since I envision that this function will be expanded to… | |||||
/// Update attributes while linking. | |||||
void updateAttributes(GlobalValue &GV); | |||||
public: | public: | ||||
IRLinker(Module &DstM, MDMapT &SharedMDs, | IRLinker(Module &DstM, MDMapT &SharedMDs, | ||||
IRMover::IdentifiedStructTypeSet &Set, std::unique_ptr<Module> SrcM, | IRMover::IdentifiedStructTypeSet &Set, std::unique_ptr<Module> SrcM, | ||||
ArrayRef<GlobalValue *> ValuesToLink, | ArrayRef<GlobalValue *> ValuesToLink, | ||||
IRMover::LazyCallback AddLazyFor, bool IsPerformingImport) | IRMover::LazyCallback AddLazyFor, bool IsPerformingImport) | ||||
: DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)), | : DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)), | ||||
TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this), | TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this), | ||||
SharedMDs(SharedMDs), IsPerformingImport(IsPerformingImport), | SharedMDs(SharedMDs), IsPerformingImport(IsPerformingImport), | ||||
▲ Show 20 Lines • Show All 94 Lines • ▼ Show 20 Lines | Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) { | ||||
// new definition for the indirect symbol ("New" will be different). | // new definition for the indirect symbol ("New" will be different). | ||||
if ((ForIndirectSymbol && ValueMap.lookup(SGV) == New) || | if ((ForIndirectSymbol && ValueMap.lookup(SGV) == New) || | ||||
(!ForIndirectSymbol && IndirectSymbolValueMap.lookup(SGV) == New)) | (!ForIndirectSymbol && IndirectSymbolValueMap.lookup(SGV) == New)) | ||||
return New; | return New; | ||||
if (ForIndirectSymbol || shouldLink(New, *SGV)) | if (ForIndirectSymbol || shouldLink(New, *SGV)) | ||||
setError(linkGlobalValueBody(*New, *SGV)); | setError(linkGlobalValueBody(*New, *SGV)); | ||||
updateAttributes(*New); | |||||
return New; | return New; | ||||
} | } | ||||
/// Loop through the global variables in the src module and merge them into the | /// Loop through the global variables in the src module and merge them into the | ||||
/// dest module. | /// dest module. | ||||
GlobalVariable *IRLinker::copyGlobalVariableProto(const GlobalVariable *SGVar) { | GlobalVariable *IRLinker::copyGlobalVariableProto(const GlobalVariable *SGVar) { | ||||
// No linking to be performed or linking from the source: simply create an | // No linking to be performed or linking from the source: simply create an | ||||
// identical version of the symbol over in the dest module... the | // identical version of the symbol over in the dest module... the | ||||
▲ Show 20 Lines • Show All 875 Lines • ▼ Show 20 Lines | static std::string adjustInlineAsm(const std::string &InlineAsm, | ||||
const Triple &Triple) { | const Triple &Triple) { | ||||
if (Triple.getArch() == Triple::thumb || Triple.getArch() == Triple::thumbeb) | if (Triple.getArch() == Triple::thumb || Triple.getArch() == Triple::thumbeb) | ||||
return ".text\n.balign 2\n.thumb\n" + InlineAsm; | return ".text\n.balign 2\n.thumb\n" + InlineAsm; | ||||
if (Triple.getArch() == Triple::arm || Triple.getArch() == Triple::armeb) | if (Triple.getArch() == Triple::arm || Triple.getArch() == Triple::armeb) | ||||
return ".text\n.balign 4\n.arm\n" + InlineAsm; | return ".text\n.balign 4\n.arm\n" + InlineAsm; | ||||
return InlineAsm; | return InlineAsm; | ||||
} | } | ||||
void IRLinker::updateAttributes(GlobalValue &GV) { | |||||
Since I envision this function will be expanded for other attribute updates, suggest moving this comment down into the body, where it is removing this attribute. tejohnson: Since I envision this function will be expanded for other attribute updates, suggest moving… | |||||
/// Remove nocallback attribute while linking, because nocallback attribute | |||||
/// indicates that the function is only allowed to jump back into caller's | |||||
/// module only by a return or an exception. When modules are linked, this | |||||
/// property cannot be guaranteed anymore. For example, the nocallback | |||||
/// function may contain a call to another module. But if we merge its caller | |||||
Not Done ReplyInline ActionsShould "important" be "imported"? Also, I'm not completely following the situation described in these sentences. Can you give me a more detailed example? I might be able to suggest clearer wording once I understand better. tejohnson: Should "important" be "imported"?
Also, I'm not completely following the situation described… | |||||
This is the great example that @mysterymath came up with to explain the problem in the bug reported to gcc (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106725) ==> ext.c <== void set_value(int v); void external_call(void) { set_value(0); } ==> lto.c <== static int value; void set_value(int v) { value = v; } int get_value(void) { return value; } ==> main.c <== #include <stdio.h> void set_value(int v); int get_value(void); __attribute__((leaf)) void external_call(void); int main(void) { set_value(42); external_call(); printf("%d\n", get_value()); } This is the scenario that I was trying to summarize here, where a function with nocallback attribute returns to its caller's module other than a return or an exception. external_call() is the function that has the leaf attribute, and main() is its caller. Normally, external_call() returns to its caller's module via a return. When lto.c and main.c are linked together, external_call() calls an imported function(set_value()), which is now in the same module with its caller. So, it returned to its caller's module via a function call, not with a return or an exception. gulfem: This is the great example that @mysterymath came up with to explain the problem in the bug… | |||||
Not Done ReplyInline ActionsOk, thanks. I understand this better after looking at the example here and in that bug. Per the bug, the issue comes in when ext.c is compiled to native code, and only main.c and lto.c are LTO-linked (merged). What would happen if all 3 of these were LTO-linked (merged)? Would there still be an issue? I.e. if the definition of external_call was linked into the same module as the call and the declaration that was marked leaf (which I guess turns into "nocallback" internally). In that case you would have one module that would not be calling out at all, so it isn't clear to me that nocallback would be wrong. Because the current code is stripping the nocallback attribute in all situations, it isn't clear to me whether it needs to or whether this is overly conservative. Essentially, you will never end up with a nocallback attribute on any LTO'ed code. Also, the usage of the word "imported" is a little confusing, because in LLVM we tend to use this terminology for ThinLTO, where definitions may be imported into other modules (i.e. we aren't merging the full modules like in regular LTO). In this case, the imported definitions get available_externally linkage, and thus have their definitions dropped after inlining (because the original definition is kept in the original module). In that case, I think we wouldn't need to drop the nocallback attribute, so I think in the ThinLTO case this may be a little overly conservative. tejohnson: Ok, thanks. I understand this better after looking at the example here and in that bug. Per the… | |||||
When all three modules are LTO-linked, leaf attribute is not going to have any effect based on the GCC description ("The attribute has no effect on functions defined within the current compilation unit"). You are right that we are being overly conservative about the usage of leaf attribute, and we plan to drop it for any LTO-ed code. The issue is that there was a lot of disagreement on the semantics of the leaf attribute for LTO usage (https://reviews.llvm.org/D131628), and we were not able to get a clear answer from the GCC community. Therefore, we decided to go with the restricted semantics. gulfem: When all three modules are LTO-linked, leaf attribute is not going to have any effect based on… | |||||
Thanks for the clarification. Can you add the note about it having no effect on functions defined within the unit to the description in D131628? Revisiting the example you included, I would suggest something like the following wording (keep your original 2 sentences but modify the example description): For example, the nocallback function may contain a call to another module. But if we merge its caller and callee module here, and not the module containing the nocallback function definition itself, the nocallback property will be violated (since the nocallback function will call back into the newly merged module containing both its caller and callee). This could happen if the module containing the nocallback function definition is native code, so it does not participate in the LTO link. Note if the nocallback function does participate in the LTO link, and thus ends up in the merged module containing its caller and callee, removing the attribute doesn't hurt as it has no effect on definitions in the same compilation unit. tejohnson: Thanks for the clarification. Can you add the note about it having no effect on functions… | |||||
I incorporated your comment in the latest revision, and extended the description in D131628. gulfem: I incorporated your comment in the latest revision, and extended the description in D131628. | |||||
/// and callee module here, and not the module containing the nocallback | |||||
/// function definition itself, the nocallback property will be violated | |||||
/// (since the nocallback function will call back into the newly merged module | |||||
/// containing both its caller and callee). This could happen if the module | |||||
/// containing the nocallback function definition is native code, so it does | |||||
/// not participate in the LTO link. Note if the nocallback function does | |||||
/// participate in the LTO link, and thus ends up in the merged module | |||||
/// containing its caller and callee, removing the attribute doesn't hurt as | |||||
/// it has no effect on definitions in the same module. | |||||
if (auto *F = dyn_cast<Function>(&GV)) { | |||||
if (!F->isIntrinsic()) | |||||
F->removeFnAttr(llvm::Attribute::NoCallback); | |||||
// Remove nocallback attribute when it is on a call-site. | |||||
for (BasicBlock &BB : *F) | |||||
for (Instruction &I : BB) | |||||
if (CallInst *CI = dyn_cast<CallInst>(&I)) | |||||
tejohnsonUnsubmitted Not Done ReplyInline ActionsShould this be CallBase (so it handles InvokeInst as well)? I missed during the review, but it was pointed out by @arsenm here: https://reviews.llvm.org/D139209#inline-1368889 tejohnson: Should this be CallBase (so it handles InvokeInst as well)? I missed during the review, but it… | |||||
gulfemAuthorUnsubmitted Thanks for pointing that out, and and I uploaded D141740. gulfem: Thanks for pointing that out, and and I uploaded D141740. | |||||
CI->removeFnAttr(Attribute::NoCallback); | |||||
} | |||||
} | |||||
Error IRLinker::run() { | Error IRLinker::run() { | ||||
// Ensure metadata materialized before value mapping. | // Ensure metadata materialized before value mapping. | ||||
if (SrcM->getMaterializer()) | if (SrcM->getMaterializer()) | ||||
if (Error Err = SrcM->getMaterializer()->materializeMetadata()) | if (Error Err = SrcM->getMaterializer()->materializeMetadata()) | ||||
return Err; | return Err; | ||||
// Inherit the target data from the source module if the destination module | // Inherit the target data from the source module if the destination module | ||||
// doesn't have one already. | // doesn't have one already. | ||||
▲ Show 20 Lines • Show All 224 Lines • Show Last 20 Lines |
Probably make the comment more generic, since I envision that this function will be expanded to do other attribute updates. Maybe call it "updateAttributes"?