[patch,wip] warn on noncontiguous pointers

[patch,wip] warn on noncontiguous pointers

[Cesar Philippidis]

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

As of GCC 8, gfortran now errors when a pointer with a contiguous
attribute is set to point to a target without a contiguous attribute. I
think this is overly strict, and should probably be demoted to a
pedantic warning as I've done in the attached patch.

I ran into this issue while I was tuning GCC for lsdalton. Specifically,
CMake generates (not exactly because I reduced it) the following test
case for ScaTeLib to determine if that library can be enabled:

program test
   implicit none
   real,pointer :: fptr1(:)
   real,pointer,contiguous :: fptr3(:,:,:)

   allocate(fptr1(12))
   call random_number(fptr1)

   !Test pointer reshape II

   fptr3(1:2,1:2,1:2) => fptr1(4:)
end program

Note how fptr1 doesn't have a contiguous attribute. Does anyone have
thoughts on this? Maybe the ScaTeLib code needs to be updated.

Thanks,
Cesar

[-- Attachment #2: 0001-Fortran-Disable-Assignment-to-contiguous-pointer-fro.patch --]
[-- Type: text/x-patch, Size: 1195 bytes --]

Disable "Assignment to contiguous pointer from non-contiguous target" error

2018-XX-YY  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/fortran/
	* expr.c (gfc_check_pointer_assign): Demote "Assignment to
	contiguous pointer from non-contiguous target" to a warning.
---

diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index 3315bb840af..74caa4f2d59 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -3957,13 +3957,13 @@ gfc_check_pointer_assign (gfc_expr *lvalue, gfc_expr *rvalue)
 	  }
     }
 
-  /* Error for assignments of contiguous pointers to targets which is not
+  /* Warn for assignments of contiguous pointers to targets which is not
      contiguous.  Be lenient in the definition of what counts as
      contiguous.  */
 
   if (lhs_attr.contiguous && !gfc_is_simply_contiguous (rvalue, false, true))
