[llvm] r260122 - [ThinLTO] Remove imported available externally defs from comdats.

[David Blaikie via llvm-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160208/4d7f9d9d/attachment-0001.html>