[Bug fortran/44235] New: array temporary with high upper bound

[Bug fortran/44235] New: array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

subroutine foo(a, b)
  real :: a(10), b(10)
  a(1:4:2) = a(2:4:2)
  b(1:3:2) = b(2:4:2)
end subroutine foo
ig25@linux-fd1f:/tmp> gfortran -c -Warray-temporaries dep.f90
dep.f90:3.13:

  a(1:4:2) = a(2:4:2)
             1
Warnung: Creating array temporary at (1)

The upper bound of '4' in the first array assingment is no different
from '3'; the generation of the array temporary should be based on
the number of iterations, not the upper bound.


-- 
           Summary: array temporary with high upper bound
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: enhancement
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: tkoenig at gcc dot gnu dot org
OtherBugsDependingO 36854
             nThis:


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

-- 

tkoenig at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |tkoenig at gcc dot gnu dot
                   |dot org                     |org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-05-22 08:20:04
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

------- Comment #1 from tkoenig at gcc dot gnu dot org  2010-06-04 22:39 -------
The particular test case from comment #1 is now fixed.

Here's one that still fails:

subroutine foo(a, b)
  real :: a(40), b(40)
  a(1:20:3) = a(1:19:3)
  a(1:19:3) = a(1:19:3)
end subroutine foo

g25@linux-fd1f:/tmp> gfortran -Warray-temporaries -S dep2.f90
dep2.f90:3.14:

  a(1:20:3) = a(1:19:3)
              1
Warning: Creating array temporary at (1)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[dominiq at lps dot ens dot fr]

------- Comment #2 from dominiq at lps dot ens dot fr  2010-07-01 20:23 -------
Lightly tested patch that does not create temporaries for the test in comment
#1


