[llvm] r274699 - ThinLTO: Add test cases for promote+internalize.

Tue Jul 25 20:19:44 PDT 2017

I'd be in favour of that, especially if you are planning to remove the
logic that makes linkonce aliases special.

Peter

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

> I'm not sure either of us are making much sense of what this test does, or
> is intended to do - I think it's best to just remove it.
>
> On Tue, Jul 25, 2017 at 7:51 PM Peter Collingbourne <peter at pcc.me.uk>
> wrote:
>
>> On Tue, Jul 25, 2017 at 5:52 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Jul 25, 2017 at 5:49 PM David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> On Tue, Jul 25, 2017 at 4:24 PM Peter Collingbourne <peter at pcc.me.uk>
>>>> wrote:
>>>>
>>>>> On Tue, Jul 25, 2017 at 2:29 PM, David Blaikie <dblaikie at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hey Peter,
>>>>>>
>>>>>> Bit of thread necromancy here - but I'm trying to change some things
>>>>>> wrt alias+linkonce_odr (going to make it behave like the alias+external
>>>>>> case, for simplicity, less duplication, and simplified debug info) & this
>>>>>> test is failing:
>>>>>>
>>>>>> My change causes none of the aliasees to be imported (not even those
>>>>>> linkonce_odr ones).
>>>>>>
>>>>>
>>>>> Is that a good idea? I would have thought that we'd want to go in the
>>>>> opposite direction and try to import more functions that use aliases. I
>>>>> believe that is blocked on support for available_externally aliases though
>>>>> (this thread has the most recent discussion I can find on that topic:
>>>>> http://lists.llvm.org/pipermail/llvm-dev/2017-January/109227.html).
>>>>>
>>>>
>>>> Right, ultimately that's the right direction - this functionality
>>>> though, is a dead-end, it's not a stepping stone towards that outcome - and
>>>> it gets in the way (specifically it gets in the way of debug info, but also
>>>> means thinlto backend objects are larger than they need to be - by having
>>>> linkonce functions when they could be relying on the
>>>> linkonce-promoted-to-weak definition) without any specific reason to
>>>> believe the benefit (optimizing across aliases of linkonce_odr functions)
>>>> is significant.
>>>>
>>>>
>>>>>
>>>>> This causes -thinlto-action=internalize to fail ("-internalize will
>>>>>> not perform without -exportedsymbol").
>>>>>>
>>>>>
>>>>> Is it really failing because of that?
>>>>> http://llvm-cs.pcc.me.uk/tools/llvm-lto/llvm-lto.cpp#618
>>>>> Looks like it just prints a warning.
>>>>>
>>>>
>>>> Yep, it prints a warning & then doesn't internalize anything/is a
>>>> no-op, as per the warning.
>>>>
>>>
>> That warning is misleading because the condition it tests for is not the
>> same as the one that ThinLTOCodeGenerator uses.
>> http://llvm-cs.pcc.me.uk/lib/LTO/ThinLTOCodeGenerator.cpp#763
>> Right now we apply internalization despite the warning
>> because ExportList.size() is equal to 3 at that point. I think what might
>> be happening for you is that your change causes ExportList.size() to become
>> 0, so we don't actually internalize anything. Presumably the 3 symbols are
>> the linkonce aliases but I don't have time to confirm that.
>>
>>
>>>>
>>>>> It looks like if I fix this by passing -exported-symbol=main, nothing
>>>>>> is internalized (all the aliases are called from main) so it may defeat the
>>>>>> purpose of this test?
>>>>>>
>>>>>
>>>>> Right, if we export main then that would prevent us from internalizing
>>>>> anything across modules that is referenced by main, as we don't currently
>>>>> support cross-module internalization.
>>>>>
>>>>
>>>> Not sure I follow that - it seemed to work (in the sense that saying
>>>> main was exported, nothing was internalized - which seems like the correct
>>>> answer to me, rather than "unsupported" (though unsupported could mean it
>>>> does that but breaks elsewhere - fair enough))
>>>>
>>>>
>>>>> (I don't really understand how this was working/what it was doing
>>>>>> before, though: What kept the aliasees alive if not the calls from main?
>>>>>> Maybe a record in the index saying "these are being used elsewhere" - but
>>>>>> why isn't there a similar record for the aliases that would've stopped them
>>>>>> from being internalized in the first place?)
>>>>>>
>>>>>
>>>>> I don't recall for sure but I believe that the point of that part of
>>>>> the test was to show that if no symbols are exported, the linkage changes
>>>>> applied by promote are unimportant because they are wiped out by a later
>>>>> internalize pass (which would internalize pretty much everything because
>>>>> nothing is exported).
>>>>>
>>>>
>>>> Except there are exported symbols... - if there are no exported symbols
>>>> then internalize doesn't run so it wouldn't show/demonstrate anything?
>>>>
>>>
>> The symbols currently being exported are linkonce/weak so they presumably
>> end up in ExportList regardless of what flags you pass in.
>>
>>>
>>> aside: It looked like the behavior was incorrect anyway: It internalized
>>> the aliases which were actually called cross-module, but it /didn't/
>>> internalize the things that were cross-modular imported... - it looks like
>>> it did exactly the opposite of what would be desired?
>>>
>>
>> That's certainly possible, although I suspect it is more likely an
>> artifact of how llvm-lto works (which I find super confusing -- one of the
>> reasons why I designed llvm-lto2 to use the same sequence of ThinLTO
>> "passes" as production links).
>>
>> Peter
>>
>>>
>>>
>>>>
>>>>
>>>>> So I think the right change to make in that part of the test (if we
>>>>> wanted to go with your proposed change) would be to test that the remaining
>>>>> declarations are internal.
>>>>>
>>>>> Peter
>>>>>
>>>>>
>>>>>> Preciate any thoughts you have here,
>>>>>>
>>>>>> - Dave
>>>>>>
>>>>>>
>>>>>> On Wed, Jul 6, 2016 at 4:00 PM Peter Collingbourne via llvm-commits <
>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Author: pcc
>>>>>>> Date: Wed Jul  6 17:53:02 2016
>>>>>>> New Revision: 274699
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=274699&view=rev
>>>>>>> Log:
>>>>>>> ThinLTO: Add test cases for promote+internalize.
>>>>>>>
>>>>>>> This tests the effect of both promotion and internalization on a
>>>>>>> module,
>>>>>>> and helps show that D21883 is NFC wrt promotion+internalization.
>>>>>>>
>>>>>>> Differential Revision: http://reviews.llvm.org/D21915
>>>>>>>
>>>>>>> Modified:
>>>>>>>     llvm/trunk/test/ThinLTO/X86/alias_import.ll
>>>>>>>     llvm/trunk/test/ThinLTO/X86/weak_resolution.ll
>>>>>>>
>>>>>>> Modified: llvm/trunk/test/ThinLTO/X86/alias_import.ll
>>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>>>>>>> ThinLTO/X86/alias_import.ll?rev=274699&r1=274698&r2=274699&view=diff
>>>>>>> ============================================================
>>>>>>> ==================
>>>>>>> --- llvm/trunk/test/ThinLTO/X86/alias_import.ll (original)
>>>>>>> +++ llvm/trunk/test/ThinLTO/X86/alias_import.ll Wed Jul  6 17:53:02
>>>>>>> 2016
>>>>>>> @@ -2,6 +2,7 @@
>>>>>>>  ; RUN: opt -module-summary %p/Inputs/alias_import.ll -o %t2.bc
>>>>>>>  ; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc
>>>>>>> %t2.bc
>>>>>>>  ; 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
>>>>>>>
>>>>>>>  ;
>>>>>>> @@ -86,7 +87,45 @@
>>>>>>>  ; IMPORT-DAG: declare void @weakfuncWeakODRAlias()
>>>>>>>  ; IMPORT-DAG: declare void @weakfuncLinkonceODRAlias()
>>>>>>>
>>>>>>> -
>>>>>>> +; Promotion + internalization should internalize all of these,
>>>>>>> except for aliases of
>>>>>>> +; linkonce_odr functions, because alias to linkonce_odr are the
>>>>>>> only aliases we will
>>>>>>> +; import (see selectCallee() in FunctionImport.cpp).
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @globalfuncAlias = internal alias void
>>>>>>> (...), bitcast (void ()* @globalfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @globalfuncWeakAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @globalfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @globalfuncLinkonceAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @globalfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @globalfuncWeakODRAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @globalfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @globalfuncLinkonceODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @globalfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @internalfuncAlias = internal alias void
>>>>>>> (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @internalfuncWeakAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @internalfuncLinkonceAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @internalfuncWeakODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @internalfuncLinkonceODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @internalfunc.llvm.0 to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncAlias = alias void
>>>>>>> (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncWeakAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncLinkonceAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncWeakODRAlias = weak_odr
>>>>>>> alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkonceODRfuncLinkonceODRAlias =
>>>>>>> weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void
>>>>>>> (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakODRfuncAlias = internal alias void
>>>>>>> (...), bitcast (void ()* @weakODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakODRfuncWeakAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @weakODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakODRfuncLinkonceAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakODRfuncWeakODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakODRfuncLinkonceODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @weakODRfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkoncefuncAlias = internal alias void
>>>>>>> (...), bitcast (void ()* @linkoncefunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkoncefuncWeakAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @linkoncefunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkoncefuncLinkonceAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkoncefuncWeakODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @linkoncefuncLinkonceODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakfuncAlias = internal alias void
>>>>>>> (...), bitcast (void ()* @weakfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakfuncWeakAlias = internal alias void
>>>>>>> (...), bitcast (void ()* @weakfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakfuncLinkonceAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @weakfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakfuncWeakODRAlias = internal alias
>>>>>>> void (...), bitcast (void ()* @weakfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: @weakfuncLinkonceODRAlias = internal
>>>>>>> alias void (...), bitcast (void ()* @weakfunc to void (...)*)
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: define internal void @globalfunc()
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: define internal void
>>>>>>> @internalfunc.llvm.0()
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: define internal void @linkonceODRfunc()
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: define internal void @weakODRfunc()
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: define internal void @linkoncefunc()
>>>>>>> +; PROMOTE-INTERNALIZE-DAG: define internal void @weakfunc()
>>>>>>>
>>>>>>>  define i32 @main() #0 {
>>>>>>>  entry:
>>>>>>>
>>>>>>> Modified: llvm/trunk/test/ThinLTO/X86/weak_resolution.ll
>>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>>>>>>> ThinLTO/X86/weak_resolution.ll?rev=274699&r1=274698&r2=
>>>>>>> 274699&view=diff
>>>>>>> ============================================================
>>>>>>> ==================
>>>>>>> --- llvm/trunk/test/ThinLTO/X86/weak_resolution.ll (original)
>>>>>>> +++ llvm/trunk/test/ThinLTO/X86/weak_resolution.ll Wed Jul  6
>>>>>>> 17:53:02 2016
>>>>>>> @@ -7,6 +7,7 @@
>>>>>>>  ; non-prevailing ODR are not kept when possible, but non-ODR
>>>>>>> non-prevailing
>>>>>>>  ; are not affected.
>>>>>>>  ; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc
>>>>>>> -o - | llvm-dis -o - | FileCheck %s --check-prefix=MOD1
>>>>>>> +; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc
>>>>>>> -exported-symbol=linkoncefunc -o - | llvm-lto -thinlto-action=internalize
>>>>>>> -thinlto-module-id=%t.bc - -thinlto-index=%t3.bc
>>>>>>> -exported-symbol=linkoncefunc -o - | llvm-dis -o - | FileCheck %s
>>>>>>> --check-prefix=MOD1-INT
>>>>>>>  ; RUN: llvm-lto -thinlto-action=promote %t2.bc
>>>>>>> -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s
>>>>>>> --check-prefix=MOD2
>>>>>>>  ; When exported, we always preserve a linkonce
>>>>>>>  ; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc
>>>>>>> -o - --exported-symbol=linkonceodrfuncInSingleModule | llvm-dis -o
>>>>>>> - | FileCheck %s --check-prefix=EXPORTED
>>>>>>> @@ -47,6 +48,7 @@ entry:
>>>>>>>    ret void
>>>>>>>  }
>>>>>>>  ; MOD1: define weak void @linkoncefunc()
>>>>>>> +; MOD1-INT: define weak void @linkoncefunc()
>>>>>>>  ; MOD2: define linkonce void @linkoncefunc()
>>>>>>>  define linkonce void @linkoncefunc() #0 {
>>>>>>>  entry:
>>>>>>> @@ -66,6 +68,7 @@ entry:
>>>>>>>  }
>>>>>>>
>>>>>>>  ; MOD1: define linkonce_odr void @linkonceodrfuncInSingleModule()
>>>>>>> +; MOD1-INT: define internal void @linkonceodrfuncInSingleModule()
>>>>>>>  ; EXPORTED: define weak_odr void @linkonceodrfuncInSingleModule()
>>>>>>>  define linkonce_odr void @linkonceodrfuncInSingleModule() #0 {
>>>>>>>  entry:
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> --
>>>>> Peter
>>>>>
>>>>
>>
>>
>> --
>> --
>> Peter
>>
>

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170725/7236f525/attachment.html>