Re: [PATCH 0/2, fortran] Better code generation for DO loops with +-1 step

Re: [PATCH 0/2, fortran] Better code generation for DO loops with +-1 step

[Jakub Jelinek]

On Fri, Jul 08, 2016 at 11:03:35AM +0200, Jan Hubicka wrote:
> > On Fri, Jul 08, 2016 at 10:33:45AM +0200, Martin Liška wrote:
> > > On 07/07/2016 04:40 PM, Jan Hubicka wrote:
> > > >>
> > > >> Why is the behavior only undefined for step 1 if the last iteration IV
> > > >> increment overflows?
> > > >> Doesn't this apply to all step values?
> > > > 
> > > > This is what Fortran standard says:
> > > > 
> > > >   The iteration count is established and is the value of the expression (m2-m1+m3)/m3 unless that value is negative,
> > > >   in which case the iteration count is 0.
> > > > 
> > > > My reading of this is that the do statement is undefined whenever the expression above is undefined
> > > > (m1 is lower bound, m2 is upper bound, m3 is step) and because I think the evaulation order of
> > > > m2-m1+m3 is not fixed, I think the statement is not defined whethever (m2-m1), (m1+m3) or (m2-m1)+m3
> > 
> > m1+m3?  Did you mean m3-m1 or -m1+m3 instead?
> 
> Ah yes, -m1+m3.  But I am by no means language expert - this was meant as a heads up to Fortran
> people :)

Heads up to Fortran people should be sent to fortran@gcc.gnu.org ;)

	Jakub

Re: [PATCH 0/2, fortran] Better code generation for DO loops with +-1 step

[FX]

>>>>> This is what Fortran standard says:
>>>>> 
>>>>>  The iteration count is established and is the value of the expression (m2-m1+m3)/m3 unless that value is negative,
>>>>>  in which case the iteration count is 0.
>>>>> 
>>>>> My reading of this is that the do statement is undefined whenever the expression above is undefined
>>>>> (m1 is lower bound, m2 is upper bound, m3 is step) and because I think the evaulation order of
>>>>> m2-m1+m3 is not fixed, I think the statement is not defined whethever (m2-m1), (m1+m3) or (m2-m1)+m3

In the Fortran standard, (m2-m1+m3)/m3 is a mathematical expression, not a “construct”. So it cannot be “undefined”.
If you have explicit cases where you are asking “is this valid or invalid” please post them here (fortran@) and we will tell you.

FX

Re: [PATCH] Fix Fortran DO loop fallback

[Mike Stump]

> On Jul 11, 2016, at 7:44 AM, Jeff Law <law@redhat.com> wrote:
> 
> On 07/08/2016 08:26 AM, Martin Liška wrote:
>> Hello
>> 
>> Following patch fixes fallout caused by the patch set:
>> https://gcc.gnu.org/ml/gcc-regression/2016-07/msg00097.html
>> 
>> Ready after it finished regression tests?
>> Thanks,
>> Martin
>> 
>> 
>> 0001-Fix-Fortran-DO-loop-fallback.patch
>> 
>> 
>> From c5dd7ad62f795cce560c7f1bb8767b7ed9298d8a Mon Sep 17 00:00:00 2001
>> From: marxin <mliska@suse.cz>
>> Date: Fri, 8 Jul 2016 15:51:54 +0200
>> Subject: [PATCH] Fix Fortran DO loop fallback
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 2016-07-08  Martin Liska  <mliska@suse.cz>
>> 
>> 	* gfortran.dg/ldist-1.f90: Update expected dump scan.
>> 	* gfortran.dg/pr42108.f90: Likewise.
>> 	* gfortran.dg/vect/pr62283.f: Likewise.
> Shouldn't ldist-1.f90 be scan-tree-dump-not?  It seems like you change it from that just last week, but it wasn't mentioned in the ChangeLog.
> 
> OK with that change.

A gentle reminder, fortran patches should include the fortran list...

Re: [PATCH] Fix Fortran DO loop fallback

[Martin Liška]

