[llvm] r260122 - [ThinLTO] Remove imported available externally defs from comdats.
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 22 11:10:42 PST 2016
Ping (David Majnemer: see question from David Blaikie below).
Thanks,
Teresa
On Mon, Feb 8, 2016 at 2:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Feb 8, 2016 at 1:04 PM, Teresa Johnson <tejohnson at google.com>
> wrote:
>
>>
>>
>> On Mon, Feb 8, 2016 at 11:27 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Mon, Feb 8, 2016 at 11:22 AM, Teresa Johnson <tejohnson at google.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Feb 8, 2016 at 11:06 AM, David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> I couldn't quite follow the test - but wouldn't removing things (or in
>>>>> any way modifyidng) from a COMDAT cause problems? (the COMDAT would no
>>>>> longer match between objects, etc?)
>>>>>
>>>>
>>>> The comdat will not be emitted in the importing module if we import its
>>>> definitions as available_externally, since any such imported defs will be
>>>> removed from the comdat with this patch. Any non-imported members remained
>>>> decls and are not part of a comdat in the importing module either.
>>>>
>>>
>>> Ah, right - thanks! (the available_externally helps me understand)
>>>
>>> Though perhaps we should remove the comdats, to be safe? Once we're
>>> adding/removing things from them they're no longer accurate anyway. Is
>>> there any benefit to keeping them? Seems like they're just likely to be a
>>> trap/misleading thing to keep them around.
>>>
>>
>> I can do that. The empty comdats never get emitted but I am not sure
>> where they are ultimately removed/suppressed.
>>
>
> Fair enough - I'm probably not the right person to make that call, but
> David Majnemer might be able to sanity-check & see if my suggestion is
> meaningful/useful.
>
> - Dave
>
>
>> Teresa
>>
>>
>>>
>>>>
>>>>
>>>>> On Mon, Feb 8, 2016 at 10:47 AM, Teresa Johnson via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> Author: tejohnson
>>>>>> Date: Mon Feb 8 12:47:20 2016
>>>>>> New Revision: 260122
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=260122&view=rev
>>>>>> Log:
>>>>>> [ThinLTO] Remove imported available externally defs from comdats.
>>>>>>
>>>>>> Summary:
>>>>>> Available externally definitions are considered declarations for the
>>>>>> linker and eventually dropped. As such they are not allowed to be
>>>>>> in comdats. Remove any such imported functions from comdats.
>>>>>>
>>>>>> Reviewers: rafael
>>>>>>
>>>>>> Subscribers: davidxl, llvm-commits, joker.eph
>>>>>>
>>>>>> Differential Revision: http://reviews.llvm.org/D16120
>>>>>>
>>>>>> Added:
>>>>>> llvm/trunk/test/Linker/Inputs/funcimport_comdat.ll
>>>>>> llvm/trunk/test/Linker/funcimport_comdat.ll
>>>>>> Modified:
>>>>>> llvm/trunk/lib/Linker/LinkModules.cpp
>>>>>>
>>>>>> Modified: llvm/trunk/lib/Linker/LinkModules.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=260122&r1=260121&r2=260122&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/lib/Linker/LinkModules.cpp (original)
>>>>>> +++ llvm/trunk/lib/Linker/LinkModules.cpp Mon Feb 8 12:47:20 2016
>>>>>> @@ -722,9 +722,21 @@ void ThinLTOGlobalProcessing::processGlo
>>>>>> GV.setVisibility(GlobalValue::HiddenVisibility);
>>>>>> if (isModuleExporting())
>>>>>> NewExportedValues.insert(&GV);
>>>>>> - return;
>>>>>> + } else
>>>>>> + GV.setLinkage(getLinkage(&GV));
>>>>>> +
>>>>>> + // Remove functions imported as available externally defs from
>>>>>> comdats,
>>>>>> + // as this is a declaration for the linker, and will be dropped
>>>>>> eventually.
>>>>>> + // It is illegal for comdats to contain declarations.
>>>>>> + auto *GO = dyn_cast_or_null<GlobalObject>(&GV);
>>>>>> + if (GO && GO->isDeclarationForLinker() && GO->hasComdat()) {
>>>>>> + // The IRMover should not have placed any imported declarations
>>>>>> in
>>>>>> + // a comdat, so the only declaration that should be in a comdat
>>>>>> + // at this point would be a definition imported as
>>>>>> available_externally.
>>>>>> + assert(GO->hasAvailableExternallyLinkage() &&
>>>>>> + "Expected comdat on definition (possibly available
>>>>>> external)");
>>>>>> + GO->setComdat(nullptr);
>>>>>> }
>>>>>> - GV.setLinkage(getLinkage(&GV));
>>>>>> }
>>>>>>
>>>>>> void ThinLTOGlobalProcessing::processGlobalsForThinLTO() {
>>>>>>
>>>>>> Added: llvm/trunk/test/Linker/Inputs/funcimport_comdat.ll
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/funcimport_comdat.ll?rev=260122&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/test/Linker/Inputs/funcimport_comdat.ll (added)
>>>>>> +++ llvm/trunk/test/Linker/Inputs/funcimport_comdat.ll Mon Feb 8
>>>>>> 12:47:20 2016
>>>>>> @@ -0,0 +1,4 @@
>>>>>> +define i32 @main() #0 {
>>>>>> +entry:
>>>>>> + ret i32 0
>>>>>> +}
>>>>>>
>>>>>> Added: llvm/trunk/test/Linker/funcimport_comdat.ll
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/funcimport_comdat.ll?rev=260122&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/test/Linker/funcimport_comdat.ll (added)
>>>>>> +++ llvm/trunk/test/Linker/funcimport_comdat.ll Mon Feb 8 12:47:20
>>>>>> 2016
>>>>>> @@ -0,0 +1,28 @@
>>>>>> +; Do setup work for all below tests: generate bitcode and combined
>>>>>> index
>>>>>> +; RUN: llvm-as -function-summary %s -o %t.bc
>>>>>> +; RUN: llvm-as -function-summary %p/Inputs/funcimport_comdat.ll -o
>>>>>> %t2.bc
>>>>>> +; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
>>>>>> +
>>>>>> +; Ensure linking of comdat containing external linkage global and
>>>>>> function
>>>>>> +; removes the imported available_externally defs from comdat.
>>>>>> +; RUN: llvm-link %t2.bc -functionindex=%t3.thinlto.bc
>>>>>> -import=comdat1_func1:%t.bc -S | FileCheck %s --check-prefix=IMPORTCOMDAT
>>>>>> +; IMPORTCOMDAT-NOT: $comdat1 = comdat any
>>>>>> +; IMPORTCOMDAT-NOT: comdat($comdat1)
>>>>>> +
>>>>>> +; Ensure linking of comdat containing internal linkage function with
>>>>>> alias
>>>>>> +; removes the imported and promoted available_externally defs from
>>>>>> comdat.
>>>>>> +; RUN: llvm-link %t2.bc -functionindex=%t3.thinlto.bc
>>>>>> -import=comdat2_func1:%t.bc -S | FileCheck %s --check-prefix=IMPORTCOMDAT2
>>>>>> +; IMPORTCOMDAT2-NOT: $comdat2 = comdat any
>>>>>> +; IMPORTCOMDAT2-NOT: comdat($comdat2)
>>>>>> +
>>>>>> +$comdat1 = comdat any
>>>>>> + at comdat1_glob = global i32 0, comdat($comdat1)
>>>>>> +define void @comdat1_func1() comdat($comdat1) {
>>>>>> + ret void
>>>>>> +}
>>>>>> +
>>>>>> +$comdat2 = comdat any
>>>>>> + at comdat2_alias = alias void (), void ()* @comdat2_func1
>>>>>> +define internal void @comdat2_func1() comdat($comdat2) {
>>>>>> + ret void
>>>>>> +}
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>>
>
>
--
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160222/51118fe1/attachment.html>
More information about the llvm-commits
mailing list