Patchwork [Fortran] PR56907 - do not 'pack' arrays passed to C_LOC

login
register
mail settings
Submitter Tobias Burnus
Date April 10, 2013, 8:17 p.m.
Message ID <5165C8D0.7020606@net-b.de>
Download mbox | patch
Permalink /patch/235468/
State New
Headers show

Comments

Tobias Burnus - April 10, 2013, 8:17 p.m.
Fortran 2008 supports  C_LOC(array); if the argument is not simply 
contiguous, the current code adds a call to __gfortran_intrinsic_pack.

The pack call shouldn't be there. Fortran 2008 demands that the actual 
argument is contiguous and intrinsic_pack copy creates a copy if the 
run-time check shows that the argument is not contiguous. Thus, it is 
not a wrong-code issue. However, for performance reasons, it makes sense 
to avoid the call __gfortran_intrinsic_pack.

Build and regtested on x86-64-gnu-linux.
OK for the trunk?

Tobias
Tobias Burnus - April 15, 2013, 8:52 p.m.
*ping*

Also pending is the NO_ARGS_CHECK patch at: 
http://gcc.gnu.org/ml/fortran/2013-04/msg00120.html

On April 10, Tobias Burnus wrote:
> Fortran 2008 supports  C_LOC(array); if the argument is not simply 
> contiguous, the current code adds a call to __gfortran_intrinsic_pack.
>
> The pack call shouldn't be there. Fortran 2008 demands that the actual 
> argument is contiguous and intrinsic_pack copy creates a copy if the 
> run-time check shows that the argument is not contiguous. Thus, it is 
> not a wrong-code issue. However, for performance reasons, it makes 
> sense to avoid the call __gfortran_intrinsic_pack.
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?
>
> Tobias
Janus Weil - April 19, 2013, 8:12 p.m.
Hi Tobias,

> Fortran 2008 supports  C_LOC(array); if the argument is not simply
> contiguous, the current code adds a call to __gfortran_intrinsic_pack.
>
> The pack call shouldn't be there. Fortran 2008 demands that the actual
> argument is contiguous and intrinsic_pack copy creates a copy if the
> run-time check shows that the argument is not contiguous. Thus, it is not a
> wrong-code issue. However, for performance reasons, it makes sense to avoid
> the call __gfortran_intrinsic_pack.
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?

finally found a few silent minutes to have a look at this patch.

What I don't quite understand is:


@@ -6317,8 +6317,13 @@ conv_isocbinding_function (gfc_se *se, gfc_expr *expr)
     {
       if (arg->expr->rank == 0)
     gfc_conv_expr_reference (se, arg->expr);
-      else
+      else if (gfc_is_simply_contiguous (arg->expr, false))
     gfc_conv_array_parameter (se, arg->expr, true, NULL, NULL, NULL);
+      else
+    {
+      gfc_conv_expr_descriptor (se, arg->expr);
+      se->expr = gfc_conv_descriptor_data_get (se->expr);
+    }


Why doesn't 'gfc_conv_array_parameter' handle this situation properly?
After all there is code like this inside:

  no_pack = (...)
              ||
         gfc_is_simply_contiguous (expr, false));


Wouldn't it be better to fix the logic in there, instead of avoiding
to call it alltogether? This might also help in other situations ...

Cheers,
Janus
Tobias Burnus - April 19, 2013, 9:15 p.m.
Janus Weil:
> What I don't quite understand is:
> @@ -6317,8 +6317,13 @@ conv_isocbinding_function (gfc_se *se, gfc_expr *expr)
>       {
>         if (arg->expr->rank == 0)
>       gfc_conv_expr_reference (se, arg->expr);
> -      else
> +      else if (gfc_is_simply_contiguous (arg->expr, false))
>       gfc_conv_array_parameter (se, arg->expr, true, NULL, NULL, NULL);
> +      else
> +    {
> +      gfc_conv_expr_descriptor (se, arg->expr);
> +      se->expr = gfc_conv_descriptor_data_get (se->expr);
> +    }
>
>
> Why doesn't 'gfc_conv_array_parameter' handle this situation properly?

Well, it does: As it doesn't know whether the array is contiguous or not 
- it packs the array. That's what one usually wants.

However, for C_LOC one knows that the array is contiguous - the user 
promises this to the compiler - and, hence, no packing is needed.

> After all there is code like this inside:
>    no_pack = (...)
>                ||
>           gfc_is_simply_contiguous (expr, false));
> Wouldn't it be better to fix the logic in there, instead of avoiding
> to call it alltogether? This might also help in other situations ...