On 07/12/2016 12:14 PM, Richard Biener wrote:
> On Mon, Jul 11, 2016 at 4:44 PM, Jeff Law <law@redhat.com> wrote:
>> On 07/08/2016 08:26 AM, Martin Liška wrote:
>>>
>>> Hello
>>>
>>> Following patch fixes fallout caused by the patch set:
>>> https://gcc.gnu.org/ml/gcc-regression/2016-07/msg00097.html
>>>
>>> Ready after it finished regression tests?
>>> Thanks,
>>> Martin
>>>
>>>
>>> 0001-Fix-Fortran-DO-loop-fallback.patch
>>>
>>>
>>> From c5dd7ad62f795cce560c7f1bb8767b7ed9298d8a Mon Sep 17 00:00:00 2001
>>> From: marxin <mliska@suse.cz>
>>> Date: Fri, 8 Jul 2016 15:51:54 +0200
>>> Subject: [PATCH] Fix Fortran DO loop fallback
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2016-07-08  Martin Liska  <mliska@suse.cz>
>>>
>>>         * gfortran.dg/ldist-1.f90: Update expected dump scan.
>>>         * gfortran.dg/pr42108.f90: Likewise.
>>>         * gfortran.dg/vect/pr62283.f: Likewise.
>>
>> Shouldn't ldist-1.f90 be scan-tree-dump-not?  It seems like you change it
>> from that just last week, but it wasn't mentioned in the ChangeLog.
> 
> gfortran.dg/pr42108.f90 also looks pointless now?  I suppose there is nothing
> to hoist after the change?  Do we now have an option to revert back to old
> behavior?  If so it would be better to use that flag here.

No, there's no option. So, as the test-case now looks pointless, should I mark it
with xfail now?

> 
> diff --git a/gcc/testsuite/gfortran.dg/vect/pr62283.f
> b/gcc/testsuite/gfortran.dg/vect/pr62283.f
> index 7df3d99..2933f51 100644
> --- a/gcc/testsuite/gfortran.dg/vect/pr62283.f
> +++ b/gcc/testsuite/gfortran.dg/vect/pr62283.f
> @@ -13,4 +13,4 @@ C { dg-additional-options "-fvect-cost-model=dynamic" }
>        beta=3.141593
>        y=y+beta*x
>        end
> -C { dg-final { scan-tree-dump-times "vectorized 1 loops" 2 "vect" {
> target { vect_hw_misalign } } } }
> +C { dg-final { scan-tree-dump "vectorized 1 loops" "vect" { target {
> vect_hw_misalign } } } }
> 
> so why do we no longer vectorize 1 loops in two functions?  The
> testcase works for me
> unchanged.

Yeah, it works on -m64, however as we're able to merge the functions with -m32 (-fipa-icf),
then I recommend to disable ICF for the test-case.

Reason why the pair of functions on x86_64 is that:

