public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [Patch, fortran] [0/5] PR 45648: Inline transpose part 2
@ 2010-09-20 22:21 Mikael Morin
  2010-09-20 22:40 ` [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds Mikael Morin
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Mikael Morin @ 2010-09-20 22:21 UTC (permalink / raw)
  To: gfortran, patches

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

Hello,

this is the second part of the inline transpose patch, re-enabling the
non-copying transposed descriptor optimization.

Allmost all happens in gfc_conv_expr_descriptor: 
1/5: We use the scalarizer's dim array to get the right bounds while creating the descriptor. 
2/5: This prevents a regression in ret_array_1 where a transposed array was handled as a full array with patch 3/5 alone.
3/5: This bypasses the temporary generation in gfc_conv_expr_descriptor in the transpose case. 
4/5: It is a cleanup I made while working on this, but it is somewhat unrelated, and optional actually. 
5/5: This forces the creation of a temporary in case there is an alias between actual arguments (corrects a wrong code regression introduced by patch 3/5).

Each patch is regression tested on an older revision and all together on today's trunk. 
I will ask for trunk commital for each one of them. 

Mikael



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
@ 2010-09-20 22:40 ` Mikael Morin
  2010-09-21 14:09   ` Paul Richard Thomas
  2010-09-20 23:27 ` [Patch, fortran] [5/5] PR 45648: Inline transpose part 2: Do dependency analysis in case of transpose optimization Mikael Morin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Mikael Morin @ 2010-09-20 22:40 UTC (permalink / raw)
  To: gfortran, patches

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

