Hi Harald and Jerry,
thanks for taking the time to read my false assumptions ;-)
The assumption that v1%n(n:m, m:n) => v2%n(m:n, n:m) is valid in a pointer
remapping is wrong. (F2018 ยง10.2.2.3 paragraph 9). The rhs is neither simply
contiguous nor of rank one and therefore rejected. I spent some time to compose
a better example, but failed. Given this, I assume that with the ok'ed patch
the issue is sufficiently fixed. I therefore commit it. Should there be some
corner case I/we did not find, then I am sure we get a bug report about it.
While examining the code I figured that the test I've added, checked the input
and not the output as it was intended to do. I fixed this and committed as
gcc-15-7925-gf2339cefd69.
Attached is the fixed patch just for reference.
Thanks for the review and discussion,
Andre
On Thu, 6 Mar 2025 22:29:11 +0100
Harald Anlauf <anl...@gmx.de> wrote:
> Hi Andre,
>
> Am 06.03.25 um 09:15 schrieb Andre Vehreschild:
> > Hi Harald,
> >
> > I try to explain why I think my patch although solving the issue for this
> > case, does not do so in every case:
> >
> > My patch lets dependency analysis figure that the two objects can not have
> > any dependencies on each other or memory they are pointing to when the
> > types are different. Lets assume a simple list with a head node:
> >
> > type node
> > type(node), pointer :: prev, next
> > end type
> >
> > type head
> > type(node), pointer :: first, last
> > end type
> >
> > (Just as an example; this might not be correct Fortran). Setting up a node
> >
> > type(head) :: h
> > type(node), allocatable, target :: n
> >
> > allocate(n)
> > n%prev = n%next = NULL()
> > h%first => n
> > h%last => n
> >
> > The patched dependency analysis will deem `h%first => n` operands to be
> > independent of each other based on both having different types. Note,
> > dependency analysis looks at the object's type first, i.e. `head` vs. `node`
> > where I patched it!
> >
> > While the patch may be correct for the example, it may not be for every
> > case. Just look one ref further: `h%first`'s type is `node` this would be
> > deemed having (potentially) a dependency on the type of the target `n`,
> > because both are of type `node`. Just having the same type is enough here!
> >
> > So much just for the consequences of my change. Now let me try to explain,
> > why I think the patch is insufficient:
> >
> > Assume a type (again pseudo-Fortran):
> >
> > type T
> > type(T), pointer :: n(:,:)
> > end type
> >
> > Now doing something like
> >
> > type(T) :: v1, v2
> >
> > v1%n(n:m, m:n) => v2%n(m:n, n:m)
> >
> > The types on lhs and rhs need to be the same. Then even the patched
> > dependency analysis concludes that in a `forall` a temporary is needed,
> > because the pointer assignment may overlap (trans-stmt.cc:5411):
> >
> > need_temp = gfc_check_dependency (c->expr1, c->expr2, 0);
> >
> > The indexes don't really matter here. They just need to be complicated,
> > i.e.not just a AR_FULL or AR_ELEMENT. Now
> > `gfc_trans_pointer_assign_need_temp()` is chosen. That routine in line
> > trans-stmt.cc:5072 converts the lhs into a descriptor. But because of the
> > non-full/complicated array addressing (note, the non-contiguous indexes !)
> > `gfc_conv_expr_descriptor()` creates a `parm.12` (in my case) temporary
> > descriptor. That temporary descriptor, hence the name, is usually used as
> > actual argument to call a function. But the next line in
> > `gfc_trans_pointer_assign_need_temp` just assigns the rhs` temporary to
> > that `parm.12` (I have omitted only some casts, but in the code generated
> > there are *no* member-references):
> >
> > parm.12 = lhs-temp
> >
> > This leads to the gimplification error, because the types of the rhs
> > temporary and the temporary `parm.12` are not compatible. Furthermore is
> > the assignment as it is done there non-sense in my eyes, because it is
> > literally `parm.12 = rhs-temp`. I.e. in the code generated just before
> > that, `parm.12` is constructed an initialized laboriously and then shall be
> > overwritten completely. I assume that we would rather need something like
> > (pseudo-pseudo code):
> >
> > for (n = 1: #elem(rhs-temp))
> > parm.12.data[i] = rhs-temp.data[i]
> > end for
> >
> > But I have no clue how to easily accomplish that. I tried putting the
> > rhs-temp into the se of the `gfc_conv_expr_descriptor()` and setting
> > `se.direct_byref = 1`, but that does it the wrong way around, i.e. rhs =
> > lhs (Side note: having a routine doing an assignment, that is otherwise
> > just delivering a descriptor, is some odd design decision). So I am at a
> > loss here.
> >
> > I hope to not have confused everyone. The possibility that I am mislead or
> > overlooking something or even thinking to complicated here is very likely.
> > So please correct me!
> >
> > Therefore let's discuss this a bit more. I hold the patch back until we
> > come to a conclusion, that it is worth merging w/o breaking too much (which
> > could be possible, because that dependency analysis is used in many
> > locations).
>
> ok, I think I understand the potential issue you are bringing up here.
> After the patch, we might not generate a temporary when it is actually
> needed. But does the current code do it right?
>
> If not, we (= you) might proceed by committing the present patch,
> open a PR about a missing / improper dependency analysis, and
> link to the current discussion on the ML so that your arguments
> are properly tracked. Especially if the discussion and the solution
> takes a little longer.
>
> Or you do think your patch makes anything worse?
>
> Thanks,
> Harald
>
> > Regards,
> > Andre
> >
> >
> > On Wed, 5 Mar 2025 20:53:37 +0100
> > Harald Anlauf <anl...@gmx.de> wrote:
> >
> >> Hi Andre,
> >>
> >> Jerry already OK'ed your patch, but:
> >>
> >> Am 05.03.25 um 15:34 schrieb Andre Vehreschild:
> >>> This fixes the PR, but not really the problem, because when say a
> >>> obj(i)%arr(2:5) => obj(i)%arr(1:4) is done we run into the same issue. I
> >>> don't have a solution for that error. It might be needed to prevent
> >>> generating the parm.NN variable for the lhs, because that is mostly
> >>> useless there. (Or I don't understand (yet) how to use it).
> >>
> >> can you explain where you do see an issue here?
> >>
> >> A pointer assignment in the way you describe seems perfectly legal
> >> Fortran and works here.
> >>
> >> If we are missing anything, would you please open a PR with more
> >> details?
> >>
> >> Thanks,
> >> Harald
> >>
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
> >
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
From 63c147ef88885ef4fc6f4f846c6324e49d81a33c Mon Sep 17 00:00:00 2001
From: Andre Vehreschild <ve...@gcc.gnu.org>
Date: Wed, 5 Mar 2025 15:18:48 +0100
Subject: [PATCH] Fortran: Fix gimplification error for pointer remapping in
forall [PR107143]
Enhance dependency checking for data pointers to check for same derived
type and not only for a type being a derived type. This prevent
generation of a descriptor for a function call, that is unsuitable in
forall's pointer assignment.
PR fortran/107143
gcc/fortran/ChangeLog:
* dependency.cc (check_data_pointer_types): Do not just compare
for derived type, but for same derived type.
gcc/testsuite/ChangeLog:
* gfortran.dg/forall_20.f90: New test.
---
gcc/fortran/dependency.cc | 3 +-
gcc/testsuite/gfortran.dg/forall_20.f90 | 40 +++++++++++++++++++++++++
2 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gfortran.dg/forall_20.f90
diff --git a/gcc/fortran/dependency.cc b/gcc/fortran/dependency.cc
index 8354b185f34..28b872f6638 100644
--- a/gcc/fortran/dependency.cc
+++ b/gcc/fortran/dependency.cc
@@ -1250,7 +1250,8 @@ check_data_pointer_types (gfc_expr *expr1, gfc_expr *expr2)
sym2 = expr2->symtree->n.sym;
/* Keep it simple for now. */
- if (sym1->ts.type == BT_DERIVED && sym2->ts.type == BT_DERIVED)
+ if (sym1->ts.type == BT_DERIVED && sym2->ts.type == BT_DERIVED
+ && sym1->ts.u.derived == sym2->ts.u.derived)
return false;
if (sym1->attr.pointer)
diff --git a/gcc/testsuite/gfortran.dg/forall_20.f90 b/gcc/testsuite/gfortran.dg/forall_20.f90
new file mode 100644
index 00000000000..b0bb0dcb62f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/forall_20.f90
@@ -0,0 +1,40 @@
+!{ dg-do run }
+!
+! Check pointer aliasing is done w/o temp.
+! Contributed by Arseny Solokha <asolo...@gmx.com>
+
+program pr107143
+ type ta
+ integer, POINTER :: ip(:)
+ end type ta
+
+ type tb
+ integer, POINTER :: ip(:,:)
+ end type tb
+
+ integer, parameter :: cnt = 3
+ type(ta) :: a(cnt)
+ type(tb) :: b(cnt)
+ integer, target :: arr(8) = [1,2,3,4,5,6,7,8]
+
+ do i = 1, cnt
+ allocate(a(i)%ip(8), SOURCE=arr * i)
+ end do
+ call s5(b, a, 2, 4)
+
+ do i = 1, cnt
+ if (any(b(i)%ip /= reshape(arr * i, [2, 4]))) stop i
+ end do
+
+contains
+
+subroutine s5(y,z,n1,n2)
+
+ type(tb) :: y(:)
+ type(ta), TARGET :: z(:)
+
+ forall (i=1:cnt)
+ y(i)%ip(1:n1,1:n2) => z(i)%ip
+ end forall
+end subroutine s5
+end program
--
2.48.1