I don't know of any other situation where the *user* guarantees that the 
argument is contiguous while the compiler cannot check this. And even if 
the user lied to the compiler - doing no packing gives at least the 
address of the first element instead of the address of an immediately 
feed temporary array.

(Actually, there are other places in the compiler, where the user 
guarantees this: Pointer assignment to a "contiguous" pointer; there, 
the user promises that the target is contiguous. Maybe there are other 
spots as well. On the other hand, if the dummy argument has the 
CONTIGUOUS attribute, an automatic copy-in/out is required if it is not 
contiguous.)

Tobias

Tobias
Janus Weil - April 20, 2013, 10:42 a.m.
>> What I don't quite understand is:
>> @@ -6317,8 +6317,13 @@ conv_isocbinding_function (gfc_se *se, gfc_expr
>> *expr)
>>       {
>>         if (arg->expr->rank == 0)
>>       gfc_conv_expr_reference (se, arg->expr);
>> -      else
>> +      else if (gfc_is_simply_contiguous (arg->expr, false))
>>       gfc_conv_array_parameter (se, arg->expr, true, NULL, NULL, NULL);
>> +      else
>> +    {
>> +      gfc_conv_expr_descriptor (se, arg->expr);
>> +      se->expr = gfc_conv_descriptor_data_get (se->expr);
>> +    }
>>
>>
>> Why doesn't 'gfc_conv_array_parameter' handle this situation properly?
>
> Well, it does: As it doesn't know whether the array is contiguous or not -
> it packs the array. That's what one usually wants.
>
> However, for C_LOC one knows that the array is contiguous - the user
> promises this to the compiler - and, hence, no packing is needed.

Ok, I see. Then the patch is certainly ok. (The fact that
conv_isocbinding_function is used also for C_ASSOCIATED and C_FUNLOC
is no problem either, I guess).

However, if calling C_LOC on a non-contiguous array is invalid,
shouldn't one add a check for cases like

  integer, dimension(1:5,1:5), target :: zzz
  type(c_ptr) :: ptr
  ptr = c_loc (zzz(4:,4:))

where the compiler can easily tell that the argument is not contiguous ... ?

Cheers,
Janus
Tobias Burnus - April 20, 2013, 10:59 a.m.
Am 20.04.2013 12:42, schrieb Janus Weil:
>> Well, it does: As it doesn't know whether the array is contiguous or 
>> not - it packs the array. That's what one usually wants. However, for 
>> C_LOC one knows that the array is contiguous - the user promises this 
>> to the compiler - and, hence, no packing is needed. 
> Ok, I see. Then the patch is certainly ok. (The fact that
> conv_isocbinding_function is used also for C_ASSOCIATED and C_FUNLOC
> is no problem either, I guess).
Well, the code in question is under:

   if (expr->value.function.isym->id == GFC_ISYM_C_LOC)


> However, if calling C_LOC on a non-contiguous array is invalid,
> shouldn't one add a check for cases like
>    integer, dimension(1:5,1:5), target :: zzz
>    type(c_ptr) :: ptr
>    ptr = c_loc (zzz(4:,4:))
> where the compiler can easily tell that the argument is not contiguous ... ?

Definitely. I think there is also a PR about adding a 
gfc_is_simply_noncontiguous() or something like that. It has several 
uses: C_LOC, pointer-assignment to a contiguous pointer, removing some 
"if"s related to packing (as one knows that internal_pack will pack). 
And for compile-time simplification of the (unimplemented) Fortran 2008 
function "is_contiguous".

Tobias
Janus Weil - April 20, 2013, 11:44 a.m.
>>> Well, it does: As it doesn't know whether the array is contiguous or not
>>> - it packs the array. That's what one usually wants. However, for C_LOC one
>>> knows that the array is contiguous - the user promises this to the compiler
>>> - and, hence, no packing is needed.
>>
>> Ok, I see. Then the patch is certainly ok. (The fact that
>> conv_isocbinding_function is used also for C_ASSOCIATED and C_FUNLOC
>> is no problem either, I guess).
>
> Well, the code in question is under:
>
>   if (expr->value.function.isym->id == GFC_ISYM_C_LOC)

Ah, sure, I missed that.

Btw, wouldn't it make more sense to split up
'conv_isocbinding_function' into three separate functions, since there
isn't really any common code and one could directly call them in:

    case GFC_ISYM_C_ASSOCIATED:
    case GFC_ISYM_C_FUNLOC:
    case GFC_ISYM_C_LOC:
      conv_isocbinding_function (se, expr);
      break;


Anyway, the patch is ok as is (or with this additional cleanup, if you prefer).


>> However, if calling C_LOC on a non-contiguous array is invalid,
>> shouldn't one add a check for cases like
>>    integer, dimension(1:5,1:5), target :: zzz
>>    type(c_ptr) :: ptr
>>    ptr = c_loc (zzz(4:,4:))
>> where the compiler can easily tell that the argument is not contiguous ...
>> ?
>
> Definitely. I think there is also a PR about adding a
> gfc_is_simply_noncontiguous() or something like that. It has several uses:
> C_LOC, pointer-assignment to a contiguous pointer, removing some "if"s
> related to packing (as one knows that internal_pack will pack). And for
> compile-time simplification of the (unimplemented) Fortran 2008 function
> "is_contiguous".

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

Cheers,
Janus

Patch

2013-04-10  Tobias Burnus  <burnus@net-b.de>

	PR fortran/56907
	* trans-intrinsic.c (conv_isocbinding_function): Don't pack array
	passed to C_LOC

2013-04-10  Tobias Burnus  <burnus@net-b.de>

	PR fortran/56907
	* gfortran.dg/c_loc_test_22.f90: New.

diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
index 9b2cc19..005dd73 100644
--- a/gcc/fortran/trans-intrinsic.c
+++ b/gcc/fortran/trans-intrinsic.c
@@ -6317,8 +6317,13 @@  conv_isocbinding_function (gfc_se *se, gfc_expr *expr)
     {
       if (arg->expr->rank == 0)
 	gfc_conv_expr_reference (se, arg->expr);
-      else
+      else if (gfc_is_simply_contiguous (arg->expr, false))
 	gfc_conv_array_parameter (se, arg->expr, true, NULL, NULL, NULL);
+      else
+	{
+	  gfc_conv_expr_descriptor (se, arg->expr);
+	  se->expr = gfc_conv_descriptor_data_get (se->expr);
+	}
 
       /* TODO -- the following two lines shouldn't be necessary, but if
 	 they're removed, a bug is exposed later in the code path.
--- /dev/null	2013-04-10 09:49:18.320086712 +0200
+++ gcc/gcc/testsuite/gfortran.dg/c_loc_test_22.f90	2013-04-10 21:42:20.835284814 +0200
@@ -0,0 +1,24 @@ 
+! { dg-do compile }
+! { dg-options "-fdump-tree-original" }
+!
+! PR fortran/56907
+!
+subroutine sub(xxx, yyy)
+  use iso_c_binding
+  implicit none
+  integer, target, contiguous :: xxx(:)
+  integer, target             :: yyy(:)
+  type(c_ptr) :: ptr1, ptr2, ptr3, ptr4
+  ptr1 = c_loc (xxx)
+  ptr2 = c_loc (xxx(5:))
+  ptr3 = c_loc (yyy)
+  ptr4 = c_loc (yyy(5:))
+end
+! { dg-final { scan-tree-dump-not " _gfortran_internal_pack" "original" } }
+! { dg-final { scan-tree-dump-times "parm.\[0-9\]+.data = \\(void .\\) &\\(.xxx.\[0-9\]+\\)\\\[0\\\];" 1 "original" } }
+! { dg-final { scan-tree-dump-times "parm.\[0-9\]+.data = \\(void .\\) &\\(.xxx.\[0-9\]+\\)\\\[D.\[0-9\]+ \\* 4\\\];" 1 "original" } }
+! { dg-final { scan-tree-dump-times "parm.\[0-9\]+.data = \\(void .\\) &\\(.yyy.\[0-9\]+\\)\\\[0\\\];" 1 "original" } }
+! { dg-final { scan-tree-dump-times "parm.\[0-9\]+.data = \\(void .\\) &\\(.yyy.\[0-9\]+\\)\\\[D.\[0-9\]+ \\* 4\\\];" 1 "original" } }
+
+! { dg-final { scan-tree-dump-times "D.\[0-9\]+ = parm.\[0-9\]+.data;\[^;]+ptr\[1-4\] = D.\[0-9\]+;" 4 "original" } }
+! { dg-final { cleanup-tree-dump "optimized" } }