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

Tue Jul 25 22:43:53 PDT 2017

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

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