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

Wed Jul 26 06:47:56 PDT 2017

On Tue, Jul 25, 2017 at 10:43 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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.
>>
>
That's not what was actually happening though. We still had some code that
was left over from when we forcibly imported linkonce_odr on reference
 (before we did the weak resolution). That code is removed in David's
patch.  See the change in FunctionImportGlobalProcessing::getLinkage which
removed the following case:

  case GlobalValue::LinkOnceAnyLinkage:
  case GlobalValue::LinkOnceODRLinkage:
    // These both stay the same when importing the definition.
    // The ThinLTO pass will eventually force-import their definitions.
    return SGV->getLinkage();

The second part of the comment there is stale and wrong now, but never got
cleaned up. This code is what sets the linkage type on the values being
imported. Conversely, the weak resolution adjusts the linkage type on the
values already in the module (and runs on the module in the backend
*before* we do any importing).

The fact that David is removing the above handling is what is requiring the
other part of this patch - removing the special casing of handling for
linkonce_odr aliases.

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

Use -Wl,-plugin-opt,save-temps (for gold at least) to get the bitcode out
after each thinlto optimization to see the linkage types we change them to.
The ones named .*.promote.bc include both promotion to global and the weak
resolution changes, then .*.internalize.bc contain the subsequent
internalization changes, etc.

I wonder if the thinlto internalization is coming along and deciding to
change the non-prevailing available_externally copy to internal linkage
since it isn't exported. That would be a bug. Hmm, looking
at thinLTOInternalizeAndPromoteGUID() it certainly seems like we could be
doing that, I don't see a guard against converting available_externally to
internal...

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

-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170726/73033f4e/attachment.html>