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

[Teresa Johnson via llvm-commits]

On Fri, Jan 8, 2016 at 9:20 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>
>> 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).

Thinking through it a bit more - if we want to disable function
importing for a particular module when debugging, this would currently
also disable promotion/renaming on it which is required when its
functions are imported by a different module, which could lead to link
failures. So it does seem good to decouple them. At the least I will
add a FIXME to that effect.

Teresa

>
> —
> 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
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413