Re: [PATCH] Fix PR tree-optimization/77550

[Christophe Lyon]

On 26 September 2016 at 22:09, Bernd Edlinger  wrote:
> On 09/26/16 21:19, Christophe Lyon wrote:
>>
>> Sorry for the delay, I've been travelling.
>>
>> Compilation is:
>> .../xg++ 
>> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/gcc/testsuite/g++/../../
>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr77550.C
>> g++_tg.o -fno-diagnostics-show-caret -fdiagnostics-color=never
>> -nostdinc++ 
>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/arm-none-eabi
>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include
>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++
>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward
>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util
>> -fmessage-length=0 -std=c++14 -O3
>> -L/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs
>> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs
>> -Wl,-wrap,exit -Wl,-wrap,_exit -Wl,-wrap,main -Wl,-wrap,abort -lm -o
>> ./pr77550.exe
>>
>> Then, during execution, I have:
>> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>> and that's all
>>
>> My gcc configuration flags are:
>> --target=arm-none-eabi --disable-libgomp --disable-libmudflap
>> --disable-libcilkrts --enable-checking --enable-languages=c,c++
>> --with-newlib --disable-tls --with-mode=arm --with-cpu=cortex-a9
>>
>> Christophe
>>
>
> Interesting.  At this point it might be OK to open a new PR.
>
OK, this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77748

> So far, I cannot reproduce, but I used also cortex-a9 but
> target=arm-linux-gnueabihf.
>
It does work for me with this configuration.
arm-non-eabi makes the difference.

> Could you please start the command above manually and add
> -save-temps ?
>
>
> Thanks
> Bernd.

Re: [PATCH] Fix PR tree-optimization/77550

[Bernd Edlinger]

On 09/26/16 21:19, Christophe Lyon wrote:
>
> Sorry for the delay, I've been travelling.
>
> Compilation is:
> .../xg++ 
> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/gcc/testsuite/g++/../../
> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr77550.C
> g++_tg.o -fno-diagnostics-show-caret -fdiagnostics-color=never
> -nostdinc++ 
> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/arm-none-eabi
> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include
> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++
> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward
> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util
> -fmessage-length=0 -std=c++14 -O3
> -L/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs
> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs
> -Wl,-wrap,exit -Wl,-wrap,_exit -Wl,-wrap,main -Wl,-wrap,abort -lm -o
> ./pr77550.exe
>
> Then, during execution, I have:
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> and that's all
>
> My gcc configuration flags are:
> --target=arm-none-eabi --disable-libgomp --disable-libmudflap
> --disable-libcilkrts --enable-checking --enable-languages=c,c++
> --with-newlib --disable-tls --with-mode=arm --with-cpu=cortex-a9
>
> Christophe
>

Interesting.  At this point it might be OK to open a new PR.

So far, I cannot reproduce, but I used also cortex-a9 but
target=arm-linux-gnueabihf.

Could you please start the command above manually and add
-save-temps ?


Thanks
Bernd.

Re: [PATCH] Fix PR tree-optimization/77550

[Christophe Lyon]

On 22 September 2016 at 22:30, Bernd Edlinger  wrote:
> On 09/22/16 21:30, Christophe Lyon wrote:
>> On 21 September 2016 at 22:20, Bernd Edlinger  
>> wrote:
>>> On 09/21/16 21:57, Christophe Lyon wrote:
 Hi,

 The new testcase pr77550.C fails on arm:
 /testsuite/g++.dg/pr77550.C:39:43: error: 'operator new' takes type
 'size_t' ('unsigned int') as first parameter [-fpermissive]
 compiler exited with status 1

 Christophe

>>>
>>> Yes, I see, thanks.  This should fix it.
>>>
>>> OK for trunk, gcc-6 after a while ?
>>>
>>>
>>> Bernd.
>>
>> Hi,
>>
>> The test now compiles and runs OK on most arm configurations, but
>> on arm-none-eabi --with-cpu=cortex-a9, execution fails:
>> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>> FAIL: g++.dg/pr77550.C   execution test
>>
>> And on arm-none-eabi with default cpu, the test still does not compile.
>>
>> Christophe
>>
>
> Hmm.  This test case is hardly worth it, because on my armv7hf target
> it doesn't even reproduce the original problem :(
>
> Could you send me the exact compile-flags and the error message
> please?
>

