Bug 71237 - [7 regression] scev tests failing after pass reorganization
Summary: [7 regression] scev tests failing after pass reorganization
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 7.0
: P1 normal
Target Milestone: 7.0
Assignee: Richard Biener
URL:
Keywords:
: 78961 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-05-23 13:42 UTC by Andre Vieira
Modified: 2019-12-20 23:04 UTC (History)
4 users (show)

See Also:
Host:
Target: arm-none-eabi, sparc*-*-solaris2.* hppa64-hp-hpux11.11
Build:
Known to work: 6.0
Known to fail: 7.0
Last reconfirmed: 2016-10-28 00:00:00


Attachments
Assembly with the changed passes.def removing one pass of lim (330 bytes, text/plain)
2016-05-26 17:42 UTC, Andre Vieira
Details
Regular generation at -O2 (321 bytes, text/plain)
2016-05-26 17:43 UTC, Andre Vieira
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Vieira 2016-05-23 13:42:40 UTC
Ever since the reorganization of the passes moving lim before sink makes these tests fail.

FAIL: gcc.dg/tree-ssa/scev-3.c scan-tree-dump-times optimized "&a" 1
FAIL: gcc.dg/tree-ssa/scev-4.c scan-tree-dump-times optimized "&a" 1
FAIL: gcc.dg/tree-ssa/scev-5.c scan-tree-dump-times optimized "&a" 1

This was first reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52563#c11
for sparc*-*-solaris2.*, it also fails on arm-none-eabi.

Some further discussions on that thread.

Quoting Richard:

rguenther@suse.de 2016-05-23 07:40:31 UTC

>On Fri, 20 May 2016, andre.simoesdiasvieira at arm dot com wrote:
> 
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52563
>> 
>> --- Comment #17 from Andre Vieira <andre.simoesdiasvieira at arm dot com> ---
>> Ah yes my bad, its not sccp doing it... got a bit confused there... It is
>> indeed sink that moves that sequence down. Sorry for the noise.
>> 
>> Question remains on how to clean this up though. Ideally you would like to >end
>> up with
>> 
>> a_p = <last_computed_base>;
>> outside of the loop.
> 
>First of all please open a new bug for the FAILs.  Second, the fix will
>be mostly adjusting the testcase expectations (eventually disabling LIM
>for example if we want to test SCCP abilities).
> 
>As to your question it techincally is a job of CSE though it may be a
>tough job to make it figure out the equivalence.
> 
>Now, in the case of scev-5.c (the only regression I see on x86_64, with
>-m32), SCCP fails to do final value replacement for i_24:
> 
>  <bb 4>:
>  # i_12 = PHI <i_10(3), i_9(5)>
>  MEM[(int *)&a][i_12] = 100;
>  i_9 = i_5 + i_12;
>  if (i_9 <= 999)
>    goto <bb 5>;
>  else
>    goto <bb 6>;
> 
>  <bb 5>:
>  goto <bb 4>;
> 
>  <bb 6>:
>  # i_24 = PHI <i_12(4)>
>  _2 = (sizetype) i_24;
>  _3 = _2 * 4;
>  _1 = &a + _3;
>  a_p = _1;
> 
>which may or may not be a good thing - this way IVOPTs can see the
>extra use of i_12 on the loop exit and it _could_ look for derived
>uses of that so it _may_ be able to replace the use of i_24 with
>something better.
Comment 1 Andre Vieira 2016-05-26 17:40:35 UTC
So yes disabling LIM will make the tests "PASS". Though I couldnt find an option to do this, I disabled the pass by changing passes.def, so that doesnt sound like a good idea to test SCCP. 

However, it might be good to point out that at least for arm-none-eabi and x86_64-pc-linux-gnu these tests are no longer testing SCCP, SCCP will not change this code. I looked at the dumps and compared assembly of -O2 with and without '-fno-tree-scev-cprop'.

On arm-none-eabi, it used to be IVOPTS that made the test pass, it would reuse the same ivtmp for computing the address used by the memory dereference and the a_p assignment. Now due to the reordering of LIM, it will no longer do this.

On x86_64 I see the following code coming out of the OPTIMIZED dump for the scev-4.c case:

...
  <bb 4>:
  # ivtmp.10_14 = PHI <_24(3), ivtmp.10_25(4)>
  i_11 = (int) ivtmp.10_14;
  MEM[symbol: a, index: ivtmp.10_14, step: 8, offset: 4B] = 100;
  ivtmp.10_25 = ivtmp.10_14 + _24;
  i_22 = (int) ivtmp.10_25;
  if (i_22 <= 999)
    goto <bb 4>;
  else
    goto <bb 5>;

  <bb 5>:
  _2 = (sizetype) i_11;
  _3 = _2 * 8;
  _10 = _3 + 4;
  _1 = &a + _10;
  a_p = _1;
...

Now yes the scan-times &a will pass, but thats because the MEM is using symbol:a instead of base: &a. Not sure this can be qualified as a proper PASS.
Disabling LIM here the same way I did before, that is removing the pass_lim after pass_laddress and before pass_split_crit_edges generates the following OPTIMIZED dump:

...
  <bb 4>:
  _16 = (sizetype) k_4(D);
  _15 = _16 * 8;
  _21 = _15 + 4;
  _22 = &a + _21;
  ivtmp.9_14 = (unsigned long) _22;

  <bb 5>:
  # i_11 = PHI <k_4(D)(4), i_8(5)>
  # ivtmp.9_13 = PHI <ivtmp.9_14(4), ivtmp.9_17(5)>
  _1 = (int *) ivtmp.9_13;
  MEM[base: _1, offset: 0B] = 100;
  i_8 = k_4(D) + i_11;
  ivtmp.9_17 = ivtmp.9_13 + _15;
  if (i_8 <= 999)
    goto <bb 5>;
  else
    goto <bb 6>;

  <bb 6>:
  a_p = _1;
...

I prefer this output, since you loose the needless tailing address calculation. I am not so sure the eventually generated assembly is better in this case though. Ill add both as attachments.
Comment 2 Andre Vieira 2016-05-26 17:42:21 UTC
Created attachment 38575 [details]
Assembly with the changed passes.def removing one pass of lim
Comment 3 Andre Vieira 2016-05-26 17:43:18 UTC
Created attachment 38576 [details]
Regular generation at -O2
Comment 4 rguenther@suse.de 2016-05-27 10:19:48 UTC
On Thu, 26 May 2016, andre.simoesdiasvieira at arm dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71237
> 
> --- Comment #1 from Andre Vieira <andre.simoesdiasvieira at arm dot com> ---
> So yes disabling LIM will make the tests "PASS". Though I couldnt find an
> option to do this, I disabled the pass by changing passes.def, so that doesnt
> sound like a good idea to test SCCP. 

-fno-tree-loop-im

> However, it might be good to point out that at least for arm-none-eabi and
> x86_64-pc-linux-gnu these tests are no longer testing SCCP, SCCP will not
> change this code. I looked at the dumps and compared assembly of -O2 with and
> without '-fno-tree-scev-cprop'.

Yeah, so we're likely looking at bit-rotten SCCP tests ... :/

> On arm-none-eabi, it used to be IVOPTS that made the test pass, it would 
> reuse the same ivtmp for computing the address used by the memory 
> dereference and the a_p assignment. Now due to the reordering of LIM, it 
> will no longer do this.
> 
> On x86_64 I see the following code coming out of the OPTIMIZED dump for the
> scev-4.c case:
> 
> ...
>   <bb 4>:
>   # ivtmp.10_14 = PHI <_24(3), ivtmp.10_25(4)>
>   i_11 = (int) ivtmp.10_14;
>   MEM[symbol: a, index: ivtmp.10_14, step: 8, offset: 4B] = 100;
>   ivtmp.10_25 = ivtmp.10_14 + _24;
>   i_22 = (int) ivtmp.10_25;
>   if (i_22 <= 999)
>     goto <bb 4>;
>   else
>     goto <bb 5>;
> 
>   <bb 5>:
>   _2 = (sizetype) i_11;
>   _3 = _2 * 8;
>   _10 = _3 + 4;
>   _1 = &a + _10;
>   a_p = _1;
> ...
> 
> Now yes the scan-times &a will pass, but thats because the MEM is using
> symbol:a instead of base: &a. Not sure this can be qualified as a proper PASS.
> Disabling LIM here the same way I did before, that is removing the pass_lim
> after pass_laddress and before pass_split_crit_edges generates the following
> OPTIMIZED dump:
> 
> ...
>   <bb 4>:
>   _16 = (sizetype) k_4(D);
>   _15 = _16 * 8;
>   _21 = _15 + 4;
>   _22 = &a + _21;
>   ivtmp.9_14 = (unsigned long) _22;
> 
>   <bb 5>:
>   # i_11 = PHI <k_4(D)(4), i_8(5)>
>   # ivtmp.9_13 = PHI <ivtmp.9_14(4), ivtmp.9_17(5)>
>   _1 = (int *) ivtmp.9_13;
>   MEM[base: _1, offset: 0B] = 100;
>   i_8 = k_4(D) + i_11;
>   ivtmp.9_17 = ivtmp.9_13 + _15;
>   if (i_8 <= 999)
>     goto <bb 5>;
>   else
>     goto <bb 6>;
> 
>   <bb 6>:
>   a_p = _1;
> ...
> 
> I prefer this output, since you loose the needless tailing address 
> calculation. I am not so sure the eventually generated assembly is 
> better in this case though. Ill add both as attachments.

Yes, in a more complicated loop with more register pressure the
new variant could be better.

That said, it would be interesting to see what the testcase originally
was added for and if we can massage it to test that again.
Comment 5 Eric Botcazou 2016-10-28 09:01:48 UTC
Still present on SPARC/Solaris.
Comment 6 Eric Botcazou 2016-12-20 08:46:24 UTC
Author: ebotcazou
Date: Tue Dec 20 08:45:52 2016
New Revision: 243818