-    gfc_error ("Assignment to contiguous pointer from non-contiguous "
-	       "target at %L", &rvalue->where);
+    gfc_warning (OPT_Wpedantic, "Assignment to contiguous pointer from "
+		 "non-contiguous target at %L", &rvalue->where);
 
   /* Warn if it is the LHS pointer may lives longer than the RHS target.  */
   if (warn_target_lifetime
-- 
2.17.1

Re: [patch,wip] warn on noncontiguous pointers

[Thomas Koenig]

Hi Cesar,

> As of GCC 8, gfortran now errors when a pointer with a contiguous
> attribute is set to point to a target without a contiguous attribute. I
> think this is overly strict, and should probably be demoted to a
> pedantic warning as I've done in the attached patch.

We had a lengthy discussion on that one. Still, we can dig into the
standard for that one.

J3/10-007 says in 7.2.2.3  Data pointer assignment

# 7 If the pointer object has the CONTIGUOUS attribute, the pointer
# target shall be contiguous.

# 9 If bounds-remapping-list is specified, the pointer target shall
# be simply contiguous (6.5.4) or of rank one

program test
    implicit none
    real,pointer :: fptr1(:)
    real,pointer,contiguous :: fptr3(:,:,:)

    allocate(fptr1(12))
    call random_number(fptr1)

    !Test pointer reshape II

    fptr3(1:2,1:2,1:2) => fptr1(4:)

end program

So, by paragraph 9, this would be OK. Let's see what paragraph 7
means when it says "contiguous". 5.3.7 says

An object is contiguous if it is

# (1) an object with the CONTIGUOUS attribute,
# (2) a nonpointer whole array that is not assumed-shape,
# (3) an assumed-shape array that is argument associated with an
      array that is contiguous,
# (4) an array allocated by an ALLOCATE statement,
# (5) a pointer associated with a contiguous target, or
# (6) a nonzero-sized array section (6.5.3) provided that
#   (a) its base object is contiguous,
#   (b) it does not have a vector subscript,
#   (c) the elements of the section, in array element order, are a
#       subset of the base object elements that are consecutive in
#       array element order,
#   (d) if the array is of type character and a substring-range appears,
#       the substring-range specifies all of the characters of the
#       parent string (6.4.1),
#   (e) only its final part-ref has nonzero rank, and
#   (f) it is not the real or imaginary part (6.4.4) of an array of type
#       complex.

An object is not contiguous if it is an array subobject, and

[conditions not relevant elided]

# It is processor dependent whether any other object is contiguous.

If we go down the list, we see that fptr1(4:) is not contiguous; it
is not an array (it is a pointer), so (4) also does not apply.

So, we are in the realm of processor dependent behavior, so we can
chose what to do.

The last time we discussed this, we agreed on a hard error.  One
important argument is that a mistakenly applied contiguous
attribute will lead to wrong code, and that it is quite easy
to check this, as we do now.

So, I think we should leave the behavior as it is now, and

 > Maybe the ScaTeLib code needs to be updated.

sounds like a good idea to me.

Regards

	Thomas

Re: [patch,wip] warn on noncontiguous pointers

[Cesar Philippidis]

On 09/26/2018 01:49 PM, Thomas Koenig wrote:
> Hi Cesar,
> 
>> As of GCC 8, gfortran now errors when a pointer with a contiguous
>> attribute is set to point to a target without a contiguous attribute. I
>> think this is overly strict, and should probably be demoted to a
>> pedantic warning as I've done in the attached patch.
> 
> We had a lengthy discussion on that one. Still, we can dig into the
> standard for that one.
> 
> J3/10-007 says in 7.2.2.3  Data pointer assignment
> 
> # 7 If the pointer object has the CONTIGUOUS attribute, the pointer
> # target shall be contiguous.
> 
> # 9 If bounds-remapping-list is specified, the pointer target shall
> # be simply contiguous (6.5.4) or of rank one
> 
> program test
>    implicit none
>    real,pointer :: fptr1(:)
>    real,pointer,contiguous :: fptr3(:,:,:)
> 
>    allocate(fptr1(12))
>    call random_number(fptr1)
> 
>    !Test pointer reshape II
> 
>    fptr3(1:2,1:2,1:2) => fptr1(4:)
> 
> end program
> 
> So, by paragraph 9, this would be OK. Let's see what paragraph 7
> means when it says "contiguous". 5.3.7 says
> 
> An object is contiguous if it is
> 
> # (1) an object with the CONTIGUOUS attribute,
> # (2) a nonpointer whole array that is not assumed-shape,
> # (3) an assumed-shape array that is argument associated with an
>      array that is contiguous,
> # (4) an array allocated by an ALLOCATE statement,
> # (5) a pointer associated with a contiguous target, or
> # (6) a nonzero-sized array section (6.5.3) provided that
> #   (a) its base object is contiguous,
> #   (b) it does not have a vector subscript,
> #   (c) the elements of the section, in array element order, are a
> #       subset of the base object elements that are consecutive in
> #       array element order,
> #   (d) if the array is of type character and a substring-range appears,
> #       the substring-range specifies all of the characters of the
> #       parent string (6.4.1),
> #   (e) only its final part-ref has nonzero rank, and
> #   (f) it is not the real or imaginary part (6.4.4) of an array of type
> #       complex.
> 
> An object is not contiguous if it is an array subobject, and
> 
> [conditions not relevant elided]
> 
> # It is processor dependent whether any other object is contiguous.
> 
> If we go down the list, we see that fptr1(4:) is not contiguous; it
> is not an array (it is a pointer), so (4) also does not apply.
> 
> So, we are in the realm of processor dependent behavior, so we can
> chose what to do.
> 
> The last time we discussed this, we agreed on a hard error.  One
> important argument is that a mistakenly applied contiguous
> attribute will lead to wrong code, and that it is quite easy
> to check this, as we do now.
> 
> So, I think we should leave the behavior as it is now, and

Thank you for the explanation. That all seems reasonable.

>> Maybe the ScaTeLib code needs to be updated.
> 
> sounds like a good idea to me.

ACK.

Thanks,
Cesar

Re: [patch,wip] warn on noncontiguous pointers

[Thomas Koenig]

Hi Cesar,

> How's this patch look? I bootstrapped and regression tested it for
> x86_64 Linux with nvptx offloading (not that it exercised any offloading
> functionality).
> 
> Is it OK for trunk?

OK.

Maybe we should one day revisit this one day and separate out the cases
where we can prove that the array cannot be contiguous vs. the ones
where we cannot prove either way.

For now, this fixes a rejects-valid.

Thanks!

Regards

	Thomas

Re: [patch,wip] warn on noncontiguous pointers

[Cesar Philippidis]

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

On 09/30/2018 11:12 AM, Thomas Koenig wrote:
> Am 28.09.18 um 07:12 schrieb Bader, Reinhold:
>>   Looking at 5.3.8.1 (Fortran 2008 for the sake of this discussion),
>> we have that
>> an object that has the DIMENSION attribute is an array. array-spec
>> includes deferred-shape-spec-list,
>> so any object declared with deferred shape is an array.
> 
> OK, I see, so this code is indeed legal.  Thanks!
> 
> @Cesar:  The idea behind your patch is sound.  I think this would
> fit into the "legal, but can sometimes bite you" category,
> so I think would be better with -Wextra (where we accept that
> there might be quite a few false positives).
> 
> Could you adapt your patch accordingly and also add two test cases,
> one testing for the absence of the warning/error with normal
> options, and one for the warning with -Wextra?

How's this patch look? I bootstrapped and regression tested it for
x86_64 Linux with nvptx offloading (not that it exercised any offloading
functionality).

Is it OK for trunk?

Thanks,
Cesar

[-- Attachment #2: 0001-Fortran-Disable-Assignment-to-contiguous-pointer-fro.patch --]
[-- Type: text/x-patch, Size: 3253 bytes --]

[Fortran] Disable "Assignment to contiguous pointer from non-contiguous target" error

2018-XX-YY  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/fortran/
	* expr.c (gfc_check_pointer_assign): Demote "Assignment to
	contiguous pointer from non-contiguous target" to a warning.

	gcc/testsuite/
	* gfortran.dg/contiguous_4.f90: Adjust.
	* gfortran.dg/contiguous_4.f90: New test.
---
 gcc/fortran/expr.c                         |  6 +++---
 gcc/testsuite/gfortran.dg/contiguous_4.f90 |  6 +++---
 gcc/testsuite/gfortran.dg/contiguous_7.f90 | 24 ++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/contiguous_7.f90

diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index 3315bb8..1cfda5f 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -3957,13 +3957,13 @@ gfc_check_pointer_assign (gfc_expr *lvalue, gfc_expr *rvalue)
 	  }
     }
 
