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