Sorry for the delay, I've been travelling.

Compilation is:
.../xg++ 
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/gcc/testsuite/g++/../../
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr77550.C
g++_tg.o -fno-diagnostics-show-caret -fdiagnostics-color=never
-nostdinc++ 
-I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include/arm-none-eabi
-I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/include
-I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++
-I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward
-I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util
-fmessage-length=0 -std=c++14 -O3
-L/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-eabi/gcc3/arm-none-eabi/./libstdc++-v3/src/.libs
-Wl,-wrap,exit -Wl,-wrap,_exit -Wl,-wrap,main -Wl,-wrap,abort -lm -o
./pr77550.exe

Then, during execution, I have:
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
and that's all

My gcc configuration flags are:
--target=arm-none-eabi --disable-libgomp --disable-libmudflap
--disable-libcilkrts --enable-checking --enable-languages=c,c++
--with-newlib --disable-tls --with-mode=arm --with-cpu=cortex-a9

Christophe


>
> Bernd.

Re: [PATCH] Fix PR tree-optimization/77550

[Bernd Edlinger]

On 09/22/16 21:30, Christophe Lyon wrote:
> On 21 September 2016 at 22:20, Bernd Edlinger  
> wrote:
>> On 09/21/16 21:57, Christophe Lyon wrote:
>>> Hi,
>>>
>>> The new testcase pr77550.C fails on arm:
>>> /testsuite/g++.dg/pr77550.C:39:43: error: 'operator new' takes type
>>> 'size_t' ('unsigned int') as first parameter [-fpermissive]
>>> compiler exited with status 1
>>>
>>> Christophe
>>>
>>
>> Yes, I see, thanks.  This should fix it.
>>
>> OK for trunk, gcc-6 after a while ?
>>
>>
>> Bernd.
>
> Hi,
>
> The test now compiles and runs OK on most arm configurations, but
> on arm-none-eabi --with-cpu=cortex-a9, execution fails:
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> FAIL: g++.dg/pr77550.C   execution test
>
> And on arm-none-eabi with default cpu, the test still does not compile.
>
> Christophe
>

Hmm.  This test case is hardly worth it, because on my armv7hf target
it doesn't even reproduce the original problem :(

Could you send me the exact compile-flags and the error message
please?


Bernd.

Re: [PATCH] Fix PR tree-optimization/77550

[Christophe Lyon]

On 21 September 2016 at 22:20, Bernd Edlinger  wrote:
> On 09/21/16 21:57, Christophe Lyon wrote:
>> Hi,
>>
>> The new testcase pr77550.C fails on arm:
>> /testsuite/g++.dg/pr77550.C:39:43: error: 'operator new' takes type
>> 'size_t' ('unsigned int') as first parameter [-fpermissive]
>> compiler exited with status 1
>>
>> Christophe
>>
>
> Yes, I see, thanks.  This should fix it.
>
> OK for trunk, gcc-6 after a while ?
>
>
> Bernd.

Hi,

The test now compiles and runs OK on most arm configurations, but
on arm-none-eabi --with-cpu=cortex-a9, execution fails:
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
FAIL: g++.dg/pr77550.C   execution test

And on arm-none-eabi with default cpu, the test still does not compile.

Christophe

Re: [PATCH] Fix PR tree-optimization/77550

[Richard Biener]

On Wed, 21 Sep 2016, Bernd Edlinger wrote:

> On 09/21/16 21:57, Christophe Lyon wrote:
> > Hi,
> >
> > The new testcase pr77550.C fails on arm:
> > /testsuite/g++.dg/pr77550.C:39:43: error: 'operator new' takes type
> > 'size_t' ('unsigned int') as first parameter [-fpermissive]
> > compiler exited with status 1
> >
> > Christophe
> >
> 
> Yes, I see, thanks.  This should fix it.
> 
> OK for trunk, gcc-6 after a while ?

