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

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

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

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