[PATCH] D35875: ThinLTO: Don't import aliases of any kind (even linkonce_odr)

Wed Jul 26 09:00:36 PDT 2017

On Wed, Jul 26, 2017 at 6:47 AM Teresa Johnson <tejohnson at google.com> wrote:

> On Tue, Jul 25, 2017 at 10:43 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
>
>>
>>
>> On Tue, Jul 25, 2017 at 10:02 PM Mehdi AMINI <joker.eph at gmail.com> wrote:
>>
>>> 2017-07-25 21:32 GMT-07:00 David Blaikie <dblaikie at gmail.com>:
>>>
>>>>
>>>>
>>>> On Tue, Jul 25, 2017 at 9:27 PM Mehdi AMINI <joker.eph at gmail.com>
>>>> wrote:
>>>>
>>>>> 2017-07-25 21:17 GMT-07:00 David Blaikie <dblaikie at gmail.com>:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Jul 25, 2017 at 9:03 PM Mehdi AMINI via Phabricator <
>>>>>> reviews at reviews.llvm.org> wrote:
>>>>>>
>>>>>>> mehdi_amini added a comment.
>>>>>>>
>>>>>>> > they had to be emitted linkonce_odr in all the destination modules
>>>>>>> (even if they weren't used by an alias) rather than as available_externally
>>>>>>> - causing extra object size.
>>>>>>>
>>>>>>> Can you clarify this? It isn't clear to me.
>>>>>>>
>>>>>>
>>>>>> Functions imported as linkonce_odr, if they aren't optimized away,
>>>>>> must be emitted into the resulting object file. (whereas if they're
>>>>>> imported available_externally, if they aren't optimized away (inlined into
>>>>>> all call sites, etc) they can be dropped and the external definition can be
>>>>>> relied on)
>>>>>>
>>>>>
>>>>> Right, but the part I don't connect to is "even if they weren't used
>>>>> by an alias"? We're talking about function that are imported to be
>>>>> referenced by an alias, if they are not "used by an alias" what's their use
>>>>> then?
>>>>>
>>>>
>>>> Ah, no - this logic seems to apply to all linkonce_odr imported
>>>> functions. Yes, the impact could be improved/narrowed down to only apply
>>>> when imported for an alias. (though it'd still have the problems for debug
>>>> info, which is admittedly my original concern)
>>>>
>>>> The reduced test case where I hit this looked something like this:
>>>>
>>>> foo.cpp:
>>>>   inline __attribute__((optnone)) void f1() { }
>>>>   __attribute__((always_inline)) void f2() { f1(); }
>>>> bar.cpp:
>>>>   void f2();
>>>>   int main() { f2(); }
>>>>
>>>> f2 got imported and optimized away, f1 was imported but couldn't be
>>>> optimized away (due to optnone) & was emitted into bar.o as linkonce_odr (&
>>>> created a second CU to put it in).
>>>>
>>>
>>> That's not what I expect.
>>>
>>> I would need to double check but we are supposed to import linkonce_odr
>>> as available_externally except when they are reference by an alias.
>>> So in your case it shouldn't be emitted in bar.cpp since it should be
>>> imported as available_externally.
>>>
>>
> That's not what was actually happening though. We still had some code that
> was left over from when we forcibly imported linkonce_odr on reference
>  (before we did the weak resolution). That code is removed in David's
> patch.  See the change in FunctionImportGlobalProcessing::getLinkage which
> removed the following case:
>
>   case GlobalValue::LinkOnceAnyLinkage:
>   case GlobalValue::LinkOnceODRLinkage:
>     // These both stay the same when importing the definition.
>     // The ThinLTO pass will eventually force-import their definitions.
>     return SGV->getLinkage();
>
> The second part of the comment there is stale and wrong now, but never got
> cleaned up. This code is what sets the linkage type on the values being
> imported. Conversely, the weak resolution adjusts the linkage type on the
> values already in the module (and runs on the module in the backend
> *before* we do any importing).
>
> The fact that David is removing the above handling is what is requiring
> the other part of this patch - removing the special casing of handling for
> linkonce_odr aliases.
>
>>
>>>
>>> We "promote" linkonce_odr to weak_odr in their source module (foo.cpp in
>>> your case)  when their caller is imported somewhere else because of this:
>>> f2() inlined into main() will introduce a reference to f1() and it is no
>>> longer allowed to be discarded in foo.cpp.
>>>
>>
>> Yeah, I'm certainly confused as well - and internally (where I was
>> testing most of this, though I was debugging the final/specific backend
>> actions with an open source debug built clang) I'm not seeing that
>> promotion to available_externally even in simpler cases (one header with an
>> optnone inline function, two callers in separate .cc files, all ThinLTO'd
>> together). I do see an internal definition on one side and a weak (which
>> could be linkonce_odr or weak_odr at the LLVM level, they both get lowered
>> to ELF weak) definition on the other side, according to nm.
>>
>
> Use -Wl,-plugin-opt,save-temps (for gold at least) to get the bitcode out
> after each thinlto optimization to see the linkage types we change them to.
> The ones named .*.promote.bc include both promotion to global and the weak
> resolution changes, then .*.internalize.bc contain the subsequent
> internalization changes, etc.
>
> I wonder if the thinlto internalization is coming along and deciding to
> change the non-prevailing available_externally copy to internal linkage
> since it isn't exported. That would be a bug. Hmm, looking
> at thinLTOInternalizeAndPromoteGUID() it certainly seems like we could be
> doing that, I don't see a guard against converting available_externally to
> internal...
>

