Re: [PATCH v2] Fortran: fix reallocation on assignment of polymorphic variables [PR110415]
Hi Andrew,
This is OK by me.
I attach a slightly edited version of the patch itself in the hope that it
will make the code a bit clearer.
Thanks and welcome!
Paul
On Mon, 27 Nov 2023 at 17:35, Andrew Jenner wrote:
> This is the second version of the patch - previous discussion at:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636671.html
>
> This patch adds the testcase from PR110415 and fixes the bug.
>
> The problem is that in a couple of places in trans_class_assignment in
> trans-expr.cc, we need to get the run-time size of the polymorphic
> object from the vtbl, but we are currently getting that vtbl from the
> lhs of the assignment rather than the rhs. This gives us the old value
> of the size but we need to pass the new size to __builtin_malloc and
> __builtin_realloc.
>
> I'm fixing this by adding a parameter to trans_class_vptr_len_assignment
> to retrieve the tree corresponding the vptr from the object on the rhs
> of the assignment, and then passing this where it is needed. In the case
> where trans_class_vptr_len_assignment returns NULL_TREE for the rhs vptr
> we use the lhs vptr as before.
>
> To get this to work I also needed to change the implementation of
> trans_class_vptr_len_assignment to create a temporary for the assignment
> in more circumstances. Currently, the "a = func()" assignment in MAIN__
> doesn't hit the "Create a temporary for complication expressions" case
> on line 9951 because "DECL_P (rse->expr)" is true - the expression has
> already been placed into a temporary. That means we don't hit the "if
> (temp_rhs ..." case on line 10038 and go on to get the vptr_expr from
> "gfc_lval_expr_from_sym (gfc_find_vtab (&re->ts))" on line 10057 which
> is the vtbl of the static type rather than the dynamic one from the rhs.
> So with this fix we create an extra temporary, but that should be
> optimised away in the middle-end so there should be no run-time effect.
>
> I'm not sure if this is the best way to fix this (the Fortran front-end
> is new territory for me) but I've verified that the testcase passes with
> this change, fails without it, and that the change does not introduce
> any FAILs when running the gfortran testcases on x86_64-pc-linux-gnu.
>
> After the previous submission, Tobias Burnus found a closely related
> problem and contributed testcases and a fix for it, which I have
> incorporated into this version of the patch. The problem in this case is
> with the __builtin_realloc call that is executed if one polymorphic
> variable is replaced by another. The return value of this call was being
> ignored rather than used to replace the pointer being reallocated.
>
> Is this OK for mainline, GCC 13 and OG13?
>
> Thanks,
>
> Andrew
>
> gcc/fortran/
> PR fortran/110415
> * trans-expr.cc (trans_class_vptr_len_assignment): Add
> from_vptrp parameter. Populate it. Don't check for DECL_P
> when deciding whether to create temporary.
> (trans_class_pointer_fcn, gfc_trans_pointer_assignment): Add
> NULL argument to trans_class_vptr_len_assignment calls.
> (trans_class_assignment): Get rhs_vptr from
> trans_class_vptr_len_assignment and use it for determining size
> for allocation/reallocation. Use return value from realloc.
>
> gcc/testsuite/
> PR fortran/110415
> * gfortran.dg/pr110415.f90: New test.
> * gfortran.dg/asan/pr110415-2.f90: New test.
> * gfortran.dg/asan/pr110415-3.f90: New test.
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index 1b8be081a17..35b000bf8d5 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -9892,7 +9892,9 @@ trans_get_upoly_len (stmtblock_t *block, gfc_expr *expr)
static tree
trans_class_vptr_len_assignment (stmtblock_t *block, gfc_expr * le,
gfc_expr * re, gfc_se *rse,
- tree * to_lenp, tree * from_lenp)
+ tree * to_lenp = NULL,
+ tree * from_lenp = NULL,
+ tree * from_vptrp = NULL)
{
gfc_se se;
gfc_expr * vptr_expr;
@@ -9900,12 +9902,15 @@ trans_class_vptr_len_assignment (stmtblock_t *block, gfc_expr * le,
bool set_vptr = false, temp_rhs = false;
stmtblock_t *pre = block;
tree class_expr = NULL_TREE;
+ tree from_vptr = NULL_TREE;
/* Create a temporary for complicated expressions. */
- if (re->expr_type != EXPR_VARIABLE && re->expr_type != EXPR_NULL
- && rse->expr != NULL_TREE && !DECL_P (rse->expr))
+ if (re->expr_type != EXPR_VARIABLE
+ && re->expr_type != EXPR_NULL
+ && rse->expr != NULL_TREE)
{
- if (re->ts.type == BT_CLASS && !GFC_CLASS_TYPE_P (TREE_TYPE (rse->expr)))
+ if (re->ts.type == BT_CLASS
+ && !GFC_CLASS_TYPE_P (TREE_TYPE (rse->expr)))
class_expr = gfc_get_class_from_expr (rse->expr);
if (rse->loop)
@@ -9959,8 +9964,8 @@ trans_class_vptr_len_assignment (stmtblock_t *block, gfc_expr * le,
/* Get the vptr from the rhs expression only, when it is
Re: [PATCH v7 2/5] OpenMP/OpenACC: Rework clause expansion and nested struct handling
Hi Julian,
On 29.11.23 12:43, Julian Brown wrote:
Here is a patch incorporating your initial review comments
(hopefully!).
Thanks.
The patch LGTM - with the two remarks below addressed.
(i.e. fixing one testcase and filing two PRs (or common PR) about the features
missing and exposed by the two test cases, referencing also to those testcases
- and for the lvalues mentioning the OpenMP spec issue number.)
* * *
BTW: The 1/5 has been several times approved and is just reindenting - and
is obviously still OK.
(Review wise, 3/5, 4/5 and 5/5 still has to be done.
I think the patch can go in before - given the huge improvements, even though
it regresses for a few cases (xfail added for 2 Fortran testcases). 3/5
un-xfails
one and a half of the textcases, 5/5 un-xfails the remaining half and all of
{3,4,5}/5
contain very useful improvements besides this. - But maybe waiting for at least
3/5
makes sense.
In either case, I try to review the remaining patches soon.)
* * *
Question regarding the following:
(a) The dg-xfail-run-if looks bogus as this an OpenMP test and not an OpenACC
test
(b) If there is shared memory, using 'omp target' should be fine.
Namely, given that:
--- /dev/null +++ b/libgomp/testsuite/libgomp.c++/target-49.C @@ -0,0
+1,37 @@ +#include +#include + +struct s { + int
(&a)[10]; + s(int (&a0)[10]) : a(a0) {} +}; + +int +main (int argc,
char *argv[]) +{ + int la[10]; + s v_real(la); + s *v = &v_real; + +
memset (la, 0, sizeof la); + + #pragma omp target enter data map(to:
v) + + /* Copying the whole v[0] here DOES NOT WORK yet because the
reference 'a' is + not copied "as if" it was mapped explicitly as a
member. FIXME. */ + #pragma omp target enter data map(to: v[0]) + +
//#pragma omp target + { + v->a[5]++; + } + + #pragma omp target exit
data map(release: v[0]) + #pragma omp target exit data map(from: v) +
+ assert (v->a[5] == 1); + + return 0; +} + +// { dg-xfail-run-if
"TODO" { *-*-* } { "-DACC_MEM_SHARED=0" } }
Shouldn't the XFAIL not be based on '{ target offload_device_nonshared_as }'
and the 'omp target' be uncommented?
And I wonder whether we need to file a PR about this issue - I guess it is not
addressed by any of the follow-up issues and might get forgotten unless there
is PR.
* * *
libgomp/testsuite/libgomp.c++/baseptrs-4.C ... // Needs map clause
"lvalue"-parsing support. //#define REF2ARRAY_DECL_BASE
There is an open OpenMP issue to disallow some lvalues, namely:
OpenMP Issue 2618 ("Clarify behavior of mapping lvalues on target construct")
talks about code like the following
map(*p = 10)
map(x = 20)
map(x ? y[0] : p[1])
map(f(y))
is valid or not. The sentiment was to require that a 'map' clause list item
must have a base pointer or a base variable.
However, it looks as your examples would be valid in this regard. Can you file
a PR about this one? Referencing both to this testcase and to the OpenMP issue?
(I do note that Clang and GCC reject the lvalue examples from the OpenMP issue
but not your reference examples; those are accepted by clang++-14.)
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
[PATCH] Fortran: fix TARGET attribute of associating entity in ASSOCIATE [PR112764]
Dear all,
the attached simple patch fixes the handling of the TARGET
attribute of an associate variable in an ASSOCIATE construct.
See e.g. F2018:11.1.3.3 for a standard reference.
(Note that the patch does not touch the pointer or allocatable
attributes, as that would lead to several testsuite regressions
and thus needs more work.)
Regtested on x86_64-pc-linux-gnu. OK for mainline?
Thanks,
Harald
From 023dc4691c73ed594d5c1085f1aab897ca4a7153 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Wed, 29 Nov 2023 21:47:24 +0100
Subject: [PATCH] Fortran: fix TARGET attribute of associating entity in
ASSOCIATE [PR112764]
The associating entity in an ASSOCIATE construct has the TARGET attribute
if and only if the selector is a variable and has either the TARGET or
POINTER attribute (e.g. F2018:11.1.3.3).
gcc/fortran/ChangeLog:
PR fortran/112764
* primary.cc (gfc_variable_attr): Set TARGET attribute of associating
entity dependent on TARGET or POINTER attribute of selector.
gcc/testsuite/ChangeLog:
PR fortran/112764
* gfortran.dg/associate_62.f90: New test.
---
gcc/fortran/primary.cc | 16 ++
gcc/testsuite/gfortran.dg/associate_62.f90 | 25 ++
2 files changed, 41 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/associate_62.f90
diff --git a/gcc/fortran/primary.cc b/gcc/fortran/primary.cc
index d3aeeb89362..7278932b634 100644
--- a/gcc/fortran/primary.cc
+++ b/gcc/fortran/primary.cc
@@ -2653,6 +2653,22 @@ gfc_variable_attr (gfc_expr *expr, gfc_typespec *ts)
if (pointer || attr.proc_pointer)
target = 1;
+ /* F2018:11.1.3.3: Other attributes of associate names
+ "The associating entity does not have the ALLOCATABLE or POINTER
+ attributes; it has the TARGET attribute if and only if the selector is
+ a variable and has either the TARGET or POINTER attribute." */
+ if (sym->attr.associate_var && sym->assoc && sym->assoc->target)
+{
+ if (sym->assoc->target->expr_type == EXPR_VARIABLE)
+ {
+ symbol_attribute tgt_attr;
+ tgt_attr = gfc_expr_attr (sym->assoc->target);
+ target = (tgt_attr.pointer || tgt_attr.target);
+ }
+ else
+ target = 0;
+}
+
if (ts != NULL && expr->ts.type == BT_UNKNOWN)
*ts = sym->ts;
diff --git a/gcc/testsuite/gfortran.dg/associate_62.f90 b/gcc/testsuite/gfortran.dg/associate_62.f90
new file mode 100644
index 000..ce5bf286ee8
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/associate_62.f90
@@ -0,0 +1,25 @@
+! { dg-do compile }
+! PR fortran/112764
+! Contributed by martin
+
+program assoc_target
+ implicit none
+ integer, dimension(:,:), pointer :: x
+ integer, pointer :: j
+ integer, allocatable, target :: z(:)
+ allocate (x(1:100,1:2), source=1)
+ associate (i1 => x(:,1))
+j => i1(1)
+print *, j
+if (j /= 1) stop 1
+ end associate
+ deallocate (x)
+ allocate (z(3))
+ z(:) = [1,2,3]
+ associate (i2 => z(2:3))
+j => i2(1)
+print *, j
+if (j /= 2) stop 2
+ end associate
+ deallocate (z)
+end program assoc_target
--
2.35.3
