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

Tue Jul 25 22:02:08 PDT 2017

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.

-- 
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/20170725/875b0450/attachment-0001.html>