--- ../_clean/gcc/fortran/dependency.c  2010-06-21 17:31:37.000000000 +0200
+++ gcc/fortran/dependency.c    2010-07-01 22:16:51.000000000 +0200
@@ -1037,8 +1037,9 @@ gfc_check_section_vs_section (gfc_ref *l

   /* Check for forward dependencies x:y vs. x+1:z.  */
   if (l_dir == 1 && r_dir == 1
-      && l_start && r_start && gfc_dep_compare_expr (l_start, r_start) == -1
-      && l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == -1)
+      && l_start && r_start && (gfc_dep_compare_expr (l_start, r_start) == 0
+                            || gfc_dep_compare_expr (l_start, r_start) == -1)
+      /* && l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == -1 */)
     {
       /* Check that the strides are the same.  */
       if (!l_stride && !r_stride)
@@ -1050,8 +1051,9 @@ gfc_check_section_vs_section (gfc_ref *l

   /* Check for forward dependencies x:y:-1 vs. x-1:z:-1.  */
   if (l_dir == -1 && r_dir == -1
-      && l_start && r_start && gfc_dep_compare_expr (l_start, r_start) == 1
-      && l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == 1)
+      && l_start && r_start && (gfc_dep_compare_expr (l_start, r_start) == 0
+                             || gfc_dep_compare_expr (l_start, r_start) == 1)
+      /* && l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == 1 */)
     {
       /* Check that the strides are the same.  */
       if (!l_stride && !r_stride)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[steven at gcc dot gnu dot org]

------- Comment #3 from steven at gcc dot gnu dot org  2010-08-06 21:24 -------
What's the plan with the patch of comment #2?
NB, the result of gfc_dep_compare_expr (l_start, r_start) could be cached
instead of computed twice:

+ && ((res = gfc_dep_compare_expr (l_start, r_start)) == 0
+     || res == -1)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[dominiq at lps dot ens dot fr]

------- Comment #4 from dominiq at lps dot ens dot fr  2010-08-06 21:34 -------
I am not to sure about the patch in comment #2 because the case should probably
be handled by gfc_is_same_range and the patch does it in
gfc_check_section_vs_section. Note that gfc_is_same_range has a line

  /* TODO: More sophisticated range comparison.  */

In my opinion the right way to fix this pr would be to compare the starts and
the extends, but so far I did not find how to do it.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[dominiq at lps dot ens dot fr]

------- Comment #5 from dominiq at lps dot ens dot fr  2010-08-06 22:49 -------
Note that the following patch I have in my tree for some time now also fix the
pr


--- gcc/fortran/dependency.c    2010-08-07 00:37:34.000000000 +0200
+++ ../work/gcc/fortran/dependency.c    2010-08-05 19:11:58.000000000 +0200
@@ -1172,8 +1172,8 @@ check_section_vs_section (gfc_array_ref 

   /* Check for forward dependencies x:y vs. x+1:z.  */
   if (l_dir == 1 && r_dir == 1
-      && l_start && r_start && gfc_dep_compare_expr (l_start, r_start) == -1
-      && l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == -1)
+      && ((l_start && r_start && gfc_dep_compare_expr (l_start, r_start) ==
-1)
+         || (l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == -1)))
     {
       /* Check that the strides are the same.  */
       if (!l_stride && !r_stride)
@@ -1185,8 +1185,8 @@ check_section_vs_section (gfc_array_ref 

   /* Check for forward dependencies x:y:-1 vs. x-1:z:-1.  */
   if (l_dir == -1 && r_dir == -1
-      && l_start && r_start && gfc_dep_compare_expr (l_start, r_start) == 1
-      && l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == 1)
+      && ((l_start && r_start && gfc_dep_compare_expr (l_start, r_start) == 1)
+         || (l_end && r_end && gfc_dep_compare_expr (l_end, r_end) == 1)))
     {
       /* Check that the strides are the same.  */
       if (!l_stride && !r_stride)

This followed a discussion with Tobias Burnus on IRC. It assumes that the code
is valid, i.e., lhs and rhs has the same extent, otherwise creating a temporary
does make the code valid, even if the result is not the same with or without
temporary.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[dominiq at lps dot ens dot fr]

------- Comment #6 from dominiq at lps dot ens dot fr  2010-08-07 09:01 -------
It turns out that the test in comment #0 was not fixed by the patch in comment
#5, but by revision 162966. However with the slight change

  a(4:23:3) = a(4:22:3)

a temporary is still created. The patch in comment #5 avoid temporaries in
situations such as

  a(4:19:3) = a(7:22:3)
  a(4:20:3) = a(7:22:3)
  a(4:19:3) = a(7:23:3)
  nl = 4
  nu = 20
  a(1:16:3) = a(4:nu:3)
  a(1:16:3) = a(nl:20:3)

but not for

  a(1:16:3) = a(nl:nu:3)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

------- Comment #7 from tkoenig at gcc dot gnu dot org  2010-08-07 11:17 -------
(In reply to comment #6)

Hi Dominique,

> It turns out that the test in comment #0 was not fixed by the patch in comment
> #5, but by revision 162966. However with the slight change
> 
>   a(4:23:3) = a(4:22:3)
> 
> a temporary is still created. The patch in comment #5 avoid temporaries in
> situations such as
> 
>   a(4:19:3) = a(7:22:3)
>   a(4:20:3) = a(7:22:3)
>   a(4:19:3) = a(7:23:3)
>   nl = 4
>   nu = 20
>   a(1:16:3) = a(4:nu:3)
>   a(1:16:3) = a(nl:20:3)
> 
> but not for
> 
>   a(1:16:3) = a(nl:nu:3)

the only unnecessary temporary still created with current trunk is your first
example.

I am more in favor of changing the upper bound to its actual value,
probably during resolution.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[dominiq at lps dot ens dot fr]

------- Comment #8 from dominiq at lps dot ens dot fr  2010-08-07 13:40 -------
> the only unnecessary temporary still created with current trunk is your first
> example.

Again it is because the lower bound of the section is that or the array, but if
increase it I see


pr44235_1_db.f90:22.14:

  a(4:19:3) = a(7:nu:3)
              1
Warning: Creating array temporary at (1)
pr44235_1_db.f90:23.14:

  a(4:19:3) = a(nl:23:3)
              1
Warning: Creating array temporary at (1)

with a clean trunk that disappear with the patch in comment #5

> I am more in favor of changing the upper bound to its actual value,
> probably during resolution.

I agree, it's what I tried to say in comment #4.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

------- Comment #9 from tkoenig at gcc dot gnu dot org  2010-08-08 16:08 -------
Created an attachment (id=21437)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21437&action=view)
Proposed patch

This patch seems to work, and do more or less the right thing.
It inserts the upper bound only for strides not equal to one.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[dominiq at lps dot ens dot fr]

------- Comment #10 from dominiq at lps dot ens dot fr  2010-08-08 20:08 -------
> This patch seems to work, and do more or less the right thing.

No counter-example of that!-) thanks.

> It inserts the upper bound only for strides not equal to one.

I am unable to follow the code. Do you mean that you are putting the section in
some canonical form with the minimal value of 'end'? What puzzle me is that I
do not see any division, is it hidden in the gmp calls?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

------- Comment #11 from tkoenig at gcc dot gnu dot org  2010-08-08 22:40 -------
(In reply to comment #10)
> > This patch seems to work, and do more or less the right thing.
> 
> No counter-example of that!-) thanks.

That's good.

> > It inserts the upper bound only for strides not equal to one.
> 
> I am unable to follow the code. Do you mean that you are putting the section in
> some canonical form with the minimal value of 'end'?

That is correct.  If the stride is not equal to one, and if the
size can be calculated at compile-time (which the call to gfc_ref_dimen_size
checks, and does), then the end of the array reference is set to the minimum.
I extended that function to also calculate the end.

> What puzzle me is that I
> do not see any division, is it hidden in the gmp calls?

The division is handled in gfc_ref_dimen_size, where I simply used
the size calculatd there to also calculate the minimum end.  You'll find
the division there.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

------- Comment #12 from tkoenig at gcc dot gnu dot org  2010-08-09 19:35 -------
Subject: Bug 44235

Author: tkoenig
Date: Mon Aug  9 19:34:49 2010
New Revision: 163041

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163041
Log:
2010-08-09  Thomas Koenig  <tkoenig@gcc.gnu.org>

        PR fortran/44235
        * array.c (gfc_ref_dimen_size):  Add end argument.
        If end is non-NULL, calculate it.
        (ref_size):  Adjust call to gfc_ref_dimen_size.
        (gfc_array_dimen_size):  Likewise.
        (gfc_array_res_shape):  Likewise.
        * gfortran.h:  Adjust prototype for gfc_ref_dimen_size.
        * resolve.c (resolve_array_ref):  For stride not equal to -1,
        fill in the lowest possible end.

2010-08-09  Thomas Koenig  <tkoenig@gcc.gnu.org>

        PR fortran/44235
        * gfortran.dg/dependency_32.f90:  New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/dependency_32.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/array.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/simplify.c
    trunk/gcc/testsuite/ChangeLog


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235

[Bug fortran/44235] array temporary with high upper bound

[tkoenig at gcc dot gnu dot org]

------- Comment #13 from tkoenig at gcc dot gnu dot org  2010-08-27 12:12 -------
Fixed on trunk.

Closing.


-- 

tkoenig at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44235