-  /* Error for assignments of contiguous pointers to targets which is not
+  /* Warn for assignments of contiguous pointers to targets which is not
      contiguous.  Be lenient in the definition of what counts as
      contiguous.  */
 
   if (lhs_attr.contiguous && !gfc_is_simply_contiguous (rvalue, false, true))
-    gfc_error ("Assignment to contiguous pointer from non-contiguous "
-	       "target at %L", &rvalue->where);
+    gfc_warning (OPT_Wextra, "Assignment to contiguous pointer from "
+		 "non-contiguous target at %L", &rvalue->where);
 
   /* Warn if it is the LHS pointer may lives longer than the RHS target.  */
   if (warn_target_lifetime
diff --git a/gcc/testsuite/gfortran.dg/contiguous_4.f90 b/gcc/testsuite/gfortran.dg/contiguous_4.f90
index b05dcfb..874ef8b 100644
--- a/gcc/testsuite/gfortran.dg/contiguous_4.f90
+++ b/gcc/testsuite/gfortran.dg/contiguous_4.f90
@@ -10,10 +10,10 @@ program cont_01_neg
 
   x = (/ (real(i),i=1,45) /)
   x2 = reshape(x,shape(x2))
-  r => x(::3)   ! { dg-error "Assignment to contiguous pointer" }
-  r2 => x2(2:,:) ! { dg-error "Assignment to contiguous pointer" }
+  r => x(::3)
+  r2 => x2(2:,:)
   r2 => x2(:,2:3)
   r => x2(2:3,1)
   r => x(::1)
-  r => x(::n) ! { dg-error "Assignment to contiguous pointer" }
+  r => x(::n)
 end program
diff --git a/gcc/testsuite/gfortran.dg/contiguous_7.f90 b/gcc/testsuite/gfortran.dg/contiguous_7.f90
new file mode 100644
index 0000000..cccc89f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/contiguous_7.f90
@@ -0,0 +1,24 @@
+! { dg-do compile }
+! { dg-additional-options "-Wextra" }
+!
+! Ensure that contiguous pointers pointing to noncontiguous pointers
+! to array results in a warning with -Wextra.
+
+program cont_01_neg
+  implicit none
+  real, pointer, contiguous :: r(:)
+  real, pointer, contiguous :: r2(:,:)
+  real, target :: x(45)
+  real, target :: x2(5,9)
+  integer :: i
+  integer :: n=1
+
+  x = (/ (real(i),i=1,45) /)
+  x2 = reshape(x,shape(x2))
+  r => x(::3) ! { dg-warning "ssignment to contiguous pointer from non-contiguous target" }
+  r2 => x2(2:,:) ! { dg-warning "ssignment to contiguous pointer from non-contiguous target" }
+  r2 => x2(:,2:3)
+  r => x2(2:3,1)
+  r => x(::1)
+  r => x(::n) ! { dg-warning "ssignment to contiguous pointer from non-contiguous target" }
+end program
-- 
2.7.4

Re: [patch,wip] warn on noncontiguous pointers

[Cesar Philippidis]

On 09/30/2018 11:12 AM, Thomas Koenig wrote:
> Am 28.09.18 um 07:12 schrieb Bader, Reinhold:
>>   Looking at 5.3.8.1 (Fortran 2008 for the sake of this discussion),
>> we have that
>> an object that has the DIMENSION attribute is an array. array-spec
>> includes deferred-shape-spec-list,
>> so any object declared with deferred shape is an array.
> 
> OK, I see, so this code is indeed legal.  Thanks!
> 
> @Cesar:  The idea behind your patch is sound.  I think this would
> fit into the "legal, but can sometimes bite you" category,
> so I think would be better with -Wextra (where we accept that
> there might be quite a few false positives).
> 
> Could you adapt your patch accordingly and also add two test cases,
> one testing for the absence of the warning/error with normal
> options, and one for the warning with -Wextra?

Yes, sure.

Thanks,
Cesar

Re: [patch,wip] warn on noncontiguous pointers

[Thomas Koenig]

Am 28.09.18 um 07:12 schrieb Bader, Reinhold:
>   Looking at 5.3.8.1 (Fortran 2008 for the sake of this discussion), we have that
> an object that has the DIMENSION attribute is an array. array-spec includes deferred-shape-spec-list,
> so any object declared with deferred shape is an array.

OK, I see, so this code is indeed legal.  Thanks!

@Cesar:  The idea behind your patch is sound.  I think this would
fit into the "legal, but can sometimes bite you" category,
so I think would be better with -Wextra (where we accept that
there might be quite a few false positives).

Could you adapt your patch accordingly and also add two test cases,
one testing for the absence of the warning/error with normal
options, and one for the warning with -Wextra?

Regards

	Thomas

Re: [patch,wip] warn on noncontiguous pointers

[Thomas Koenig]

Hi Reinhold,

>> If we go down the list, we see that fptr1(4:) is not contiguous; it
>> is not an array (it is a pointer), so (4) also does not apply.
> I don't agree with this conclusion. First, the array and pointer properties
> are not mutually exclusive (they are conveyed by specifying the DIMENSION
> and
> POINTER attribute, independently).

Can you maybe elaborate a bit more?

I see that you can associate a pointer with a target
(6.7.1.4), which of course can be an array. But I have
not found anything in the standard that says that a
pointer to an array is an array.

Regards

	Thomas

Re: [patch,wip] warn on noncontiguous pointers

[Bader, Reinhold]

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

Hello all, 

> We had a lengthy discussion on that one. Still, we can dig into the
> standard for that one.
>
> J3/10-007 says in 7.2.2.3  Data pointer assignment
>
> # 7 If the pointer object has the CONTIGUOUS attribute, the pointer
> # target shall be contiguous.
>
> # 9 If bounds-remapping-list is specified, the pointer target shall
> # be simply contiguous (6.5.4) or of rank one
>
> program test
>   implicit none
>   real,pointer :: fptr1(:)
>   real,pointer,contiguous :: fptr3(:,:,:)
>
>   allocate(fptr1(12))
>   call random_number(fptr1)
>
>   !Test pointer reshape II
>
>   fptr3(1:2,1:2,1:2) => fptr1(4:)
>
> end program
>
> So, by paragraph 9, this would be OK. Let's see what paragraph 7
> means when it says "contiguous". 5.3.7 says
>
> An object is contiguous if it is
>
> # (1) an object with the CONTIGUOUS attribute,
> # (2) a nonpointer whole array that is not assumed-shape,
> # (3) an assumed-shape array that is argument associated with an
>     array that is contiguous,
> # (4) an array allocated by an ALLOCATE statement,
> # (5) a pointer associated with a contiguous target, or
> # (6) a nonzero-sized array section (6.5.3) provided that
> #   (a) its base object is contiguous,
> #   (b) it does not have a vector subscript,
> #   (c) the elements of the section, in array element order, are a
> #       subset of the base object elements that are consecutive in
> #       array element order,
> #   (d) if the array is of type character and a substring-range appears,
> #       the substring-range specifies all of the characters of the
> #       parent string (6.4.1),
> #   (e) only its final part-ref has nonzero rank, and
> #   (f) it is not the real or imaginary part (6.4.4) of an array of type
> #       complex.
>
> An object is not contiguous if it is an array subobject, and
>
> [conditions not relevant elided]
>
> # It is processor dependent whether any other object is contiguous.
>
> If we go down the list, we see that fptr1(4:) is not contiguous; it
> is not an array (it is a pointer), so (4) also does not apply.

I don't agree with this conclusion. First, the array and pointer properties
are not mutually exclusive (they are conveyed by specifying the DIMENSION
and
POINTER attribute, independently). 
Specifically for the array section fptr1(4:) all conditions 
in item (6) are fulfilled, and therefore it is a contiguous TARGET suitable
for appearing on the RHS of a contiguous pointer assignment.

Regards
Reinhold



>
> So, we are in the realm of processor dependent behavior, so we can
> chose what to do.
>
> The last time we discussed this, we agreed on a hard error.  One
> important argument is that a mistakenly applied contiguous
> attribute will lead to wrong code, and that it is quite easy
> to check this, as we do now.
> 
> So, I think we should leave the behavior as it is now, and
>
>> Maybe the ScaTeLib code needs to be updated.
>
> sounds like a good idea to me.
> 
> Regards
>
>       Thomas


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7773 bytes --]