llvm dev - Aug 2016 - Use of array type in globals in LTO

> On Aug 26, 2016, at 2:06 PM, Teresa Johnson <tejohnson at google.com>
wrote:
> 
> Mehdi: I see what is going on:
> 
> +    ArrayType *Ty > +       
ArrayType::get(Type::getInt8Ty(RegularLTO.Ctx), I.second.Size);
> +    GlobalVariable *OldGV =
RegularLTO.CombinedModule->getNamedGlobal(I.first);
> +    if (OldGV && OldGV->getType()->getElementType() == Ty) {
> +      // Don't create a new global if the type is already correct,
just make
> +      // sure the alignment is correct.
> 
> We compare the existing type to an ArrayType constructed from the
> recorded merged common size. So
> the original i32 type does not look the same (since it compares
> against [4 x i8]).
Right, but that the code I wrote in r279417 right? I guess we should compare the
*size* first, and only do something if the size differs.

How was Gold doing it before r278338?

— 
Mehdi



> 
> Not that familiar with type handling in LLVM, but I guess I need to
> just check if the type size is the same?
> 
> On Fri, Aug 26, 2016 at 1:57 PM, Teresa Johnson <tejohnson at
google.com> wrote:
>> Thanks for the test case! I can reproduce this, and see with the
>> compiler I saved from just before r278338 that this is indeed a chance
>> in behavior. Looking at why this changed...
>> 
>> On Fri, Aug 26, 2016 at 1:42 PM, Mehdi Amini <mehdi.amini at
apple.com> wrote:
>>> 
>>>> On Aug 26, 2016, at 1:34 PM, junbuml at codeaurora.org wrote:
>>>> 
>>>> On 2016-08-26 12:47, Mehdi Amini wrote:
>>>>>> On Aug 26, 2016, at 9:06 AM, junbuml at codeaurora.org
wrote:
>>>>>> On 2016-08-26 11:32, Mehdi Amini wrote:
>>>>>>> Hi,
>>>>>>>> Recently, I noticed that less number of global
variables are merged in global-merge pass and in some global variable, array
types are used instead of its original type. For example, [4xi8] with align 4 is
used for a i32  global variable. For me, it seems that such pattern is observed
after r278338  (Resolution-based LTO API).
>>>>>>> Are you sure it is performed in the global merge
pass? Can you provide
>>>>>>> an example of input IR where you see this now but
didn’t before?
>>>>>>> Also can you confirm you’re using the gold-linker?
>>>>>> I used gold linker. In spec2006/perlbench, I observed
the less number of globals are merged in GlobalMerge.cpp after r278338. The
reason is because, from the very first pass, several global variables use [4xi8]
with align 4, instead of its original type i32 after r278338. Current
GlobalMerge pass doesn't seem to handle such fancy-aligned globals. If such
type change (e.g., from i32 to [4xi8]) in global variables was intended in
r278338, I think we should enhance GlobalMerge to handle such cases.
>>>>> Can you submit a reproduction for Gold please?
>>>>> We need to understand what changed with the new LTO API.
>>>> 
>>>> 
>>>> I compiled below C code for aarch64 in lto using gold
(--target=aarch64-linux-gnu  -flto -fuse-ld=gold). After r278338, two globals,
GVi32_a and GVi32_b, are [4 x i8] type in the input IR to GlobalMerge.
Therefore, GlobalMerge do not even start to handle them because as of now it
ignores fancy-aligned globals.  Before r278338, GVi32_a and GVi32_b seems to be
i32 in the input IR to GlobalMerge. Is this change in the input IR expected ?
>>>> 
>>>> -------------------------
>>>> int GVi32_a  ;
>>>> int GVi32_b  ;
>>> 
>>> These are common variables, this is what I mentioned in my first
email. Compiling with -fno-commons or defining them with “int GVi32_a = 0;”
should solve it.
>>> 
>>> However r278338 is not supposed to have changed anything on this
aspect. I would have expected maybe r279417 playing a role there.
>>> 
>>> Anyway I don’t have Gold, so I’ll leave Teresa investigate why the
change in behavior.
>>> 
>>> Do you want to try improving global merge to try to handle this
case?
>>> 
>>> —
>>> Mehdi
>>> 
>>> 
>>> 
>>> 
>>> 
>>>> 
>>>> __attribute__((noinline)) void setGV(int a) {
>>>> GVi32_a = a ;
>>>> GVi32_b = a ;
>>>> }
>>>> 
>>>> __attribute__((noinline)) int loadGV() {
>>>> return GVi32_a + GVi32_b  ;
>>>> }
>>>> 
>>>> int main(int argc, char *argv[]){
>>>> setGV(argc);
>>>> return loadGV();
>>>> }
>>>> -------------------------
>>>> 
>>>> 
>>>> 
>>>>> —
>>>>> Mehdi
>>>> 
>>>> --
>>>> Qualcomm Datacenter Technologies, Inc. as an affiliate of
Qualcomm Technologies, Inc.
>>>> Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.
>>> 
>> 
>> 
>> 
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com |
408-460-2413
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413

On Fri, Aug 26, 2016 at 3:07 PM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
>
> > On Aug 26, 2016, at 2:06 PM, Teresa Johnson <tejohnson at
google.com>
> wrote:
> >
> > Mehdi: I see what is going on:
> >
> > +    ArrayType *Ty > > +       
ArrayType::get(Type::getInt8Ty(RegularLTO.Ctx), I.second.Size);
> > +    GlobalVariable *OldGV = RegularLTO.CombinedModule->
> getNamedGlobal(I.first);
> > +    if (OldGV && OldGV->getType()->getElementType() ==
Ty) {
> > +      // Don't create a new global if the type is already
correct, just
> make
> > +      // sure the alignment is correct.
> >
> > We compare the existing type to an ArrayType constructed from the
> > recorded merged common size. So
> > the original i32 type does not look the same (since it compares
> > against [4 x i8]).
>
> Right, but that the code I wrote in r279417 right? I guess we should
> compare the *size* first, and only do something if the size differs.
>
Yeah. In fact the original handling added in r278338 (addCommons) didn't
even compare the size, it always created a new merged common with an
ArrayType.

>
> How was Gold doing it before r278338?
>
It was just letting the ModuleLinker handle it (which kept the largest one
by size), and then it fixed up the alignment later.

Will change to first compare the size.

>
> —
> Mehdi
>
>
>
>
> >
> > Not that familiar with type handling in LLVM, but I guess I need to
> > just check if the type size is the same?
> >
> > On Fri, Aug 26, 2016 at 1:57 PM, Teresa Johnson <tejohnson at
google.com>
> wrote:
> >> Thanks for the test case! I can reproduce this, and see with the
> >> compiler I saved from just before r278338 that this is indeed a
chance
> >> in behavior. Looking at why this changed...
> >>
> >> On Fri, Aug 26, 2016 at 1:42 PM, Mehdi Amini <mehdi.amini at
apple.com>
> wrote:
> >>>
> >>>> On Aug 26, 2016, at 1:34 PM, junbuml at codeaurora.org
wrote:
> >>>>
> >>>> On 2016-08-26 12:47, Mehdi Amini wrote:
> >>>>>> On Aug 26, 2016, at 9:06 AM, junbuml at
codeaurora.org wrote:
> >>>>>> On 2016-08-26 11:32, Mehdi Amini wrote:
> >>>>>>> Hi,
> >>>>>>>> Recently, I noticed that less number of
global variables are
> merged in global-merge pass and in some global variable, array types are
> used instead of its original type. For example, [4xi8] with align 4 is used
> for a i32  global variable. For me, it seems that such pattern is observed
> after r278338  (Resolution-based LTO API).
> >>>>>>> Are you sure it is performed in the global
merge pass? Can you
> provide
> >>>>>>> an example of input IR where you see this now
but didn’t before?
> >>>>>>> Also can you confirm you’re using the
gold-linker?
> >>>>>> I used gold linker. In spec2006/perlbench, I
observed the less
> number of globals are merged in GlobalMerge.cpp after r278338. The reason
> is because, from the very first pass, several global variables use [4xi8]
> with align 4, instead of its original type i32 after r278338. Current
> GlobalMerge pass doesn't seem to handle such fancy-aligned globals. If
such
> type change (e.g., from i32 to [4xi8]) in global variables was intended in
> r278338, I think we should enhance GlobalMerge to handle such cases.
> >>>>> Can you submit a reproduction for Gold please?
> >>>>> We need to understand what changed with the new LTO
API.
> >>>>
> >>>>
> >>>> I compiled below C code for aarch64 in lto using gold
> (--target=aarch64-linux-gnu  -flto -fuse-ld=gold). After r278338, two
> globals, GVi32_a and GVi32_b, are [4 x i8] type in the input IR to
> GlobalMerge. Therefore, GlobalMerge do not even start to handle them
> because as of now it ignores fancy-aligned globals.  Before r278338,
> GVi32_a and GVi32_b seems to be i32 in the input IR to GlobalMerge. Is this
> change in the input IR expected ?
> >>>>
> >>>> -------------------------
> >>>> int GVi32_a  ;
> >>>> int GVi32_b  ;
> >>>
> >>> These are common variables, this is what I mentioned in my
first
> email. Compiling with -fno-commons or defining them with “int GVi32_a = 0;”
> should solve it.
> >>>
> >>> However r278338 is not supposed to have changed anything on
this
> aspect. I would have expected maybe r279417 playing a role there.
> >>>
> >>> Anyway I don’t have Gold, so I’ll leave Teresa investigate why
the
> change in behavior.
> >>>
> >>> Do you want to try improving global merge to try to handle
this case?
> >>>
> >>> —
> >>> Mehdi
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>>
> >>>> __attribute__((noinline)) void setGV(int a) {
> >>>> GVi32_a = a ;
> >>>> GVi32_b = a ;
> >>>> }
> >>>>
> >>>> __attribute__((noinline)) int loadGV() {
> >>>> return GVi32_a + GVi32_b  ;
> >>>> }
> >>>>
> >>>> int main(int argc, char *argv[]){
> >>>> setGV(argc);
> >>>> return loadGV();
> >>>> }
> >>>> -------------------------
> >>>>
> >>>>
> >>>>
> >>>>> —
> >>>>> Mehdi
> >>>>
> >>>> --
> >>>> Qualcomm Datacenter Technologies, Inc. as an affiliate of
Qualcomm
> Technologies, Inc.
> >>>> Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a
> Linux Foundation Collaborative Project.
> >>>
> >>
> >>
> >>
> >> --
> >> Teresa Johnson | Software Engineer | tejohnson at google.com |
> 408-460-2413
> >
> >
> >
> > --
> > Teresa Johnson | Software Engineer | tejohnson at google.com |
408-460-2413
>
>

-- 
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-dev/attachments/20160826/dfaa9fe2/attachment.html>

I have a fix, just running tests.

On Fri, Aug 26, 2016 at 4:03 PM, Teresa Johnson <tejohnson at google.com>
wrote:
>
>
> On Fri, Aug 26, 2016 at 3:07 PM, Mehdi Amini <mehdi.amini at
apple.com>
> wrote:
>
>>
>> > On Aug 26, 2016, at 2:06 PM, Teresa Johnson <tejohnson at
google.com>
>> wrote:
>> >
>> > Mehdi: I see what is going on:
>> >
>> > +    ArrayType *Ty >> > +       
ArrayType::get(Type::getInt8Ty(RegularLTO.Ctx),
>> I.second.Size);
>> > +    GlobalVariable *OldGV = RegularLTO.CombinedModule->get
>> NamedGlobal(I.first);
>> > +    if (OldGV && OldGV->getType()->getElementType()
== Ty) {
>> > +      // Don't create a new global if the type is already
correct,
>> just make
>> > +      // sure the alignment is correct.
>> >
>> > We compare the existing type to an ArrayType constructed from the
>> > recorded merged common size. So
>> > the original i32 type does not look the same (since it compares
>> > against [4 x i8]).
>>
>> Right, but that the code I wrote in r279417 right? I guess we should
>> compare the *size* first, and only do something if the size differs.
>>
>
> Yeah. In fact the original handling added in r278338 (addCommons)
didn't
> even compare the size, it always created a new merged common with an
> ArrayType.
>
>
>>
>> How was Gold doing it before r278338?
>>
>
> It was just letting the ModuleLinker handle it (which kept the largest one
> by size), and then it fixed up the alignment later.
>
> Will change to first compare the size.
>
>
>>
>> —
>>
>> Mehdi
>>
>>
>>
>>
>> >
>> > Not that familiar with type handling in LLVM, but I guess I need
to
>> > just check if the type size is the same?
>> >
>> > On Fri, Aug 26, 2016 at 1:57 PM, Teresa Johnson <tejohnson at
google.com>
>> wrote:
>> >> Thanks for the test case! I can reproduce this, and see with
the
>> >> compiler I saved from just before r278338 that this is indeed
a chance
>> >> in behavior. Looking at why this changed...
>> >>
>> >> On Fri, Aug 26, 2016 at 1:42 PM, Mehdi Amini <mehdi.amini
at apple.com>
>> wrote:
>> >>>
>> >>>> On Aug 26, 2016, at 1:34 PM, junbuml at codeaurora.org
wrote:
>> >>>>
>> >>>> On 2016-08-26 12:47, Mehdi Amini wrote:
>> >>>>>> On Aug 26, 2016, at 9:06 AM, junbuml at
codeaurora.org wrote:
>> >>>>>> On 2016-08-26 11:32, Mehdi Amini wrote:
>> >>>>>>> Hi,
>> >>>>>>>> Recently, I noticed that less number
of global variables are
>> merged in global-merge pass and in some global variable, array types
are
>> used instead of its original type. For example, [4xi8] with align 4 is
used
>> for a i32  global variable. For me, it seems that such pattern is
observed
>> after r278338  (Resolution-based LTO API).
>> >>>>>>> Are you sure it is performed in the global
merge pass? Can you
>> provide
>> >>>>>>> an example of input IR where you see this
now but didn’t before?
>> >>>>>>> Also can you confirm you’re using the
gold-linker?
>> >>>>>> I used gold linker. In spec2006/perlbench, I
observed the less
>> number of globals are merged in GlobalMerge.cpp after r278338. The
reason
>> is because, from the very first pass, several global variables use
[4xi8]
>> with align 4, instead of its original type i32 after r278338. Current
>> GlobalMerge pass doesn't seem to handle such fancy-aligned globals.
If such
>> type change (e.g., from i32 to [4xi8]) in global variables was intended
in
>> r278338, I think we should enhance GlobalMerge to handle such cases.
>> >>>>> Can you submit a reproduction for Gold please?
>> >>>>> We need to understand what changed with the new
LTO API.
>> >>>>
>> >>>>
>> >>>> I compiled below C code for aarch64 in lto using gold
>> (--target=aarch64-linux-gnu  -flto -fuse-ld=gold). After r278338, two
>> globals, GVi32_a and GVi32_b, are [4 x i8] type in the input IR to
>> GlobalMerge. Therefore, GlobalMerge do not even start to handle them
>> because as of now it ignores fancy-aligned globals.  Before r278338,
>> GVi32_a and GVi32_b seems to be i32 in the input IR to GlobalMerge. Is
this
>> change in the input IR expected ?
>> >>>>
>> >>>> -------------------------
>> >>>> int GVi32_a  ;
>> >>>> int GVi32_b  ;
>> >>>
>> >>> These are common variables, this is what I mentioned in my
first
>> email. Compiling with -fno-commons or defining them with “int GVi32_a =
0;”
>> should solve it.
>> >>>
>> >>> However r278338 is not supposed to have changed anything
on this
>> aspect. I would have expected maybe r279417 playing a role there.
>> >>>
>> >>> Anyway I don’t have Gold, so I’ll leave Teresa investigate
why the
>> change in behavior.
>> >>>
>> >>> Do you want to try improving global merge to try to handle
this case?
>> >>>
>> >>> —
>> >>> Mehdi
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>>
>> >>>> __attribute__((noinline)) void setGV(int a) {
>> >>>> GVi32_a = a ;
>> >>>> GVi32_b = a ;
>> >>>> }
>> >>>>
>> >>>> __attribute__((noinline)) int loadGV() {
>> >>>> return GVi32_a + GVi32_b  ;
>> >>>> }
>> >>>>
>> >>>> int main(int argc, char *argv[]){
>> >>>> setGV(argc);
>> >>>> return loadGV();
>> >>>> }
>> >>>> -------------------------
>> >>>>
>> >>>>
>> >>>>
>> >>>>> —
>> >>>>> Mehdi
>> >>>>
>> >>>> --
>> >>>> Qualcomm Datacenter Technologies, Inc. as an affiliate
of Qualcomm
>> Technologies, Inc.
>> >>>> Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a
>> Linux Foundation Collaborative Project.
>> >>>
>> >>
>> >>
>> >>
>> >> --
>> >> Teresa Johnson | Software Engineer | tejohnson at google.com |
>> 408-460-2413
>> >
>> >
>> >
>> > --
>> > Teresa Johnson | Software Engineer | tejohnson at google.com |
>> 408-460-2413
>>
>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413
>


-- 
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-dev/attachments/20160826/ce551639/attachment-0001.html>