[wip] warn on noncontiguous pointers

Message ID e4cd4bcc-50c9-fe96-2cee-3aced4e5b15f@codesourcery.com
State New
Headers show
Series
  • [wip] warn on noncontiguous pointers
Related show

Commit Message

Cesar Philippidis Sept. 26, 2018, 6:51 p.m.
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

Comments

Thomas Koenig Sept. 26, 2018, 8:49 p.m. | #1
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
Cesar Philippidis Sept. 26, 2018, 8:55 p.m. | #2
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

Patch

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