[PATCH] D14623: [ThinLTO] Comdat importing fixes and related cleanup

[Teresa Johnson via llvm-commits]

On Tue, Dec 1, 2015 at 7:13 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> On 30 November 2015 at 19:09, Teresa Johnson <tejohnson at google.com> wrote:
>> On Mon, Nov 30, 2015 at 1:49 PM, Rafael Espíndola
>> <rafael.espindola at gmail.com> wrote:
>>>> Right, which is why I have stripDeclsFromComdats() in this patch (see
>>>> the header comments for that new routine). Forgot about that, so I
>>>> think the issue with the change made in r254170 interacting badly with
>>>> comdat groups is not an issue. So the main thing is that what we want
>>>> here is for the alias to become a decl in the importing module, which
>>>> happens during global value proto linking at head and as a post-pass
>>>> after lazy linking in this patch.
>>>
>>>
>>> What I don't get it is why copy it just to drop it in the end. Can't
>>> we make the right decision when first looking at f2?
>>
>> Consider the following example:
>>
>> In the importing module:
>>
>> define i32 @main() #0 {
>> entry:
>>   call void (...) @func1()
>>   call void (...) @func2()
>>   ret i32 0
>> }
>>
>>
>> In the import source module:
>>
>> @alias1 = alias void (...), bitcast (void ()* @comdatfunc to void (...)*)
>> $comdatfunc = comdat any
>> define linkonce_odr void @comdatfunc() comdat {
>> entry:
>>   ret void
>> }
>>
>>
>> define void @func1() {
>> entry:
>>   ret void
>> }
>>
>> define void @func2() {
>>   call void @alias1()
>>   ret void
>> }
>>
>>
>> Now consider two importing cases:
>> 1) We import func1
>> 2) We import func2
>>
>> In case 1), we don't need to import the comdat group definitions,
>> since none are referenced. When we link global value protos and see
>> the alias,
>
> Lets try to abstract this away from the current implementation. Having
> a "link global value protos" stage is just the current, bad,
> organization.
>
> The steps needed in both cases are
> 1)
>  * Create a decl for func 1
>  * Move the body of func1. We are done.
>
> 2)
>  * Create a decl for func2
>  * Move the body func2
>  * Create an alias decl for alias1

Note that (for ThinLTO) we only need to create an alias decl for
alias1 if it is part of a comdat, at which point we need to add all
its comdat group members to the worklist of things to import/link. If
alias1 had not been part of a comdat group, at this point we just
create a non-alias decl of the aliasee type for alias1 and stop (no
recursion into aliasee). This is the part I am doing in
linkGlobalAliasProto in head and as a post pass in this patch.

>  * Recurse down the aliasee:
>   * create a decl for comdatfunc
>   * copy the body for comdatfunc
>  * Set the aliasee
>
> At no point do we need to create a GlobalVariable that becomes a
> GlobalAlias or the other way around.

Agreed that the above steps are at a high level what needs to be done.
I am just trying to get this to work within the bounds of the current
module linker implementation.

I skimmed over your new module linker implementation in D15084 and it
looks like that implementation operates similar to what you describe
above. The only issue for ThinLTO as I mentioned above is that the
comdat groups to import/link will not be passed into the module
linker, but will be discovered as the import function bodies are
copied in.

Thanks,
Teresa

>
> Cheers,
> Rafael



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413