We can't have dim (new descriptor's dimension) increasing together with n (original array's dimension) if we want to support transposed reference. We have to get dim such that info->dim[dim] == n. Then we can set the descriptor bounds along dim.

OK for trunk?



[-- Attachment #2: inline_transpose_2-1.changelog --]
[-- Type: text/plain, Size: 151 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* trans-array.c (gfc_conv_expr_descriptor): Calculate dim out of n and
	info->dim.


[-- Attachment #3: inline_transpose_2-1.diff --]
[-- Type: text/plain, Size: 1507 bytes --]

diff --git a/trans-array.c b/trans-array.c
index 7bce2ef..f15b53e 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5439,7 +5439,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	}
 
       offset = gfc_index_zero_node;
-      dim = 0;
 
       /* The following can be somewhat confusing.  We have two
          descriptors, a new one and the original array.
@@ -5479,9 +5478,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	    }
 	  else
 	    {
-	      /* Check we haven't somehow got out of sync.  */
-	      gcc_assert (info->dim[dim] == n);
-
 	      /* Evaluate and remember the start of the section.  */
 	      start = info->start[n];
 	      stride = gfc_evaluate_now (stride, &loop.pre);
@@ -5505,6 +5501,12 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	  /* Vector subscripts need copying and are handled elsewhere.  */
 	  if (info->ref)
 	    gcc_assert (info->ref->u.ar.dimen_type[n] == DIMEN_RANGE);
+ 
+	  /* look for the corresponding scalarizer dimension: dim.  */
+	  for (dim = 0; dim < ndim; dim++)
+	    if (info->dim[dim] == n)
+	      break;
+	  gcc_assert (dim < ndim);
 
 	  /* Set the new lower bound.  */
 	  from = loop.from[dim];
@@ -5559,8 +5561,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	  /* Store the new stride.  */
 	  gfc_conv_descriptor_stride_set (&loop.pre, parm,
 					  gfc_rank_cst[dim], stride);
-
-	  dim++;
 	}
 
       if (se->data_not_needed)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Patch, fortran] [5/5] PR 45648: Inline transpose part 2: Do dependency analysis in case of transpose optimization.
  2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
  2010-09-20 22:40 ` [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds Mikael Morin
@ 2010-09-20 23:27 ` Mikael Morin
       [not found]   ` <AANLkTi=bzqmABzx8vW8Kg-m7_qRnk4g=fTXHrpRkvot4@mail.gmail.com>
  2010-09-20 23:45 ` [Patch, fortran] [4/5] PR 45648: Inline transpose part 2: Remove ss lookup Mikael Morin
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Mikael Morin @ 2010-09-20 23:27 UTC (permalink / raw)
  To: gfortran, patches

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

This disables the transpose optimization in case there is an alias between arguments.
For example:
!!!
call foo(a, transpose(a))
!!!
I couldn't see any better way than setting a force_tmp field in the gfc_se struct before entering gfc_conv_array_parameter for argument evaluation, and then using it to set the need_tmp flag in gfc_conv_expr_descriptor.

Ok for trunk ?



[-- Attachment #2: inline_transpose_2-5.changelog --]
[-- Type: text/plain, Size: 328 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* trans.h (gfc_se): New field force_tmp. 
	* trans-expr.c (gfc_conv_procedure_call): Check for argument alias
	and set parmse.force_tmp if some alias is found. 
	* trans-array.c (gfc_conv_expr_descriptor): Force a temporary creation
	if se->force_tmp is set. 


[-- Attachment #3: inline_transpose_2-5.diff --]
[-- Type: text/plain, Size: 4387 bytes --]

diff --git a/trans-array.c b/trans-array.c
index 947ed4b..1bb4429 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5136,7 +5136,6 @@ get_array_charlen (gfc_expr *expr, gfc_se *se)
 }
 
 
-
 /* Convert an array for passing as an actual argument.  Expressions and
    vector subscripts are evaluated and stored in a temporary, which is then
    passed.  For whole arrays the descriptor is passed.  For array sections
@@ -5158,7 +5157,13 @@ get_array_charlen (gfc_expr *expr, gfc_se *se)
 	 EXPR is the right-hand side of a pointer assignment and
 	 se->expr is the descriptor for the previously-evaluated
 	 left-hand side.  The function creates an assignment from
-	 EXPR to se->expr.  */
+	 EXPR to se->expr.  
+
+
+   The se->force_tmp flag disables the non-copying descriptor optimization
+   that is used for transpose. It may be used in cases where there is an
+   alias between the transpose argument and another argument in the same
+   function call.  */
 
 void
 gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
@@ -5198,6 +5203,9 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       need_tmp = gfc_ref_needs_temporary_p (expr->ref)
 			&& !subref_array_target;
 
+      if (se->force_tmp)
+	need_tmp = 1;
+
       if (need_tmp)
 	full = 0;
       else if (GFC_ARRAY_TYPE_P (TREE_TYPE (desc)))
@@ -5327,6 +5335,11 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       break;
     }
 
+  /* If we are creating a temporary, we don't need to bother about aliases
+     anymore.  */
+  if (need_tmp)
+    se->force_tmp = 0;
+
   gfc_init_loopinfo (&loop);
 
   /* Associate the SS with the loop.  */
diff --git a/trans-expr.c b/trans-expr.c
index 8d4295f..abeaa36 100644
--- a/trans-expr.c
+++ b/trans-expr.c
@@ -2770,7 +2770,7 @@ conv_isocbinding_procedure (gfc_se * se, gfc_symbol * sym,
 
 int
 gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
-			 gfc_actual_arglist * arg, gfc_expr * expr,
+			 gfc_actual_arglist * args, gfc_expr * expr,
 			 VEC(tree,gc) *append_args)
 {
   gfc_interface_mapping mapping;
@@ -2789,6 +2789,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
   VEC(tree,gc) *stringargs;
   tree result = NULL;
   gfc_formal_arglist *formal;
+  gfc_actual_arglist *arg;
   int has_alternate_specifier = 0;
   bool need_interface_mapping;
   bool callee_alloc;
@@ -2809,7 +2810,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
   gfc_clear_ts (&ts);
 
   if (sym->from_intmod == INTMOD_ISO_C_BINDING
-      && conv_isocbinding_procedure (se, sym, arg))
+      && conv_isocbinding_procedure (se, sym, args))
     return 0;
 
   gfc_is_proc_ptr_comp (expr, &comp);
@@ -2859,7 +2860,8 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
     }
 
   /* Evaluate the arguments.  */
-  for (; arg != NULL; arg = arg->next, formal = formal ? formal->next : NULL)
+  for (arg = args; arg != NULL;
+       arg = arg->next, formal = formal ? formal->next : NULL)
     {
       e = arg->expr;
       fsym = formal ? formal->sym : NULL;
@@ -3040,6 +3042,24 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 	      else
 		f = f || !sym->attr.always_explicit;
 
+	      /* If the argument is a function call that may not create
+		 a temporary for the result, we have to check that we
+		 can do it, i.e. that there is no alias between this 
+		 argument and another one.  */
+	      if (gfc_get_noncopying_intrinsic_argument (e) != NULL)
+		{
+		  sym_intent intent;
+
+		  if (fsym != NULL)
+		    intent = fsym->attr.intent;
+		  else
+		    intent = INTENT_UNKNOWN;
+
+		  if (gfc_check_fncall_dependency (e, intent, sym, args,
+						   NOT_ELEMENTAL))
+		    parmse.force_tmp = 1;
+		}
+
 	      if (e->expr_type == EXPR_VARIABLE
 		    && is_subref_array (e))
 		/* The actual argument is a component reference to an
diff --git a/trans.h b/trans.h
index acdd3e3..a883cf5 100644
--- a/trans.h
+++ b/trans.h
@@ -81,6 +81,11 @@ typedef struct gfc_se
   /* If set, gfc_conv_procedure_call does not put byref calls into se->pre.  */
   unsigned no_function_call:1;
 
+  /* If set, we will force the creation of a temporary. Useful to disable
+     non-copying procedure argument passing optimizations, when some function
+     args alias.  */
+  unsigned force_tmp:1;
+
   /* Scalarization parameters.  */
   struct gfc_se *parent;
   struct gfc_ss *ss;

[-- Attachment #4: inline_transpose_2_testsuite-5.changelog --]
[-- Type: text/plain, Size: 232 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* gfortran.dg/inline_transpose_1.f90: Add function calls with aliasing
	arguments checks. Update temporary counts.
	* gfortran.dg/transpose_optimization_1.f90: New.

[-- Attachment #5: inline_transpose_2_testsuite-5.diff --]
[-- Type: text/plain, Size: 5414 bytes --]

diff --git a/inline_transpose_1.f90 b/inline_transpose_1.f90
index 36198f8..a364842 100644
--- a/inline_transpose_1.f90
+++ b/inline_transpose_1.f90
@@ -168,6 +168,19 @@
   call toto2 (c, transpose (a))
   if (any (c /= 2 * q + 13)) call abort
 
+  call toto2 (e, transpose(e))           ! { dg-warning "Creating array temporary" }
+  if (any (e /= 4 * r + 13)) call abort
+
+  call toto2 (e, transpose(transpose(e)))           ! { dg-warning "Creating array temporary" }
+  if (any (e /= 4 * r + 14)) call abort
+
+
+  call toto3 (e, transpose(e))
+  if (any (e /= 4 * r + 14)) call abort
+
+
+  call titi (nx, e, transpose(e))           ! { dg-warning "Creating array temporary" }
+  if (any (e /= 4 * s + 17)) call abort
 
   contains
 
@@ -199,17 +212,26 @@
     x = y + 1
   end subroutine toto2
 
+  subroutine toto3 (x, y)
+    integer, dimension(:,:), intent(in) :: x, y
+  end subroutine toto3
+
 end
 
+subroutine titi (n, x, y)
+  integer :: n, x(n,n), y(n,n)
+  x = y + 3
+end subroutine titi
+
 ! No call to transpose
 ! { dg-final { scan-tree-dump-times "_gfortran_transpose" 0 "original" } }
 !
-! 21 temporaries
-! { dg-final { scan-tree-dump-times "struct\[^\\n\]*atmp" 21 "original" } }
+! 24 temporaries
+! { dg-final { scan-tree-dump-times "struct\[^\\n\]*atmp" 24 "original" } }
 !
 ! 2 tests optimized out
-! { dg-final { scan-tree-dump-times "_gfortran_abort" 35 "original" } }
-! { # Commented out as failing at -O0: dg-final { scan-tree-dump-times "_gfortran_abort" 33 "optimized" } }
+! { dg-final { scan-tree-dump-times "_gfortran_abort" 39 "original" } }
+! { # Commented out as failing at -O0: dg-final { scan-tree-dump-times "_gfortran_abort" 37 "optimized" } }
 !
 ! cleanup
 ! { dg-final { cleanup-tree-dump "original" } }
diff --git a/transpose_optimization_1.f90 b/transpose_optimization_1.f90
new file mode 100644
index 0000000..885ff7c
--- /dev/null
+++ b/transpose_optimization_1.f90
@@ -0,0 +1,106 @@
+! { dg-do compile }
+! { dg-options "-Warray-temporaries -fdump-tree-original" }
+!
+! PR fortran/45648
+! Non-copying descriptor transpose optimization (for function call args).
+!
+! Contributed by Richard Sandiford <richard@codesourcery.com>
+
+module foo
+  interface
+    subroutine ext1 (a, b)
+      real, intent (in), dimension (:, :) :: a, b
+    end subroutine ext1
+    subroutine ext2 (a, b)
+      real, intent (in), dimension (:, :) :: a
+      real, intent (out), dimension (:, :) :: b
+    end subroutine ext2
+    subroutine ext3 (a, b)
+      real, dimension (:, :) :: a, b
+    end subroutine ext3
+  end interface
+contains
+  ! No temporary needed here.
+  subroutine test1 (n, a, b, c)
+    integer :: n
+    real, dimension (n, n) :: a, b, c
+    a = matmul (transpose (b), c)
+  end subroutine test1
+
+  ! No temporary either, as we know the arguments to matmul are intent(in)
+  subroutine test2 (n, a, b)
+    integer :: n
+    real, dimension (n, n) :: a, b
+    a = matmul (transpose (b), b)
+  end subroutine test2
+
+  ! No temporary needed.
+  subroutine test3 (n, a, b, c)
+    integer :: n
+    real, dimension (n, n) :: a, c
+    real, dimension (n+4, n+4) :: b
+    a = matmul (transpose (b (2:n+1, 3:n+2)), c)
+  end subroutine test3
+
+  ! A temporary is needed for the result of either the transpose or matmul.
+  subroutine test4 (n, a, b)
+    integer :: n
+    real, dimension (n, n) :: a, b
+    a = matmul (transpose (a), b)       ! { dg-warning "Creating array temporary" }
+  end subroutine test4
+
+  ! The temporary is needed here since the second argument to imp1
+  ! has unknown intent.
+  subroutine test5 (n, a)
+    integer :: n
+    real, dimension (n, n) :: a
+    call imp1 (transpose (a), a)        ! { dg-warning "Creating array temporary" }
+  end subroutine test5
+
+  ! No temporaries are needed here; imp1 can't modify either argument.
+  ! We have to pack the arguments, however. 
+  subroutine test6 (n, a, b)
+    integer :: n
+    real, dimension (n, n) :: a, b
+    call imp1 (transpose (a), transpose (b))    ! { dg-warning "Creating array temporary" }
+  end subroutine test6
+
+  ! No temporaries are needed here; imp1 can't modify either argument.
+  ! We don't have to pack the arguments. 
+  subroutine test6_bis (n, a, b)
+    integer :: n
+    real, dimension (n, n) :: a, b
+    call ext3 (transpose (a), transpose (b))
+  end subroutine test6_bis
+
+  ! No temporary is neede here; the second argument is intent(in).
+  subroutine test7 (n, a)
+    integer :: n
+    real, dimension (n, n) :: a
+    call ext1 (transpose (a), a)
+  end subroutine test7
+
+  ! The temporary is needed here though.
+  subroutine test8 (n, a)
+    integer :: n
+    real, dimension (n, n) :: a
+    call ext2 (transpose (a), a)        ! { dg-warning "Creating array temporary" } 
+  end subroutine test8
+
+  ! Silly, but we don't need any temporaries here.
+  subroutine test9 (n, a)
+    integer :: n
+    real, dimension (n, n) :: a
+    call ext1 (transpose (transpose (a)), a)
+  end subroutine test9
+
+  ! The outer transpose needs a temporary; the inner one doesn't.
+  subroutine test10 (n, a)
+    integer :: n
+    real, dimension (n, n) :: a
+    call ext2 (transpose (transpose (a)), a)    ! { dg-warning "Creating array temporary" }
+  end subroutine test10
+end module foo
+
+! { dg-final { scan-tree-dump-times "struct\[^\\n\]*atmp" 4 "original" } }
+! { dg-final { cleanup-tree-dump "original" } }

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Patch, fortran] [4/5] PR 45648: Inline transpose part 2:  Remove ss lookup
  2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
  2010-09-20 22:40 ` [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds Mikael Morin
  2010-09-20 23:27 ` [Patch, fortran] [5/5] PR 45648: Inline transpose part 2: Do dependency analysis in case of transpose optimization Mikael Morin
@ 2010-09-20 23:45 ` Mikael Morin
  2010-09-21 14:34   ` Paul Richard Thomas
  2010-09-21  0:01 ` [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization Mikael Morin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Mikael Morin @ 2010-09-20 23:45 UTC (permalink / raw)
  To: gfortran, patches

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

This one is optional. 
I noticed while working on these patches that there were loops to get the ss corresponding to the expr to be evaluated. This is a contradiction with other places in the code where se->ss is supposed to be in sync (this is the point of gfc_advance_se_ss_chain) with the expression to be calculated (grep "ss->expr == expr"). Furthermore, the first half of the function was using secss (with lookup) and the second ss (without).
Thus, this removes the ss lookup and replaces the asserts with their equivalent without lookup. There is no behavioural change and this is not directly related to the patchset, so this is optional. 

OK for trunk?   



[-- Attachment #2: inline_transpose_2-4.changelog --]
[-- Type: text/plain, Size: 141 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	* trans-array.c (gfc_conv_expr_descriptor): Remove ss lookup.
	Update asserts accordingly.


[-- Attachment #3: inline_transpose_2-4.diff --]
[-- Type: text/plain, Size: 4427 bytes --]

diff --git a/trans-array.c b/trans-array.c
index 9171183..947ed4b 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5164,7 +5164,6 @@ void
 gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 {
   gfc_loopinfo loop;
-  gfc_ss *secss;
   gfc_ss_info *info;
   int need_tmp;
   int n;
@@ -5177,6 +5176,7 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
   bool subref_array_target = false;
   gfc_expr *arg;
 
+  gcc_assert (ss != NULL);
   gcc_assert (ss != gfc_ss_terminator);
 
   /* Special case things we know we can pass easily.  */
@@ -5186,16 +5186,12 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       /* If we have a linear array section, we can pass it directly.
 	 Otherwise we need to copy it into a temporary.  */
 
-      /* Find the SS for the array section.  */
-      secss = ss;
-      while (secss != gfc_ss_terminator && secss->type != GFC_SS_SECTION)
-	secss = secss->next;
-
-      gcc_assert (secss != gfc_ss_terminator);
-      info = &secss->data.info;
+      gcc_assert (ss->type == GFC_SS_SECTION);
+      gcc_assert (ss->expr == expr);
+      info = &ss->data.info;
 
       /* Get the descriptor for the array.  */
-      gfc_conv_ss_descriptor (&se->pre, secss, 0);
+      gfc_conv_ss_descriptor (&se->pre, ss, 0);
       desc = info->descriptor;
 
       subref_array_target = se->direct_byref && is_subref_array (expr);
@@ -5271,26 +5267,28 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	 array descriptor.  We still need to go through the scalarizer
 	 to create the descriptor.  Elemental functions ar handled as
 	 arbitrary expressions, i.e. copy to a temporary.  */
-      secss = ss;
-      /* Look for the SS for this function.  */
-      while (secss != gfc_ss_terminator
-	     && (secss->type != GFC_SS_FUNCTION || secss->expr != expr))
-      	secss = secss->next;
 
       if (se->direct_byref)
 	{
-	  gcc_assert (secss != gfc_ss_terminator);
+	  gcc_assert (ss->type == GFC_SS_FUNCTION && ss->expr == expr);
 
 	  /* For pointer assignments pass the descriptor directly.  */
-	  se->ss = secss;
+	  if (se->ss == NULL)
+	    se->ss = ss;
+	  else
+	    gcc_assert (se->ss == ss);
 	  se->expr = gfc_build_addr_expr (NULL_TREE, se->expr);
 	  gfc_conv_expr (se, expr);
 	  return;
 	}
 
-      if (secss == gfc_ss_terminator)
+      if (ss->expr != expr)
 	{
 	  /* Elemental function.  */
+	  gcc_assert ((expr->value.function.esym != NULL
+		       && expr->value.function.esym->attr.elemental)
+		      || (expr->value.function.isym != NULL
+			  && expr->value.function.isym->elemental));
 	  need_tmp = 1;
 	  if (expr->ts.type == BT_CHARACTER
 		&& expr->ts.u.cl->length->expr_type != EXPR_CONSTANT)
@@ -5301,7 +5299,7 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       else
 	{
 	  /* Transformational function.  */
-	  info = &secss->data.info;
+	  info = &ss->data.info;
 	  need_tmp = 0;
 	}
       break;
@@ -5314,12 +5312,10 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	{
 	  need_tmp = 0;
 	  info = &ss->data.info;
-	  secss = ss;
 	}
       else
 	{
 	  need_tmp = 1;
-	  secss = NULL;
 	  info = NULL;
 	}
       break;
@@ -5327,7 +5323,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
     default:
       /* Something complicated.  Copy it into a temporary.  */
       need_tmp = 1;
-      secss = NULL;
       info = NULL;
       break;
     }
@@ -5443,7 +5438,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	se->string_length =  gfc_get_expr_charlen (expr);
 
       desc = info->descriptor;
-      gcc_assert (secss && secss != gfc_ss_terminator);
       if (se->direct_byref && !se->byref_noassign)
 	{
 	  /* For pointer assignments we fill in the destination.  */
@@ -5465,7 +5459,7 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       /* The following can be somewhat confusing.  We have two
          descriptors, a new one and the original array.
          {parm, parmtype, dim} refer to the new one.
-         {desc, type, n, secss, loop} refer to the original, which maybe
+         {desc, type, n, loop} refer to the original, which maybe
          a descriptorless array.
          The bounds of the scalarization are the bounds of the section.
          We don't have to worry about numeric overflows when calculating

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization
  2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
                   ` (2 preceding siblings ...)
  2010-09-20 23:45 ` [Patch, fortran] [4/5] PR 45648: Inline transpose part 2: Remove ss lookup Mikael Morin
@ 2010-09-21  0:01 ` Mikael Morin
  2010-09-21 14:30   ` Paul Richard Thomas
  2010-12-09  6:51   ` H.J. Lu
  2010-09-21  2:05 ` [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array Mikael Morin
  2010-12-09  5:24 ` [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 H.J. Lu
  5 siblings, 2 replies; 25+ messages in thread
From: Mikael Morin @ 2010-09-21  0:01 UTC (permalink / raw)
  To: gfortran, patches

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

With this, the transpose optimization is back.
The two previous patches permitted to call gfc_conv_expr_descriptor on transpose's arg, so now we just have to bypass the temporary generation in the transpose case. 
I don't add the testcase from the original commit as there is a wrong code regression introduced by this patch (uncaught by the testsuite) to be fixed in patch 5/5. 

OK for trunk ?



[-- Attachment #2: inline_transpose_2-3.changelog --]
[-- Type: text/plain, Size: 163 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* trans-array.c (gfc_conv_expr_descriptor): Special case noncopying
	intrinsic function call. 


[-- Attachment #3: inline_transpose_2-3.diff --]
[-- Type: text/plain, Size: 1140 bytes --]

diff --git a/trans-array.c b/trans-array.c
index 52e6d2a..9171183 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5175,6 +5175,7 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
   tree offset;
   int full;
   bool subref_array_target = false;
+  gfc_expr *arg;
 
   gcc_assert (ss != gfc_ss_terminator);
 
@@ -5253,6 +5254,19 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       break;
       
     case EXPR_FUNCTION:
+
+      /* We don't need to copy data in some cases.  */
+      arg = gfc_get_noncopying_intrinsic_argument (expr);
+      if (arg)
+	{
+	  /* This is a call to transpose...  */
+	  gcc_assert (expr->value.function.isym->id == GFC_ISYM_TRANSPOSE);
+	  /* ... which has already been handled by the scalarizer, so
+	     that we just need to get its argument's descriptor.  */
+	  gfc_conv_expr_descriptor (se, expr->value.function.actual->expr, ss);
+	  return;
+	}
+
       /* A transformational function return value will be a temporary
 	 array descriptor.  We still need to go through the scalarizer
 	 to create the descriptor.  Elemental functions ar handled as

[-- Attachment #4: inline_transpose_2_testsuite-3.changelog --]
[-- Type: text/plain, Size: 186 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* gfortran.dg/inline_transpose_1.f90: Update temporary's locations
	and counts. Add non-elemental function call check.

[-- Attachment #5: inline_transpose_2_testsuite-3.diff --]
[-- Type: text/plain, Size: 4295 bytes --]

diff --git a/inline_transpose_1.f90 b/inline_transpose_1.f90
index 50290c6..36198f8 100644
--- a/inline_transpose_1.f90
+++ b/inline_transpose_1.f90
@@ -61,10 +61,10 @@
   if (u /= v) call abort
 
 
-  a = foo(transpose(c)) ! Unnecessary { dg-warning "Creating array temporary" }
+  a = foo(transpose(c))
   if (any(a /= p+1)) call abort
 
-  write(u,*) foo(transpose(c))    ! 2 temps, should be 1 { dg-warning "Creating array temporary" }
+  write(u,*) foo(transpose(c))    ! { dg-warning "Creating array temporary" }
   write(v,*) p+1
   if (u /= v) call abort
 
@@ -77,10 +77,10 @@
   if (u /= v) call abort
 
 
-  e = foo(transpose(e))     ! 2 temps, should be 1 { dg-warning "Creating array temporary" }
+  e = foo(transpose(e))     ! { dg-warning "Creating array temporary" }
   if (any(e /= 2*s+1)) call abort
 
-  write(u,*) transpose(foo(transpose(e))-1)     ! 2 temps, should be 1 { dg-warning "Creating array temporary" }
+  write(u,*) transpose(foo(transpose(e))-1)     ! { dg-warning "Creating array temporary" }
   write(v,*) 2*s+1
   if (u /= v) call abort
 
@@ -141,27 +141,32 @@
   if (u /= v) call abort
 
 
-  if (any(transpose(matmul(a,c)) /= matmul(transpose(c), transpose(a)))) call abort      ! 4 temps, should be 2 { dg-warning "Creating array temporary" }
+  if (any(transpose(matmul(a,c)) /= matmul(transpose(c), transpose(a)))) call abort      ! 2 temps { dg-warning "Creating array temporary" }
 
   write(u,*) transpose(matmul(a,c))     ! { dg-warning "Creating array temporary" }
-  write(v,*) matmul(transpose(c), transpose(a))     ! 3 temps, should be 1 { dg-warning "Creating array temporary" }
+  write(v,*) matmul(transpose(c), transpose(a))     ! { dg-warning "Creating array temporary" }
   if (u /= v) call abort
 
 
-  if (any(transpose(matmul(e,a)) /= matmul(transpose(a), transpose(e)))) call abort     ! 4 temps, should be 2 { dg-warning "Creating array temporary" }
+  if (any(transpose(matmul(e,a)) /= matmul(transpose(a), transpose(e)))) call abort     ! 2 temps { dg-warning "Creating array temporary" }
 
   write(u,*) transpose(matmul(e,a))     ! { dg-warning "Creating array temporary" }
-  write(v,*) matmul(transpose(a), transpose(e))     ! 3 temps, should be 1 { dg-warning "Creating array temporary" }
+  write(v,*) matmul(transpose(a), transpose(e))     ! { dg-warning "Creating array temporary" }
   if (u /= v) call abort
 
 
-  call baz (transpose(a))       ! Unnecessary { dg-warning "Creating array temporary" }
+  call baz (transpose(a))
 
-  call toto (f, transpose (e))
-  if (any (f /= 4 * s + 12)) call abort
 
-  call toto (f, transpose (f))          ! { dg-warning "Creating array temporary" }
-  if (any (f /= 8 * r + 24)) call abort
+  call toto1 (a, transpose (c))
+  if (any (a /= 2 * p + 12)) call abort
+
+  call toto1 (e, transpose (e))          ! { dg-warning "Creating array temporary" }
+  if (any (e /= 4 * s + 12)) call abort
+
+
+  call toto2 (c, transpose (a))
+  if (any (c /= 2 * q + 13)) call abort
 
 
   contains
@@ -182,23 +187,30 @@
     integer, intent(in) :: x(:,:)
   end subroutine baz
 
-  elemental subroutine toto (x, y)
+  elemental subroutine toto1 (x, y)
     integer, intent(out) :: x
     integer, intent(in)  :: y
     x = y + y
-  end subroutine toto
+  end subroutine toto1
+
+  subroutine toto2 (x, y)
+    integer, dimension(:,:), intent(out) :: x
+    integer, dimension(:,:), intent(in)  :: y
+    x = y + 1
+  end subroutine toto2
 
 end
+
 ! No call to transpose
 ! { dg-final { scan-tree-dump-times "_gfortran_transpose" 0 "original" } }
 !
-! 34 temporaries
-! { dg-final { scan-tree-dump-times "struct\[^\\n\]*atmp" 34 "original" } }
+! 21 temporaries
+! { dg-final { scan-tree-dump-times "struct\[^\\n\]*atmp" 21 "original" } }
 !
 ! 2 tests optimized out
-! { dg-final { scan-tree-dump-times "_gfortran_abort" 34 "original" } }
-! { # Commented out as failing at -O0: dg-final { scan-tree-dump-times "_gfortran_abort" 32 "optimized" } }
+! { dg-final { scan-tree-dump-times "_gfortran_abort" 35 "original" } }
+! { # Commented out as failing at -O0: dg-final { scan-tree-dump-times "_gfortran_abort" 33 "optimized" } }
 !
 ! cleanup
-! { #dg-final { cleanup-tree-dump "original" } }
+! { dg-final { cleanup-tree-dump "original" } }
 ! { dg-final { cleanup-tree-dump "optimized" } }

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array
  2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
                   ` (3 preceding siblings ...)
  2010-09-21  0:01 ` [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization Mikael Morin
@ 2010-09-21  2:05 ` Mikael Morin
  2010-09-21 14:14   ` Paul Richard Thomas
  2010-12-09  5:24 ` [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 H.J. Lu
  5 siblings, 1 reply; 25+ messages in thread
From: Mikael Morin @ 2010-09-21  2:05 UTC (permalink / raw)
  To: gfortran, patches

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

We have to take into account the fact that we can access an array transposed when checking whether an array reference is a reference to a full array. 

OK for trunk ?



[-- Attachment #2: inline_transpose_2-2.changelog --]
[-- Type: text/plain, Size: 175 bytes --]

2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* trans-array.c (gfc_conv_expr_descriptor): Unset full if we are
	accessing dimensions in reversed order. 


[-- Attachment #3: inline_transpose_2-2.diff --]
[-- Type: text/plain, Size: 465 bytes --]

diff --git a/trans-array.c b/trans-array.c
index f15b53e..52e6d2a 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5216,6 +5216,14 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	full = gfc_full_array_ref_p (info->ref, NULL);
 
       if (full)
+	for (n = 0; n < info->dimen; n++)
+	  if (info->dim[n] != n)
+	    {
+	      full = 0;
+	      break;
+	    }
+
+      if (full)
 	{
 	  if (se->direct_byref && !se->byref_noassign)
 	    {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-20 22:40 ` [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds Mikael Morin
@ 2010-09-21 14:09   ` Paul Richard Thomas
  2010-09-21 14:46     ` Daniel Kraft
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 14:09 UTC (permalink / raw)
  To: Mikael Morin; +Cc: gfortran, patches

Mikael,

This is OK except that I do not understand what the gcc_assert is doing in:
+
+	  /* look for the corresponding scalarizer dimension: dim.  */
+	  for (dim = 0; dim < ndim; dim++)
+	    if (info->dim[dim] == n)
+	      break;
+	  gcc_assert (dim < ndim);

Have I missed something?

Cheers

Paul

On 9/21/10, Mikael Morin <mikael.morin@sfr.fr> wrote:
> We can't have dim (new descriptor's dimension) increasing together with n
> (original array's dimension) if we want to support transposed reference. We
> have to get dim such that info->dim[dim] == n. Then we can set the
> descriptor bounds along dim.
>
> OK for trunk?
>
>
>


-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array
  2010-09-21  2:05 ` [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array Mikael Morin
@ 2010-09-21 14:14   ` Paul Richard Thomas
  2010-09-21 17:11     ` Mikael Morin
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 14:14 UTC (permalink / raw)
  To: Mikael Morin; +Cc: gfortran, patches

Mikael,

This is OK.

What would be the consequence of putting the additional code in
gfc_full_array_ref_p?

Cheers

Paul

On 9/21/10, Mikael Morin <mikael.morin@sfr.fr> wrote:
> We have to take into account the fact that we can access an array transposed
> when checking whether an array reference is a reference to a full array.
>
> OK for trunk ?
>
>
>


-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization
  2010-09-21  0:01 ` [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization Mikael Morin
@ 2010-09-21 14:30   ` Paul Richard Thomas
  2010-12-09  6:51   ` H.J. Lu
  1 sibling, 0 replies; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 14:30 UTC (permalink / raw)
  To: Mikael Morin; +Cc: gfortran, patches

Mikael,

This is also OK.

Paul

On 9/21/10, Mikael Morin <mikael.morin@sfr.fr> wrote:
> With this, the transpose optimization is back.
> The two previous patches permitted to call gfc_conv_expr_descriptor on
> transpose's arg, so now we just have to bypass the temporary generation in
> the transpose case.
> I don't add the testcase from the original commit as there is a wrong code
> regression introduced by this patch (uncaught by the testsuite) to be fixed
> in patch 5/5.
>
> OK for trunk ?
>
>
>


-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [4/5] PR 45648: Inline transpose part 2: Remove ss lookup
  2010-09-20 23:45 ` [Patch, fortran] [4/5] PR 45648: Inline transpose part 2: Remove ss lookup Mikael Morin
@ 2010-09-21 14:34   ` Paul Richard Thomas
  0 siblings, 0 replies; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 14:34 UTC (permalink / raw)
  To: Mikael Morin; +Cc: gfortran, patches

Mikael,

This is OK.

Cheers

Paul

On 9/21/10, Mikael Morin <mikael.morin@sfr.fr> wrote:
> This one is optional.
> I noticed while working on these patches that there were loops to get the ss
> corresponding to the expr to be evaluated. This is a contradiction with
> other places in the code where se->ss is supposed to be in sync (this is the
> point of gfc_advance_se_ss_chain) with the expression to be calculated (grep
> "ss->expr == expr"). Furthermore, the first half of the function was using
> secss (with lookup) and the second ss (without).
> Thus, this removes the ss lookup and replaces the asserts with their
> equivalent without lookup. There is no behavioural change and this is not
> directly related to the patchset, so this is optional.
>
> OK for trunk?
>
>
>


-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 14:09   ` Paul Richard Thomas
@ 2010-09-21 14:46     ` Daniel Kraft
  2010-09-21 15:04       ` Paul Richard Thomas
  0 siblings, 1 reply; 25+ messages in thread
From: Daniel Kraft @ 2010-09-21 14:46 UTC (permalink / raw)
  To: Paul Richard Thomas; +Cc: Mikael Morin, gfortran, patches

Paul Richard Thomas wrote:
> Mikael,
> 
> This is OK except that I do not understand what the gcc_assert is doing in:
> +
> +	  /* look for the corresponding scalarizer dimension: dim.  */
> +	  for (dim = 0; dim < ndim; dim++)
> +	    if (info->dim[dim] == n)
> +	      break;
> +	  gcc_assert (dim < ndim);
> 
> Have I missed something?

This seems like checking the loop is exited early in any case.  (But 
I've no idea about the rest of the patch.)

Daniel

-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 14:46     ` Daniel Kraft
@ 2010-09-21 15:04       ` Paul Richard Thomas
  2010-09-21 15:06         ` Paul Richard Thomas
                           ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 15:04 UTC (permalink / raw)
  To: Daniel Kraft; +Cc: Mikael Morin, gfortran, patches

Dominique,

The loop takes dim from 0 to ndim-1.  Therefore the condition of the
gcc_assert is always going to be satisfied.

Cheers

Paul

On 9/21/10, Daniel Kraft <d@domob.eu> wrote:
> Paul Richard Thomas wrote:
>> Mikael,
>>
>> This is OK except that I do not understand what the gcc_assert is doing
>> in:
>> +
>> +	  /* look for the corresponding scalarizer dimension: dim.  */
>> +	  for (dim = 0; dim < ndim; dim++)
>> +	    if (info->dim[dim] == n)
>> +	      break;
>> +	  gcc_assert (dim < ndim);
>>
>> Have I missed something?
>
> This seems like checking the loop is exited early in any case.  (But
> I've no idea about the rest of the patch.)
>
> Daniel
>
> --
> http://www.pro-vegan.info/
> --
> Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
> To go: Hea-Mon-Pri
>


-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 15:04       ` Paul Richard Thomas
@ 2010-09-21 15:06         ` Paul Richard Thomas
  2010-09-21 16:08         ` Richard Guenther
  2010-09-21 16:09         ` Tobias Burnus
  2 siblings, 0 replies; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 15:06 UTC (permalink / raw)
  To: Daniel Kraft; +Cc: Mikael Morin, gfortran, patches

s/Dominique/Daniel/ :-)

Paul

On 9/21/10, Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> Dominique,
>
> The loop takes dim from 0 to ndim-1.  Therefore the condition of the
> gcc_assert is always going to be satisfied.
>
> Cheers
>
> Paul
>
> On 9/21/10, Daniel Kraft <d@domob.eu> wrote:
>> Paul Richard Thomas wrote:
>>> Mikael,
>>>
>>> This is OK except that I do not understand what the gcc_assert is doing
>>> in:
>>> +
>>> +	  /* look for the corresponding scalarizer dimension: dim.  */
>>> +	  for (dim = 0; dim < ndim; dim++)
>>> +	    if (info->dim[dim] == n)
>>> +	      break;
>>> +	  gcc_assert (dim < ndim);
>>>
>>> Have I missed something?
>>
>> This seems like checking the loop is exited early in any case.  (But
>> I've no idea about the rest of the patch.)
>>
>> Daniel
>>
>> --
>> http://www.pro-vegan.info/
>> --
>> Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
>> To go: Hea-Mon-Pri
>>
>
>
> --
> The knack of flying is learning how to throw yourself at the ground and
> miss.
>        --Hitchhikers Guide to the Galaxy
>


-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 15:04       ` Paul Richard Thomas
  2010-09-21 15:06         ` Paul Richard Thomas
@ 2010-09-21 16:08         ` Richard Guenther
  2010-09-21 16:09         ` Tobias Burnus
  2 siblings, 0 replies; 25+ messages in thread
From: Richard Guenther @ 2010-09-21 16:08 UTC (permalink / raw)
  To: Paul Richard Thomas; +Cc: Daniel Kraft, Mikael Morin, gfortran, patches

On Tue, Sep 21, 2010 at 4:46 PM, Paul Richard Thomas
<paul.richard.thomas@gmail.com> wrote:
> Dominique,
>
> The loop takes dim from 0 to ndim-1.  Therefore the condition of the
> gcc_assert is always going to be satisfied.

But if the loop terminates via its exit condition !(dim<ndim) then
obviously dim will be not < ndim ;)

RIchard.

> Cheers
>
> Paul
>
> On 9/21/10, Daniel Kraft <d@domob.eu> wrote:
>> Paul Richard Thomas wrote:
>>> Mikael,
>>>
>>> This is OK except that I do not understand what the gcc_assert is doing
>>> in:
>>> +
>>> +      /* look for the corresponding scalarizer dimension: dim.  */
>>> +      for (dim = 0; dim < ndim; dim++)
>>> +        if (info->dim[dim] == n)
>>> +          break;
>>> +      gcc_assert (dim < ndim);
>>>
>>> Have I missed something?
>>
>> This seems like checking the loop is exited early in any case.  (But
>> I've no idea about the rest of the patch.)
>>
>> Daniel
>>
>> --
>> http://www.pro-vegan.info/
>> --
>> Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
>> To go: Hea-Mon-Pri
>>
>
>
> --
> The knack of flying is learning how to throw yourself at the ground and miss.
>       --Hitchhikers Guide to the Galaxy
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 15:04       ` Paul Richard Thomas
  2010-09-21 15:06         ` Paul Richard Thomas
  2010-09-21 16:08         ` Richard Guenther
@ 2010-09-21 16:09         ` Tobias Burnus
  2010-09-21 16:19           ` Paul Richard Thomas
  2 siblings, 1 reply; 25+ messages in thread
From: Tobias Burnus @ 2010-09-21 16:09 UTC (permalink / raw)
  To: Paul Richard Thomas; +Cc: Daniel Kraft, Mikael Morin, gfortran, patches

  On 09/21/2010 04:46 PM, Paul Richard Thomas wrote:
> The loop takes dim from 0 to ndim-1.  Therefore the condition of the
> gcc_assert is always going to be satisfied.

I disagree. The assert triggers

a) If ndim < 1 (and the loop is never entered)

b) If one does not have an early exit - as Daniel pointed out:

#include <stdio.h>
main () {
  int i;
  for (i = 0; i < 5; i++)
    ;
  printf("%d\n", i);
  return 0;
}

Running the program prints "5".

Tobias

> On 9/21/10, Daniel Kraft<d@domob.eu>  wrote:
>> Paul Richard Thomas wrote:
>>> +	  for (dim = 0; dim<  ndim; dim++)
>>> +	    if (info->dim[dim] == n)
>>> +	      break;
>>> +	  gcc_assert (dim<  ndim);
>> This seems like checking the loop is exited early in any case.  (But
>> I've no idea about the rest of the patch.)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 16:09         ` Tobias Burnus
@ 2010-09-21 16:19           ` Paul Richard Thomas
  2010-09-21 17:07             ` Mikael Morin
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 16:19 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: Daniel Kraft, Mikael Morin, gfortran, patches

Dear All,


> b) If one does not have an early exit - as Daniel pointed out:

duuuuh!

Paul

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 16:19           ` Paul Richard Thomas
@ 2010-09-21 17:07             ` Mikael Morin
  2010-09-21 17:25               ` Mikael Morin
  2010-09-21 17:39               ` Steve Kargl
  0 siblings, 2 replies; 25+ messages in thread
From: Mikael Morin @ 2010-09-21 17:07 UTC (permalink / raw)
  To: fortran; +Cc: Paul Richard Thomas, Tobias Burnus, Daniel Kraft, patches

On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
> Dear All,
> 
> > b) If one does not have an early exit - as Daniel pointed out:
> duuuuh!
> 
> Paul

Hello all,

yes, the assert checks the loop was exited early, and thus the dim looked for 
has been found. 
Would there be a way to present this more clearly ? How ?

Mikael

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array
  2010-09-21 14:14   ` Paul Richard Thomas
@ 2010-09-21 17:11     ` Mikael Morin
  0 siblings, 0 replies; 25+ messages in thread
From: Mikael Morin @ 2010-09-21 17:11 UTC (permalink / raw)
  To: fortran; +Cc: Paul Richard Thomas, patches

On Tuesday 21 September 2010 15:50:55 Paul Richard Thomas wrote:
> Mikael,
> 
> This is OK.
> 
> What would be the consequence of putting the additional code in
> gfc_full_array_ref_p?

Hello, 

this is not possible (at least with the current code) as info->ref doesn't 
have the order in which dimensions are accessed (which is in info->dim). 
Besides, there is a distinction between the array itself which is full and 
transpose(array) which is not. Thus, I think it is better to keep the code 
separated. 

Thanks for the review.
Mikael

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 17:07             ` Mikael Morin
@ 2010-09-21 17:25               ` Mikael Morin
  2010-09-21 17:39               ` Steve Kargl
  1 sibling, 0 replies; 25+ messages in thread
From: Mikael Morin @ 2010-09-21 17:25 UTC (permalink / raw)
  To: fortran; +Cc: Paul Richard Thomas, Tobias Burnus, Daniel Kraft, patches

On Tuesday 21 September 2010 17:41:15 Mikael Morin wrote:
> On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
> > Dear All,
> > 
> > > b) If one does not have an early exit - as Daniel pointed out:
> > duuuuh!
> > 
> > Paul
> 
> Hello all,
> 
> yes, the assert checks the loop was exited early, and thus the dim looked
> for has been found.
> Would there be a way to present this more clearly ? How ?
I will add a comment.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [5/5] PR 45648: Inline transpose part 2: Do dependency analysis in case of transpose optimization.
       [not found]   ` <AANLkTi=bzqmABzx8vW8Kg-m7_qRnk4g=fTXHrpRkvot4@mail.gmail.com>
@ 2010-09-21 17:30     ` Mikael Morin
  2010-09-21 22:22       ` Mikael Morin
  0 siblings, 1 reply; 25+ messages in thread
From: Mikael Morin @ 2010-09-21 17:30 UTC (permalink / raw)
  To: Paul Richard Thomas; +Cc: fortran, patches

On Tuesday 21 September 2010 16:05:33 Paul Richard Thomas wrote:
> Mikael,
> 
> The complete patch and the testcase are OK for trunk.  Please commit
> the whole lot in one go.
Will do.
Thanks for the reviews.

Mikael

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 17:07             ` Mikael Morin
  2010-09-21 17:25               ` Mikael Morin
@ 2010-09-21 17:39               ` Steve Kargl
  2010-09-21 19:04                 ` Paul Richard Thomas
  1 sibling, 1 reply; 25+ messages in thread
From: Steve Kargl @ 2010-09-21 17:39 UTC (permalink / raw)
  To: Mikael Morin
  Cc: fortran, Paul Richard Thomas, Tobias Burnus, Daniel Kraft, patches

On Tue, Sep 21, 2010 at 05:41:15PM +0200, Mikael Morin wrote:
> On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
> > Dear All,
> > 
> > > b) If one does not have an early exit - as Daniel pointed out:
> > duuuuh!
> > 
> > Paul
> 
> Hello all,
> 
> yes, the assert checks the loop was exited early, and thus the dim looked for 
> has been found. 
> Would there be a way to present this more clearly ? How ?
> 

Well, C has a feature called a comment.

   /* Assert that the loop exited early. */
   gcc_assert (...);

:-)

-- 
Steve

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds
  2010-09-21 17:39               ` Steve Kargl
@ 2010-09-21 19:04                 ` Paul Richard Thomas
  0 siblings, 0 replies; 25+ messages in thread
From: Paul Richard Thomas @ 2010-09-21 19:04 UTC (permalink / raw)
  To: Steve Kargl; +Cc: Mikael Morin, fortran, Tobias Burnus, Daniel Kraft, patches

Steve, Mikael,

I am not sure that it is necessary to compensate for my suffering
temporary brain failure :-)  It is about as obvious as can be.....

Cheers

Paul

On Tue, Sep 21, 2010 at 6:10 PM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Tue, Sep 21, 2010 at 05:41:15PM +0200, Mikael Morin wrote:
>> On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
>> > Dear All,
>> >
>> > > b) If one does not have an early exit - as Daniel pointed out:
>> > duuuuh!
>> >
>> > Paul
>>
>> Hello all,
>>
>> yes, the assert checks the loop was exited early, and thus the dim looked for
>> has been found.
>> Would there be a way to present this more clearly ? How ?
>>
>
> Well, C has a feature called a comment.
>
>   /* Assert that the loop exited early. */
>   gcc_assert (...);
>
> :-)
>
> --
> Steve
>



-- 
The knack of flying is learning how to throw yourself at the ground and miss.
       --Hitchhikers Guide to the Galaxy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [5/5] PR 45648: Inline transpose part 2: Do dependency analysis in case of transpose optimization.
  2010-09-21 17:30     ` Mikael Morin
@ 2010-09-21 22:22       ` Mikael Morin
  0 siblings, 0 replies; 25+ messages in thread
From: Mikael Morin @ 2010-09-21 22:22 UTC (permalink / raw)
  To: fortran; +Cc: Paul Richard Thomas, patches

On Tuesday 21 September 2010 18:07:28 Mikael Morin wrote:
> On Tuesday 21 September 2010 16:05:33 Paul Richard Thomas wrote:
> > Mikael,
> > 
> > The complete patch and the testcase are OK for trunk.  Please commit
> > the whole lot in one go.
> 
> Will do.
> Thanks for the reviews.
> 
> Mikael

Committed at revision 164494.

For the assert concern, I added the following, just in case someone has a 
temporary brain failure one day. 

diff --git a/trans-array.c b/trans-array.c
index 1bb4429..310a42b 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5535,6 +5535,8 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, 
gf
          for (dim = 0; dim < ndim; dim++)
            if (info->dim[dim] == n)
              break;
+
+         /* loop exited early: the DIM being looked for has been found.  */
          gcc_assert (dim < ndim);
 
          /* Set the new lower bound.  */

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [0/5] PR 45648: Inline transpose part 2
  2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
                   ` (4 preceding siblings ...)
  2010-09-21  2:05 ` [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array Mikael Morin
@ 2010-12-09  5:24 ` H.J. Lu
  5 siblings, 0 replies; 25+ messages in thread
From: H.J. Lu @ 2010-12-09  5:24 UTC (permalink / raw)
  To: Mikael Morin; +Cc: gfortran, patches

On Mon, Sep 20, 2010 at 3:21 PM, Mikael Morin <mikael.morin@sfr.fr> wrote:
> Hello,
>
> this is the second part of the inline transpose patch, re-enabling the
> non-copying transposed descriptor optimization.
>
> Allmost all happens in gfc_conv_expr_descriptor:
> 1/5: We use the scalarizer's dim array to get the right bounds while creating the descriptor.
> 2/5: This prevents a regression in ret_array_1 where a transposed array was handled as a full array with patch 3/5 alone.
> 3/5: This bypasses the temporary generation in gfc_conv_expr_descriptor in the transpose case.
> 4/5: It is a cleanup I made while working on this, but it is somewhat unrelated, and optional actually.
> 5/5: This forces the creation of a temporary in case there is an alias between actual arguments (corrects a wrong code regression introduced by patch 3/5).
>
> Each patch is regression tested on an older revision and all together on today's trunk.
> I will ask for trunk commital for each one of them.
>

This caused:

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


-- 
H.J.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization
  2010-09-21  0:01 ` [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization Mikael Morin
  2010-09-21 14:30   ` Paul Richard Thomas
@ 2010-12-09  6:51   ` H.J. Lu
  1 sibling, 0 replies; 25+ messages in thread
From: H.J. Lu @ 2010-12-09  6:51 UTC (permalink / raw)
  To: Mikael Morin; +Cc: gfortran, patches

On Mon, Sep 20, 2010 at 3:21 PM, Mikael Morin <mikael.morin@sfr.fr> wrote:
> With this, the transpose optimization is back.
> The two previous patches permitted to call gfc_conv_expr_descriptor on transpose's arg, so now we just have to bypass the temporary generation in the transpose case.
> I don't add the testcase from the original commit as there is a wrong code regression introduced by this patch (uncaught by the testsuite) to be fixed in patch 5/5.
>
> OK for trunk ?
>

This change is unsafe and caused:

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


-- 
H.J.

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2010-12-09  5:49 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-20 22:21 [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 Mikael Morin
2010-09-20 22:40 ` [Patch, fortran] [1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds Mikael Morin
2010-09-21 14:09   ` Paul Richard Thomas
2010-09-21 14:46     ` Daniel Kraft
2010-09-21 15:04       ` Paul Richard Thomas
2010-09-21 15:06         ` Paul Richard Thomas
2010-09-21 16:08         ` Richard Guenther
2010-09-21 16:09         ` Tobias Burnus
2010-09-21 16:19           ` Paul Richard Thomas
2010-09-21 17:07             ` Mikael Morin
2010-09-21 17:25               ` Mikael Morin
2010-09-21 17:39               ` Steve Kargl
2010-09-21 19:04                 ` Paul Richard Thomas
2010-09-20 23:27 ` [Patch, fortran] [5/5] PR 45648: Inline transpose part 2: Do dependency analysis in case of transpose optimization Mikael Morin
     [not found]   ` <AANLkTi=bzqmABzx8vW8Kg-m7_qRnk4g=fTXHrpRkvot4@mail.gmail.com>
2010-09-21 17:30     ` Mikael Morin
2010-09-21 22:22       ` Mikael Morin
2010-09-20 23:45 ` [Patch, fortran] [4/5] PR 45648: Inline transpose part 2: Remove ss lookup Mikael Morin
2010-09-21 14:34   ` Paul Richard Thomas
2010-09-21  0:01 ` [Patch, fortran] [3/5] PR 45648: Inline transpose part 2: Enable transpose optimization Mikael Morin
2010-09-21 14:30   ` Paul Richard Thomas
2010-12-09  6:51   ` H.J. Lu
2010-09-21  2:05 ` [Patch, fortran] [2/5] PR 45648: Inline transpose part 2: Take transposed dimensions into account whilst checking for full array Mikael Morin
2010-09-21 14:14   ` Paul Richard Thomas
2010-09-21 17:11     ` Mikael Morin
2010-12-09  5:24 ` [Patch, fortran] [0/5] PR 45648: Inline transpose part 2 H.J. Lu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).