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

[David Blaikie via llvm-commits]

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)


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


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


>
>
> ================
> Comment at: test/Transforms/FunctionImport/funcimport.ll:43
>
>  ; Aliases import the aliasee function
>  declare void @linkoncealias(...) #1
> ----------------
> Comment seems out-of-date?
>

Indeed. Will update - thanks!

- Dave


>
>
> https://reviews.llvm.org/D35875
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170726/01753488/attachment.html>