Ok.

Richard.

Re: [PATCH] Fix PR tree-optimization/77550

[Bernd Edlinger]

On 09/21/16 21:57, Christophe Lyon wrote:
> Hi,
>
> The new testcase pr77550.C fails on arm:
> /testsuite/g++.dg/pr77550.C:39:43: error: 'operator new' takes type
> 'size_t' ('unsigned int') as first parameter [-fpermissive]
> compiler exited with status 1
>
> Christophe
>

Yes, I see, thanks.  This should fix it.

OK for trunk, gcc-6 after a while ?


Bernd.
2016-09-21  Bernd Edlinger  

	* g++.dg/pr77550.C: Use __SIZE_TYPE__.

Index: gcc/testsuite/g++.dg/pr77550.C
===
--- gcc/testsuite/g++.dg/pr77550.C	(revision 240330)
+++ gcc/testsuite/g++.dg/pr77550.C	(working copy)
@@ -36,7 +36,7 @@ struct B {
 template  using __ptr_rebind = B;
 template  _Tp max(_Tp p1, _Tp) { return p1; }
 }
-void *operator new(unsigned long, void *p2) { return p2; }
+void *operator new(__SIZE_TYPE__, void *p2) { return p2; }
 template  struct C {
   typedef _Tp *pointer;
   pointer allocate(int p1) {
@@ -47,7 +47,7 @@ template  struct C {
 namespace std {
 template  using __allocator_base = C<_Tp>;
 template  struct allocator : __allocator_base<_Tp> {
-  typedef unsigned long size_type;
+  typedef __SIZE_TYPE__ size_type;
   template  struct rebind { typedef allocator<_Tp1> other; };
 };
 struct D {

Re: [PATCH] Fix PR tree-optimization/77550

[Christophe Lyon]

On 21 September 2016 at 09:16, Richard Biener  wrote:
> On Tue, 20 Sep 2016, Bernd Edlinger wrote:
>
>> On 09/20/16 09:41, Richard Biener wrote:
>> > On Mon, 19 Sep 2016, Bernd Edlinger wrote:
>> >
>> >> On 09/19/16 11:25, Richard Biener wrote:
>> >>> On Sun, 18 Sep 2016, Bernd Edlinger wrote:
>> >>>
>>  Hi,
>> 
>>  this PR shows that in vectorizable_store and vectorizable_load
>>  as well, the vector access always uses the first dr as the alias
>>  type for the whole access.  But that is not right, if they are
>>  different types, like in this example.
>> 
>>  So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
>>  by an alias type that is correct for all references in the whole
>>  access group.  With this patch we fall back to ptr_type_node, which
>>  can alias anything, if the group consists of different alias sets.
>> 
>> 
>>  Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>  Is it OK for trunk and gcc-6-branch?
>> >>>
>> >>> +/* Function get_group_alias_ptr_type.
>> >>> +
>> >>> +   Return the alias type for the group starting at FIRST_STMT
>> >>> +   containing GROUP_SIZE elements.  */
>> >>> +
>> >>> +static tree
>> >>> +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
>> >>> +{
>> >>> +  struct data_reference *first_dr, *next_dr;
>> >>> +  gimple *next_stmt;
>> >>> +  int i;
>> >>> +
>> >>> +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
>> >>> +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
>> >>> +  for (i = 1; i < group_size && next_stmt; i++)
>> >>> +{
>> >>>
>> >>>
>> >>> there is no need to pass in group_size, it's enough to walk
>> >>> GROUP_NEXT_ELEMENT until it becomes NULL.
>> >>>
>> >>> Ok with removing the redundant arg.
>> >>>
>> >>> Thanks,
>> >>> Richard.
>> >>>
>> >>
>> >> Hmmm, I'm afraid this needs one more iteration.
>> >>
>> >>
>> >> I tried first to check if there are no stmts after the group_size
>> >> and noticed there are cases when group_size is 0,
>> >> for instance in gcc.dg/torture/pr36244.c.
>> >>
>> >> I think there is a bug in vectorizable_load, here:
>> >>
>> >>if (grouped_load)
>> >>  {
>> >>first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
>> >> /* For SLP vectorization we directly vectorize a subchain
>> >>without permutation.  */
>> >> if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
>> >>   first_stmt = ;
>> >>
>> >> group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
>> >>
>> >> = 0, and even worse:
>> >>
>> >> group_gap_adj = vf * group_size - nunits * vec_num;
>> >>
>> >> = -4 !
>> >>
>> >> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,
>> >
>> > Yes.  I'm not sure group_size or group_gap_adj are used in the
>> > slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
>> > the computation up before we re-set first_stmt is probably a good idea.
>> >
>> >> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
>> >>
>> >> moving the GROUP_SIZE up before first_stmt is overwritten
>> >> results in no different code
>> >
>> > See above - it's eventually unused.  The load/store vectorization code
>> > is quite twisted ;)
>> >
>>
>> Agreed.
>>
>> Here is the new version of the patch:
>>
>> Moved the goups_size up, and everything works fine.
>>
>> Removed the parameter from get_group_alias_ptr_type.
>>
>> I think in the case, where first_stmt is not set to
>> GROUP_FIRST_ELEMENT (stmt_info) but directly to stmt,
>> it is likely somewhere in a list, so it is not necessary
>> to walk the GROUP_NEXT_ELEMENT, so I would like to call
>> reference_alias_ptr_type directly in that case.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk and gcc-6 branch?
>
> Ok for trunk and gcc-6 branch after a while.
>
> Richard.


Hi,

The new testcase pr77550.C fails on arm:
/testsuite/g++.dg/pr77550.C:39:43: error: 'operator new' takes type
'size_t' ('unsigned int') as first parameter [-fpermissive]
compiler exited with status 1

Christophe

Re: [PATCH] Fix PR tree-optimization/77550

[Richard Biener]

On Tue, 20 Sep 2016, Bernd Edlinger wrote:

> On 09/20/16 09:41, Richard Biener wrote:
> > On Mon, 19 Sep 2016, Bernd Edlinger wrote:
> >
> >> On 09/19/16 11:25, Richard Biener wrote:
> >>> On Sun, 18 Sep 2016, Bernd Edlinger wrote:
> >>>
>  Hi,
> 
>  this PR shows that in vectorizable_store and vectorizable_load
>  as well, the vector access always uses the first dr as the alias
>  type for the whole access.  But that is not right, if they are
>  different types, like in this example.
> 
>  So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
>  by an alias type that is correct for all references in the whole
>  access group.  With this patch we fall back to ptr_type_node, which
>  can alias anything, if the group consists of different alias sets.
> 
> 
>  Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>  Is it OK for trunk and gcc-6-branch?
> >>>
> >>> +/* Function get_group_alias_ptr_type.
> >>> +
> >>> +   Return the alias type for the group starting at FIRST_STMT
> >>> +   containing GROUP_SIZE elements.  */
> >>> +
> >>> +static tree
> >>> +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
> >>> +{
> >>> +  struct data_reference *first_dr, *next_dr;
> >>> +  gimple *next_stmt;
> >>> +  int i;
> >>> +
> >>> +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
> >>> +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
> >>> +  for (i = 1; i < group_size && next_stmt; i++)
> >>> +{
> >>>
> >>>
> >>> there is no need to pass in group_size, it's enough to walk
> >>> GROUP_NEXT_ELEMENT until it becomes NULL.
> >>>
> >>> Ok with removing the redundant arg.
> >>>
> >>> Thanks,
> >>> Richard.
> >>>
> >>
> >> Hmmm, I'm afraid this needs one more iteration.
> >>
> >>
> >> I tried first to check if there are no stmts after the group_size
> >> and noticed there are cases when group_size is 0,
> >> for instance in gcc.dg/torture/pr36244.c.
> >>
> >> I think there is a bug in vectorizable_load, here:
> >>
> >>if (grouped_load)
> >>  {
> >>first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
> >> /* For SLP vectorization we directly vectorize a subchain
> >>without permutation.  */
> >> if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
> >>   first_stmt = ;
> >>
> >> group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
> >>
> >> = 0, and even worse:
> >>
> >> group_gap_adj = vf * group_size - nunits * vec_num;
> >>
> >> = -4 !
> >>
> >> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,
> >
> > Yes.  I'm not sure group_size or group_gap_adj are used in the
> > slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
> > the computation up before we re-set first_stmt is probably a good idea.
> >
> >> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
> >>
> >> moving the GROUP_SIZE up before first_stmt is overwritten
> >> results in no different code
> >
> > See above - it's eventually unused.  The load/store vectorization code
> > is quite twisted ;)
> >
> 
> Agreed.
> 
> Here is the new version of the patch:
> 
> Moved the goups_size up, and everything works fine.
> 
> Removed the parameter from get_group_alias_ptr_type.
> 
> I think in the case, where first_stmt is not set to
> GROUP_FIRST_ELEMENT (stmt_info) but directly to stmt,
> it is likely somewhere in a list, so it is not necessary
> to walk the GROUP_NEXT_ELEMENT, so I would like to call
> reference_alias_ptr_type directly in that case.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk and gcc-6 branch?

Ok for trunk and gcc-6 branch after a while.

Richard.

Re: [PATCH] Fix PR tree-optimization/77550

[Bernd Edlinger]

On 09/20/16 09:41, Richard Biener wrote:
> On Mon, 19 Sep 2016, Bernd Edlinger wrote:
>
>> On 09/19/16 11:25, Richard Biener wrote:
>>> On Sun, 18 Sep 2016, Bernd Edlinger wrote:
>>>
 Hi,

 this PR shows that in vectorizable_store and vectorizable_load
 as well, the vector access always uses the first dr as the alias
 type for the whole access.  But that is not right, if they are
 different types, like in this example.

 So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
 by an alias type that is correct for all references in the whole
 access group.  With this patch we fall back to ptr_type_node, which
 can alias anything, if the group consists of different alias sets.


 Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
 Is it OK for trunk and gcc-6-branch?
>>>
>>> +/* Function get_group_alias_ptr_type.
>>> +
>>> +   Return the alias type for the group starting at FIRST_STMT
>>> +   containing GROUP_SIZE elements.  */
>>> +
>>> +static tree
>>> +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
>>> +{
>>> +  struct data_reference *first_dr, *next_dr;
>>> +  gimple *next_stmt;
>>> +  int i;
>>> +
>>> +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
>>> +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
>>> +  for (i = 1; i < group_size && next_stmt; i++)
>>> +{
>>>
>>>
>>> there is no need to pass in group_size, it's enough to walk
>>> GROUP_NEXT_ELEMENT until it becomes NULL.
>>>
>>> Ok with removing the redundant arg.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>> Hmmm, I'm afraid this needs one more iteration.
>>
>>
>> I tried first to check if there are no stmts after the group_size
>> and noticed there are cases when group_size is 0,
>> for instance in gcc.dg/torture/pr36244.c.
>>
>> I think there is a bug in vectorizable_load, here:
>>
>>if (grouped_load)
>>  {
>>first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
>> /* For SLP vectorization we directly vectorize a subchain
>>without permutation.  */
>> if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
>>   first_stmt = ;
>>
>> group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
>>
>> = 0, and even worse:
>>
>> group_gap_adj = vf * group_size - nunits * vec_num;
>>
>> = -4 !
>>
>> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,
>
> Yes.  I'm not sure group_size or group_gap_adj are used in the
> slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
> the computation up before we re-set first_stmt is probably a good idea.
>
>> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
>>
>> moving the GROUP_SIZE up before first_stmt is overwritten
>> results in no different code
>
> See above - it's eventually unused.  The load/store vectorization code
> is quite twisted ;)
>

Agreed.

Here is the new version of the patch:

Moved the goups_size up, and everything works fine.

Removed the parameter from get_group_alias_ptr_type.

I think in the case, where first_stmt is not set to
GROUP_FIRST_ELEMENT (stmt_info) but directly to stmt,
it is likely somewhere in a list, so it is not necessary
to walk the GROUP_NEXT_ELEMENT, so I would like to call
reference_alias_ptr_type directly in that case.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk and gcc-6 branch?


Thanks
Bernd.
gcc:
2016-09-18  Bernd Edlinger  

PR tree-optimization/77550
* tree-vect-stmts.c (create_array_ref): Change parameters.
(get_group_alias_ptr_type): New function.
(vectorizable_store, vectorizable_load): Use get_group_alias_ptr_type.

testsuite:
2016-09-18  Bernd Edlinger  

PR tree-optimization/77550
* g++.dg/pr77550.C: New test.
Index: gcc/tree-vect-stmts.c
===
--- gcc/tree-vect-stmts.c	(revision 240251)
+++ gcc/tree-vect-stmts.c	(working copy)
@@ -170,11 +170,10 @@ write_vector_array (gimple *stmt, gimple_stmt_iter
(and its group).  */
 
 static tree
-create_array_ref (tree type, tree ptr, struct data_reference *first_dr)
+create_array_ref (tree type, tree ptr, tree alias_ptr_type)
 {
-  tree mem_ref, alias_ptr_type;
+  tree mem_ref;
 
-  alias_ptr_type = reference_alias_ptr_type (DR_REF (first_dr));
   mem_ref = build2 (MEM_REF, type, ptr, build_int_cst (alias_ptr_type, 0));
   /* Arrays have the same alignment as their type.  */
   set_ptr_info_alignment (get_ptr_info (ptr), TYPE_ALIGN_UNIT (type), 0);
@@ -5432,6 +5431,35 @@ ensure_base_align (stmt_vec_info stmt_info, struct
 }
 
 
+/* Function get_group_alias_ptr_type.
+
+   Return the alias type for the group starting at FIRST_STMT.  */
+
+static tree
+get_group_alias_ptr_type (gimple *first_stmt)
+{
+  struct data_reference *first_dr, *next_dr;
+  gimple *next_stmt;
+
+  first_dr = 

Re: [PATCH] Fix PR tree-optimization/77550

[Richard Biener]

On Mon, 19 Sep 2016, Bernd Edlinger wrote:

> On 09/19/16 11:25, Richard Biener wrote:
> > On Sun, 18 Sep 2016, Bernd Edlinger wrote:
> >
> >> Hi,
> >>
> >> this PR shows that in vectorizable_store and vectorizable_load
> >> as well, the vector access always uses the first dr as the alias
> >> type for the whole access.  But that is not right, if they are
> >> different types, like in this example.
> >>
> >> So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
> >> by an alias type that is correct for all references in the whole
> >> access group.  With this patch we fall back to ptr_type_node, which
> >> can alias anything, if the group consists of different alias sets.
> >>
> >>
> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk and gcc-6-branch?
> >
> > +/* Function get_group_alias_ptr_type.
> > +
> > +   Return the alias type for the group starting at FIRST_STMT
> > +   containing GROUP_SIZE elements.  */
> > +
> > +static tree
> > +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
> > +{
> > +  struct data_reference *first_dr, *next_dr;
> > +  gimple *next_stmt;
> > +  int i;
> > +
> > +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
> > +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
> > +  for (i = 1; i < group_size && next_stmt; i++)
> > +{
> >
> >
> > there is no need to pass in group_size, it's enough to walk
> > GROUP_NEXT_ELEMENT until it becomes NULL.
> >
> > Ok with removing the redundant arg.
> >
> > Thanks,
> > Richard.
> >
> 
> Hmmm, I'm afraid this needs one more iteration.
> 
> 
> I tried first to check if there are no stmts after the group_size
> and noticed there are cases when group_size is 0,
> for instance in gcc.dg/torture/pr36244.c.
> 
> I think there is a bug in vectorizable_load, here:
> 
>   if (grouped_load)
> {
>   first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
>/* For SLP vectorization we directly vectorize a subchain
>   without permutation.  */
>if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
>  first_stmt = ;
> 
>group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
> 
>= 0, and even worse:
> 
>group_gap_adj = vf * group_size - nunits * vec_num;
> 
>= -4 !
> 
> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,

Yes.  I'm not sure group_size or group_gap_adj are used in the
slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
the computation up before we re-set first_stmt is probably a good idea.

> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
> 
> moving the GROUP_SIZE up before first_stmt is overwritten
> results in no different code

See above - it's eventually unused.  The load/store vectorization code
is quite twisted ;)

Re: [PATCH] Fix PR tree-optimization/77550

[Bernd Edlinger]

On 09/19/16 11:25, Richard Biener wrote:
> On Sun, 18 Sep 2016, Bernd Edlinger wrote:
>
>> Hi,
>>
>> this PR shows that in vectorizable_store and vectorizable_load
>> as well, the vector access always uses the first dr as the alias
>> type for the whole access.  But that is not right, if they are
>> different types, like in this example.
>>
>> So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
>> by an alias type that is correct for all references in the whole
>> access group.  With this patch we fall back to ptr_type_node, which
>> can alias anything, if the group consists of different alias sets.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk and gcc-6-branch?
>
> +/* Function get_group_alias_ptr_type.
> +
> +   Return the alias type for the group starting at FIRST_STMT
> +   containing GROUP_SIZE elements.  */
> +
> +static tree
> +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
> +{
> +  struct data_reference *first_dr, *next_dr;
> +  gimple *next_stmt;
> +  int i;
> +
> +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
> +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
> +  for (i = 1; i < group_size && next_stmt; i++)
> +{
>
>
> there is no need to pass in group_size, it's enough to walk
> GROUP_NEXT_ELEMENT until it becomes NULL.
>
> Ok with removing the redundant arg.
>
> Thanks,
> Richard.
>

Hmmm, I'm afraid this needs one more iteration.


I tried first to check if there are no stmts after the group_size
and noticed there are cases when group_size is 0,
for instance in gcc.dg/torture/pr36244.c.

I think there is a bug in vectorizable_load, here:

  if (grouped_load)
{
  first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
   /* For SLP vectorization we directly vectorize a subchain
  without permutation.  */
   if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
 first_stmt = ;

   group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));

   = 0, and even worse:

   group_gap_adj = vf * group_size - nunits * vec_num;

   = -4 !

apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,
while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]

moving the GROUP_SIZE up before first_stmt is overwritten
results in no different code


Bernd.

Re: [PATCH] Fix PR tree-optimization/77550

[Richard Biener]

On Sun, 18 Sep 2016, Bernd Edlinger wrote:

> Hi,
> 
> this PR shows that in vectorizable_store and vectorizable_load
> as well, the vector access always uses the first dr as the alias
> type for the whole access.  But that is not right, if they are
> different types, like in this example.
> 
> So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
> by an alias type that is correct for all references in the whole
> access group.  With this patch we fall back to ptr_type_node, which
> can alias anything, if the group consists of different alias sets.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk and gcc-6-branch?

+/* Function get_group_alias_ptr_type.
+
+   Return the alias type for the group starting at FIRST_STMT
+   containing GROUP_SIZE elements.  */
+
+static tree
+get_group_alias_ptr_type (gimple *first_stmt, int group_size)
+{
+  struct data_reference *first_dr, *next_dr;
+  gimple *next_stmt;
+  int i;
+
+  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
+  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
+  for (i = 1; i < group_size && next_stmt; i++)
+{


there is no need to pass in group_size, it's enough to walk
GROUP_NEXT_ELEMENT until it becomes NULL.

Ok with removing the redundant arg.

Thanks,
Richard.