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

[Mehdi AMINI via llvm-commits]

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?



>
>
>>
>> > 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 :)




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


-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170725/71e519ff/attachment.html>