Thanks for the in person help - for posterity: We figured it out: My
example was degenerate. The second use of the inline function was in a dead
object file, which in the distributed ThinLTO case causes that object file
to be produced as normal, without any use of a summary. So it got a normal
linkonce_odr definition of the inline function and strong definition of the
caller to it (& since this object wasn't considered at all, the
linkonce_odr function was correctly internalized in the other object since
that was the only non-dead use of it). Once we fixed that we saw the
expected behavior: weak in one object, undefined (from
available_externally) in the other object.

Carry on...

>
>
>> Just more mysteries :/
>>
>> - Dave
>>
>>
>>>
>>>
>>> --
>>> Medhi
>>>
>>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> > There's no reason to believe this particularly narrow alias
>>>>>>> importing was especially/meaningfully important, only that it was
>>>>>>> /possible/ to implement in this way.
>>>>>>>
>>>>>>> Do you have benchmark results (SPEC and your internal ones)? Also
>>>>>>> statistics on the number of imports before/after?
>>>>>>>
>>>>>>
>>>>>> Nope - though I don't think there was any data to support the
>>>>>> original feature except the observation that it was possible in this
>>>>>> particular case, not that it was a common case, etc.
>>>>>>
>>>>>> I could probably create some way to gather statistics, might take on
>>>>>> the order of hours to build/gather/etc. My rough guess is that it's not
>>>>>> worth it to do, but certainly open to other perspectives.
>>>>>>
>>>>>> We'll certainly be running numbers on this internally (in the sense
>>>>>> that we'd notice fi it breaks anything). I can frontload that (by asking
>>>>>> Teresa & co what the best path is to do such an assessment internally) if
>>>>>> you think the risk is high enough to be worth it.
>>>>>>
>>>>>
>>>>> I don't know, I leave this assessment up to Teresa.  I was expecting
>>>>> that you guys have facilities to collect this information very easily :)
>>>>>
>>>>
>>>> Fair - I'll let Teresa speak to this (I haven't been dabbling much with
>>>> the optimization side of ThinLTO until recently, so not sure what the state
>>>> of the art/preferred level of validation is).
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ================
>>>>>>> Comment at: test/Linker/funcimport.ll:63
>>>>>>>  ; An alias to an imported function is imported as alias if the
>>>>>>> function is not
>>>>>>>  ; available_externally.
>>>>>>>  ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc
>>>>>>> -import=linkoncefunc:%t.bc -S | FileCheck %s --check-prefix=IMPORTGLOB5
>>>>>>> ----------------
>>>>>>> Is this comment still up to date?
>>>>>>>
>>>>>>
>>>>>> Looks stale, indeed - will update.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ================
>>>>>>> Comment at: test/ThinLTO/X86/alias_import.ll:5
>>>>>>>  ; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.index.bc
>>>>>>> %t2.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=PROMOTE
>>>>>>> -; RUN: llvm-lto -thinlto-action=promote -thinlto-index %t.index.bc
>>>>>>> %t2.bc -o - | llvm-lto -thinlto-action=internalize -thinlto-index
>>>>>>> %t.index.bc -thinlto-module-id=%t2.bc - -o - | llvm-dis -o - | FileCheck %s
>>>>>>> --check-prefix=PROMOTE-INTERNALIZE
>>>>>>>  ; RUN: llvm-lto -thinlto-action=import -thinlto-index %t.index.bc
>>>>>>> %t1.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
>>>>>>> ----------------
>>>>>>> It isn't clear to me that what is tested here is obsolete by your
>>>>>>> patch.
>>>>>>>
>>>>>>
>>>>>> There was a conversation on another thread with Peter (& an in person
>>>>>> conversation ) - see r274699 on the commits list.
>>>>>>
>>>>>
>>>>> OK! LGTM.
>>>>>
>>>>
>>>> Thanks for checking/looking/reviewing (:
>>>>
>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Mehdi
>>>>>
>>>>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170726/ce69d4ea/attachment-0001.html>