test3 (real(kind=4)[4] * restrict x, real(kind=4)[4] * restrict y)
{
  <bb 2>:

  <bb 3>:
  # S.0_6 = PHI <1(2), S.0_12(4)>
  if (S.0_6 > 4)
    goto <bb 5>;
  else
    goto <bb 4>;
...

test2 (real(kind=4)[4] * restrict x, real(kind=4)[4] * restrict y)
{
  integer(kind=4) i;

  <bb 2>:

  <bb 3>:
  # i_6 = PHI <1(2), i_12(4)>
...

On x86_64 types of 'S.0_6' and 'i' are not compatible. As I've just read tree dump files,
  # S.0_6 = PHI <1(2), S.0_12(4)>
  if (S.0_6 > 4)

is optimized out by VRP, which runs after IPA ICF.

I'll send patch right after we'll agree on pr42108.f90.

Thanks,
Martin

> 
> Richard.
> 
>> OK with that change.
>>
>> jeff
>>
>>

Re: [PATCH] Fix Fortran DO loop fallback

[Richard Biener]

On Tue, Jul 12, 2016 at 2:31 PM, Martin Liška <mliska@suse.cz> wrote:
> On 07/12/2016 12:14 PM, Richard Biener wrote:
>> On Mon, Jul 11, 2016 at 4:44 PM, Jeff Law <law@redhat.com> wrote:
>>> On 07/08/2016 08:26 AM, Martin Liška wrote:
>>>>
>>>> Hello
>>>>
>>>> Following patch fixes fallout caused by the patch set:
>>>> https://gcc.gnu.org/ml/gcc-regression/2016-07/msg00097.html
>>>>
>>>> Ready after it finished regression tests?
>>>> Thanks,
>>>> Martin
>>>>
>>>>
>>>> 0001-Fix-Fortran-DO-loop-fallback.patch
>>>>
>>>>
>>>> From c5dd7ad62f795cce560c7f1bb8767b7ed9298d8a Mon Sep 17 00:00:00 2001
>>>> From: marxin <mliska@suse.cz>
>>>> Date: Fri, 8 Jul 2016 15:51:54 +0200
>>>> Subject: [PATCH] Fix Fortran DO loop fallback
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2016-07-08  Martin Liska  <mliska@suse.cz>
>>>>
>>>>         * gfortran.dg/ldist-1.f90: Update expected dump scan.
>>>>         * gfortran.dg/pr42108.f90: Likewise.
>>>>         * gfortran.dg/vect/pr62283.f: Likewise.
>>>
>>> Shouldn't ldist-1.f90 be scan-tree-dump-not?  It seems like you change it
>>> from that just last week, but it wasn't mentioned in the ChangeLog.
>>
>> gfortran.dg/pr42108.f90 also looks pointless now?  I suppose there is nothing
>> to hoist after the change?  Do we now have an option to revert back to old
>> behavior?  If so it would be better to use that flag here.
>
> No, there's no option. So, as the test-case now looks pointless, should I mark it
> with xfail now?

The scan for 1 *n_ after FRE looks still useful.  Btw, the testcase
doesn't fail for me,
we _do_ hoist the division in PRE, just not with -m32 anymore.  Can
you confirm this?


>>
>> diff --git a/gcc/testsuite/gfortran.dg/vect/pr62283.f
>> b/gcc/testsuite/gfortran.dg/vect/pr62283.f
>> index 7df3d99..2933f51 100644
>> --- a/gcc/testsuite/gfortran.dg/vect/pr62283.f
>> +++ b/gcc/testsuite/gfortran.dg/vect/pr62283.f
>> @@ -13,4 +13,4 @@ C { dg-additional-options "-fvect-cost-model=dynamic" }
>>        beta=3.141593
>>        y=y+beta*x
>>        end
>> -C { dg-final { scan-tree-dump-times "vectorized 1 loops" 2 "vect" {
>> target { vect_hw_misalign } } } }
>> +C { dg-final { scan-tree-dump "vectorized 1 loops" "vect" { target {
>> vect_hw_misalign } } } }
>>
>> so why do we no longer vectorize 1 loops in two functions?  The
>> testcase works for me
>> unchanged.
>
> Yeah, it works on -m64, however as we're able to merge the functions with -m32 (-fipa-icf),
> then I recommend to disable ICF for the test-case.
>
> Reason why the pair of functions on x86_64 is that:
>
> test3 (real(kind=4)[4] * restrict x, real(kind=4)[4] * restrict y)
> {
>   <bb 2>:
>
>   <bb 3>:
>   # S.0_6 = PHI <1(2), S.0_12(4)>
>   if (S.0_6 > 4)
>     goto <bb 5>;
>   else
>     goto <bb 4>;
> ...
>
> test2 (real(kind=4)[4] * restrict x, real(kind=4)[4] * restrict y)
> {
>   integer(kind=4) i;
>
>   <bb 2>:
>
>   <bb 3>:
>   # i_6 = PHI <1(2), i_12(4)>
> ...
>
> On x86_64 types of 'S.0_6' and 'i' are not compatible. As I've just read tree dump files,
>   # S.0_6 = PHI <1(2), S.0_12(4)>
>   if (S.0_6 > 4)
>
> is optimized out by VRP, which runs after IPA ICF.
>
> I'll send patch right after we'll agree on pr42108.f90.
>
> Thanks,
> Martin
>
>>
>> Richard.
>>
>>> OK with that change.
>>>
>>> jeff
>>>
>>>
>

Re: [PATCH] Fix Fortran DO loop fallback

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

On 07/12/2016 03:15 PM, Richard Biener wrote:
> The scan for 1 *n_ after FRE looks still useful.  Btw, the testcase
> doesn't fail for me,
> we _do_ hoist the division in PRE, just not with -m32 anymore.  Can
> you confirm this?

Yeah, it works for both -m32 and -m64.
This is final version of the patch, may I install it?

Thanks,
Martin

[-- Attachment #2: 0001-Fix-Fortran-DO-loop-fallback-v2.patch --]
[-- Type: text/x-patch, Size: 2149 bytes --]

From e23c13dcc2f5e1967b0941c7a627241e3b6759e8 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 8 Jul 2016 15:51:54 +0200
Subject: [PATCH] Fix Fortran DO loop fallback

gcc/testsuite/ChangeLog:

2016-07-08  Martin Liska  <mliska@suse.cz>

	* gfortran.dg/ldist-1.f90: Revert change introduces in r238114.
	* gfortran.dg/pr42108.f90: Add -fno-ipa-icf to additional
	options.
	* gfortran.dg/vect/pr62283.f: Update expected dump scan.
---
 gcc/testsuite/gfortran.dg/ldist-1.f90    | 2 +-
 gcc/testsuite/gfortran.dg/pr42108.f90    | 2 --
 gcc/testsuite/gfortran.dg/vect/pr62283.f | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/gfortran.dg/ldist-1.f90 b/gcc/testsuite/gfortran.dg/ldist-1.f90
index 2030328..ea3990d 100644
--- a/gcc/testsuite/gfortran.dg/ldist-1.f90
+++ b/gcc/testsuite/gfortran.dg/ldist-1.f90
@@ -32,4 +32,4 @@ end Subroutine PADEC
 ! There are 5 legal partitions in this code.  Based on the data
 ! locality heuristic, this loop should not be split.
 
-! { dg-final { scan-tree-dump "distributed: split to" "ldist" } }
+! { dg-final { scan-tree-dump-not "distributed: split to" "ldist" } }
diff --git a/gcc/testsuite/gfortran.dg/pr42108.f90 b/gcc/testsuite/gfortran.dg/pr42108.f90
index eb93604..a913aa4 100644
--- a/gcc/testsuite/gfortran.dg/pr42108.f90
+++ b/gcc/testsuite/gfortran.dg/pr42108.f90
@@ -21,7 +21,5 @@ subroutine  eval(foo1,foo2,foo3,foo4,x,n,nnd)
   end do
 end subroutine eval
 
-! We should have hoisted the division
-! { dg-final { scan-tree-dump "in all uses of countm1\[^\n\]* / " "pre" } }
 ! There should be only one load from n left
 ! { dg-final { scan-tree-dump-times "\\*n_" 1 "fre1" } }
diff --git a/gcc/testsuite/gfortran.dg/vect/pr62283.f b/gcc/testsuite/gfortran.dg/vect/pr62283.f
index 7df3d99..4d8cba1 100644
--- a/gcc/testsuite/gfortran.dg/vect/pr62283.f
+++ b/gcc/testsuite/gfortran.dg/vect/pr62283.f
@@ -1,5 +1,5 @@
 C { dg-do compile }
-C { dg-additional-options "-fvect-cost-model=dynamic" }
+C { dg-additional-options "-fvect-cost-model=dynamic -fno-ipa-icf" }
       subroutine test2(x,y)
       real x(4),y(4)
       beta=3.141593
-- 
2.8.4

Re: [PATCH] Fix Fortran DO loop fallback

[Richard Biener]

On Tue, Jul 12, 2016 at 4:54 PM, Martin Liška <mliska@suse.cz> wrote:
> On 07/12/2016 03:15 PM, Richard Biener wrote:
>> The scan for 1 *n_ after FRE looks still useful.  Btw, the testcase
>> doesn't fail for me,
>> we _do_ hoist the division in PRE, just not with -m32 anymore.  Can
>> you confirm this?
>
> Yeah, it works for both -m32 and -m64.
> This is final version of the patch, may I install it?

Changelog has a swapped entry.

Ok with fixing that.
Richard.

> Thanks,
> Martin

Re: [PATCH] Fix Fortran DO loop fallback

[Martin Liška]

On 07/13/2016 12:32 PM, Richard Biener wrote:
> Changelog has a swapped entry.
> 
> Ok with fixing that.
> Richard.

Fixed and installed as r238300.
M.