[llvm] r257181 - [ThinLTO] Use new in-place symbol changes for exporting module

[Mehdi Amini via llvm-commits]

> On Jan 8, 2016, at 9:18 AM, Teresa Johnson <tejohnson at google.com> wrote:
> 
> On Fri, Jan 8, 2016 at 9:15 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>> 
>>> On Jan 8, 2016, at 9:06 AM, Teresa Johnson via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>> 
>>> Author: tejohnson
>>> Date: Fri Jan  8 11:06:29 2016
>>> New Revision: 257181
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=257181&view=rev
>>> Log:
>>> [ThinLTO] Use new in-place symbol changes for exporting module
>>> 
>>> Due to the new in-place ThinLTO symbol handling support added in
>>> r257174, we now invoke renameModuleForThinLTO on the current
>>> module from within the FunctionImport pass.
>> 
>> While this is simpler, this make the FunctionImporter doing more than just importing. It couples two steps.
>> Might not be that important, but does not seem straightforward to me either.
> 
> Point taken. Open to suggestions on where to move it. I wanted to move
> it out of clang though as it isn't needed there anymore. Should it be
> a separate (new) pass?

I’d leave it like that for now, keep it simple (even if it is coupled) till we need to decouple the two steps (unless you already foresee a use case).

— 
Mehdi


> 
> Teresa
> 
>> 
>> —
>> Mehdi
>> 
>> 
>> 
>> 
>>> 
>>> Additionally, renameModuleForThinLTO no longer needs to return the
>>> Module as it is performing the renaming in place on the one provided.
>>> 
>>> This commit will be immediately preceeded by a companion clang patch to
>>> remove its invocation of renameModuleForThinLTO.
>>> 
>>> Modified:
>>>   llvm/trunk/include/llvm/Linker/Linker.h
>>>   llvm/trunk/lib/Linker/LinkModules.cpp
>>>   llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>>> 
>>> Modified: llvm/trunk/include/llvm/Linker/Linker.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Linker/Linker.h?rev=257181&r1=257180&r2=257181&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Linker/Linker.h (original)
>>> +++ llvm/trunk/include/llvm/Linker/Linker.h Fri Jan  8 11:06:29 2016
>>> @@ -69,8 +69,7 @@ public:
>>> 
>>> /// Perform in-place global value handling on the given Module for
>>> /// exported local functions renamed and promoted for ThinLTO.
>>> -std::unique_ptr<Module> renameModuleForThinLTO(std::unique_ptr<Module> M,
>>> -                                               const FunctionInfoIndex *Index);
>>> +bool renameModuleForThinLTO(Module &M, const FunctionInfoIndex *Index);
>>> 
>>> } // End llvm namespace
>>> 
>>> 
>>> Modified: llvm/trunk/lib/Linker/LinkModules.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=257181&r1=257180&r2=257181&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Linker/LinkModules.cpp (original)
>>> +++ llvm/trunk/lib/Linker/LinkModules.cpp Fri Jan  8 11:06:29 2016
>>> @@ -863,13 +863,9 @@ bool Linker::linkModules(Module &Dest, s
>>>  return L.linkInModule(std::move(Src), Flags);
>>> }
>>> 
>>> -std::unique_ptr<Module>
>>> -llvm::renameModuleForThinLTO(std::unique_ptr<Module> M,
>>> -                             const FunctionInfoIndex *Index) {
>>> -  ThinLTOGlobalProcessing ThinLTOProcessing(*M, Index);
>>> -  if (ThinLTOProcessing.run())
>>> -    return nullptr;
>>> -  return M;
>>> +bool llvm::renameModuleForThinLTO(Module &M, const FunctionInfoIndex *Index) {
>>> +  ThinLTOGlobalProcessing ThinLTOProcessing(M, Index);
>>> +  return ThinLTOProcessing.run();
>>> }
>>> 
>>> //===----------------------------------------------------------------------===//
>>> 
>>> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp?rev=257181&r1=257180&r2=257181&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Fri Jan  8 11:06:29 2016
>>> @@ -413,14 +413,19 @@ public:
>>>      Index = IndexPtr.get();
>>>    }
>>> 
>>> +    // First we need to promote to global scope and rename any local values that
>>> +    // are potentially exported to other modules.
>>> +    if (renameModuleForThinLTO(M, Index)) {
>>> +      errs() << "Error renaming module\n";
>>> +      return false;
>>> +    }
>>> +
>>>    // Perform the import now.
>>>    auto ModuleLoader = [&M](StringRef Identifier) {
>>>      return loadFile(Identifier, M.getContext());
>>>    };
>>>    FunctionImporter Importer(*Index, ModuleLoader);
>>>    return Importer.importFunctions(M);
>>> -
>>> -    return false;
>>>  }
>>> };
>>> } // anonymous namespace
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> 
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413