URL: https://gcc.gnu.org/viewcvs?rev=243818&root=gcc&view=rev
Log:
	PR testsuite/71237
	* gnat.dg/vect1.adb: Add -fno-vect-cost-model to dg-options.
	* gnat.dg/vect2.adb: Likewise.
	* gnat.dg/vect3.adb: Likewise.
	* gnat.dg/vect4.adb: Likewise.
	* gnat.dg/vect5.adb: Likewise.
	* gnat.dg/vect6.adb: Likewise.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gnat.dg/vect1.adb
    trunk/gcc/testsuite/gnat.dg/vect2.adb
    trunk/gcc/testsuite/gnat.dg/vect3.adb
    trunk/gcc/testsuite/gnat.dg/vect4.adb
    trunk/gcc/testsuite/gnat.dg/vect5.adb
    trunk/gcc/testsuite/gnat.dg/vect6.adb
Comment 7 Richard Biener 2017-01-04 12:54:40 UTC
*** Bug 78961 has been marked as a duplicate of this bug. ***
Comment 8 sandra 2017-01-11 21:28:18 UTC
This is still failing on nios2-linux-gnu.
Comment 9 Richard Biener 2017-01-12 14:58:58 UTC
Ok, I'll look at it and possibly will make GIMPLE testcases out of them (looking at the GIMPLE at the point of testcase introduction).
Comment 10 Richard Biener 2017-01-17 08:25:05 UTC
Author: rguenth
Date: Tue Jan 17 08:24:26 2017
New Revision: 244519

URL: https://gcc.gnu.org/viewcvs?rev=244519&root=gcc&view=rev
Log:
2017-01-17  Richard Biener  <rguenther@suse.de>

	PR testsuite/52563
	PR testsuite/71237
	PR testsuite/77737
	* gcc.dg/tree-ssa/scev-3.c: Re-write to a GIMPLE testcase for IVOPTs.
	* gcc.dg/tree-ssa/scev-4.c: Likewise.
	* gcc.dg/tree-ssa/scev-5.c: Likewise.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/tree-ssa/scev-3.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/scev-4.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/scev-5.c
Comment 11 Richard Biener 2017-01-17 08:25:20 UTC
Fixed.
Comment 12 Thomas Koenig 2019-02-10 15:53:10 UTC
Author: tkoenig
Date: Sun Feb 10 15:52:38 2019
New Revision: 268748

URL: https://gcc.gnu.org/viewcvs?rev=268748&root=gcc&view=rev
Log:
2019-02-10  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/71237
	* expr.c (gfc_check_assign): Add argument is_init_expr.  If we are
	looking at an init expression, issue error if the target is not a
	TARGET and we are not looking at a procedure pointer.
	* gfortran.h (gfc_check_assign): Add optional argument
	is_init_expr.

2019-02-10  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/71237
	* gfortran.dg/pointer_init_2.f90: Adjust error messages.
	* gfortran.dg/pointer_init_6.f90: Likewise.
	* gfortran.dg/pointer_init_9.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/pointer_init_9.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/expr.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/pointer_init_2.f90
    trunk/gcc/testsuite/gfortran.dg/pointer_init_6.f90
Comment 13 Thomas Koenig 2019-02-10 18:30:33 UTC
Author: tkoenig
Date: Sun Feb 10 18:30:01 2019
New Revision: 268750

URL: https://gcc.gnu.org/viewcvs?rev=268750&root=gcc&view=rev
Log:
2019-02-10  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/71237
	Backport from trunk
	* expr.c (gfc_check_assign): Add argument is_init_expr.  If we are
	looking at an init expression, issue error if the target is not a
	TARGET and we are not looking at a procedure pointer.
	* gfortran.h (gfc_check_assign): Add optional argument
	is_init_expr.

2019-02-10  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/71237
	Backport from trunk
	* gfortran.dg/pointer_init_2.f90: Adjust error messages.
	* gfortran.dg/pointer_init_6.f90: Likewise.
	* gfortran.dg/pointer_init_9.f90: New test.


Added:
    branches/gcc-8-branch/gcc/testsuite/gfortran.dg/pointer_init_9.f90
Modified:
    branches/gcc-8-branch/gcc/fortran/ChangeLog
    branches/gcc-8-branch/gcc/fortran/expr.c
    branches/gcc-8-branch/gcc/fortran/gfortran.h
    branches/gcc-8-branch/gcc/testsuite/ChangeLog
    branches/gcc-8-branch/gcc/testsuite/gfortran.dg/pointer_init_2.f90
    branches/gcc-8-branch/gcc/testsuite/gfortran.dg/pointer_init_6.f90
Comment 14 Jonathan Wakely 2019-12-20 23:04:47 UTC
(In reply to Thomas Koenig from comment #12)
> Author: tkoenig
> Date: Sun Feb 10 15:52:38 2019
> New Revision: 268748
> 
> URL: https://gcc.gnu.org/viewcvs?rev=268748&root=gcc&view=rev
> Log:
> 2019-02-10  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
> 	PR fortran/71237

Should have been PR 71723.