diff mbox

Fix Fortran DO loop fallback

Message ID CAFiYyc1sotqJVzpNaKiZ4azVsWxeouvZSRvMGXP98XStQrLDoA@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener July 12, 2016, 10:14 a.m. UTC 
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.

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

Richard.

> OK with that change.
>
> jeff
>
>

Comments

Martin Liška July 12, 2016, 12:31 p.m. UTC | #1 
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
>>
>>
Richard Biener July 12, 2016, 1:15 p.m. UTC | #2 
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
>>>
>>>
>
diff mbox

Patch

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