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


>
>
>
>>
>>
>>>
>>> > 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/f5d6949c/attachment.html>