[Patch, Fortran] PR 41907 - regression fix; wrong-code involving optional arguments

[Tobias Burnus]

(Next try - sourceware.org didn't like the email before.)

Hello all,

my previous patch* somehow got it wrong with regards to when a
  tmp = argument == NULL ? NULL : argument(.0)
  proc (tmp)
check is needed and when not.

The result was that Tonto (and thus CPU SPEC 2006) was crashing -> PR 41907


Cases and changes:

a) If one passes a non-array-descriptor optional dummy to as actual
argument to a non-array-descriptor dummy, no check is needed.  (missed
optimization)

b) If one passes an array-descriptor optional dummy to an
array-descriptor dummy, a check is needed (see below why).  (wrong-code
-> segfault)

c) If one dummy needs an array descriptor and the other doesn't, either
an array copying is involved or a reference of the type "argument.0[0]"
and thus one needs a check. (Unchanged, was working before)

I added a couple of tests to make sure that no wrong-code gets
introduced and that the optimizations indeed happen for (a) and for scalars.

 * * *

Regarding b: In principle, one would not need such a check as one could
directly pass the descriptor on. However, this does not work as the
current procedure is:

1. Convert the dummy argument such that the lower bounds start at "1"
(or some value) instead of the lbound of the actual argument. This
produces a handful of variables, but no array descriptor.

2. Create an array descriptor out of those variables

There is one unnecessary step involved. One could either pass the dummy
argument directly (though to grab it is difficult and there might be
issues with regards to using to restricted pointers pointing to the same
object). Or one uses already an array descriptor in step 1. I think
doing the latter is a sensible approach. There is now a
missed-optimization PR 41911.

 * * *

Build and regtested on x86-64-linux.
OK for the trunk?  (4.4 is not affected.)

Tobias

PS: Sorry for the breakage.

* http://gcc.gnu.org/ml/fortran/2009-10/msg00246.html ·
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41850
<http://gcc.gnu.org/ml/fortran/2009-10/msg00246.html>

[missing-opt.diff]

---

2009-11-03  Tobias Burnus  <burnus@...>

        PR fortran/41907
        * trans-expr.c (gfc_conv_procedure_call): Fix presence check
        for optional arguments.

2009-11-03  Tobias Burnus  <burnus@...>

        PR fortran/41907
        * gfortran.dg/missing_optional_dummy_6.f90: New test.

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c (revision 153817)
+++ gcc/fortran/trans-expr.c (working copy)
@@ -2998,16 +2998,19 @@ gfc_conv_procedure_call (gfc_se * se, gf
      only needed when passing an array to an elemental procedure
      as then array elements are accessed - or no NULL pointer is
      allowed and a "1" or "0" should be passed if not present.
-     When passing a deferred array to a non-deferred array dummy,
-     the array needs to be packed and a check needs thus to be
-     inserted.  */
+     When passing a non-array-descriptor full array to a
+     non-array-descriptor dummy, no check is needed. For
+     array-descriptor actual to array-descriptor dummy, see
+     PR 41911 for why a check has to be inserted.
+     fsym == NULL is checked as intrinsics required the descriptor
+     but do not always set fsym.  */
   if (e->expr_type == EXPR_VARIABLE
       && e->symtree->n.sym->attr.optional
       && ((e->rank > 0 && sym->attr.elemental)
   || e->representation.length || e->ts.type == BT_CHARACTER
-  || (e->rank > 0 && (fsym == NULL
-      || (fsym->as->type != AS_ASSUMED_SHAPE
-  && fsym->as->type != AS_DEFERRED)))))
+  || (e->rank > 0
+      && (fsym == NULL || fsym->as->type == AS_ASSUMED_SHAPE
+  || fsym->as->type == AS_DEFERRED))))
     gfc_conv_missing_dummy (&parmse, e, fsym ? fsym->ts : e->ts,
     e->representation.length);
  }
Index: gcc/testsuite/gfortran.dg/missing_optional_dummy_6.f90
===================================================================
--- gcc/testsuite/gfortran.dg/missing_optional_dummy_6.f90 (revision 0)
+++ gcc/testsuite/gfortran.dg/missing_optional_dummy_6.f90 (revision 0)
@@ -0,0 +1,60 @@
+! { dg-do run }
+! { dg-options "-fdump-tree-original" }
+!
+! PR fortran/41907
+!
+program test
+  implicit none
+  call scalar1 ()
+  call assumed_shape1 ()
+  call explicit_shape1 ()
+contains
+
+  ! Calling functions
+  subroutine scalar1 (slr1)
+    integer, optional :: slr1
+    call scalar2 (slr1)
+  end subroutine scalar1
+
+  subroutine assumed_shape1 (as1)
+    integer, dimension(:), optional :: as1
+    call assumed_shape2 (as1)
+    call explicit_shape2 (as1)
+  end subroutine assumed_shape1
+
+  subroutine explicit_shape1 (es1)
+    integer, dimension(5), optional :: es1
+    call assumed_shape2 (es1)
+    call explicit_shape2 (es1)
+  end subroutine explicit_shape1
+
+
+  ! Called functions
+  subroutine assumed_shape2 (as2)
+    integer, dimension(:),optional :: as2
+    if (present (as2)) call abort()
+  end subroutine assumed_shape2
+
+  subroutine explicit_shape2 (es2)
+    integer, dimension(5),optional :: es2
+    if (present (es2)) call abort()
+  end subroutine explicit_shape2
+
+  subroutine scalar2 (slr2)
+    integer, optional :: slr2
+    if (present (slr2)) call abort()
+  end subroutine scalar2
+
+end program test
+
+! { dg-final { scan-tree-dump-times "scalar2 \\(slr1" 1 "original" } }
+
+! { dg-final { scan-tree-dump-times "= es1 != 0B" 1 "original" } }
+! { dg-final { scan-tree-dump-times "assumed_shape2 \\(es1" 0 "original" } }
+! { dg-final { scan-tree-dump-times "explicit_shape2 \\(es1" 1 "original" } }
+
+! { dg-final { scan-tree-dump-times "= as1 != 0B" 2 "original" } }
+! { dg-final { scan-tree-dump-times "assumed_shape2 \\(as1" 0 "original" } }
+! { dg-final { scan-tree-dump-times "explicit_shape2 \\(as1" 0 "original" } }
+
+! { dg-final { cleanup-tree-dump "original" } }

---

[Daniel Kraft-4]

Tobias Burnus wrote:

> (Next try - sourceware.org didn't like the email before.)
>
> Hello all,
>
> my previous patch* somehow got it wrong with regards to when a
>   tmp = argument == NULL ? NULL : argument(.0)
>   proc (tmp)
> check is needed and when not.
>
> The result was that Tonto (and thus CPU SPEC 2006) was crashing -> PR 41907
>
> Build and regtested on x86-64-linux.
> OK for the trunk?  (4.4 is not affected.)

Ok.  Thanks for the quick fix.

Yours,
Daniel

--
Done:  Arc-Bar-Cav-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Kni-Mon-Pri