On 9/3/26 12:27 PM, Mikael Morin wrote:
From: Mikael Morin <[email protected]>
Hello,
this is the seventh part of the array descriptor series. The previous part,
still awaiting review, isolated the initialization of scalar descriptors to
three dedicated functions. These three functions do more or less the same
thing, but with subtle differences, either caused by bugs, useless code, or
different requirements or assumptions about their input.
This part fixes the bugs, removes useless differences and unifies the three
implementations.
Patches 1, 4 and 9 fix bugs in the descriptor initialization. They are
affecting either the elem_len value (patches 1 and 9) or the type value
(patch 4).
Patch 3 fixes a possible ICE bug.
Patch 6 removes the initialization of the span.
Patches 2 and 7 remove useless work in the descriptor initialization
generation.
Patches 5 and 8 factor common code to a shared function.
Fortran-tested on aarch64-unknown-linux-gnu. OK for mainline?
--
Previous parts in the array descriptor series:
part 6 (awaiting review): fortran: Move scalar descriptor init
https://inbox.sourceware.org/gcc-patches/[email protected]/
https://inbox.sourceware.org/fortran/[email protected]/
https://patchwork.sourceware.org/project/gcc/cover/[email protected]/
part 5 (pushed): fortran: Move null- or default-initialization
part 4 (pushed): fortran: Move dtype fields constants
part 3 (dropped): fortran: Ensure read-only getters
part 2 (pushed): fortran: Add getters and setters
part 1 (pushed): fortran: Move existing functions to a separate file
Mikael Morin (9):
fortran: array descriptor: Guess size from declared type [PR122521]
fortran: array descriptor: Simplify scalar dtype initialization
[PR122521]
fortran: array descriptor: Add a check for class container type
[PR122521]
fortran: array descriptor: Mark polymorphic descriptors as such
[PR122521]
fortran: array descriptor: Factor scalar descriptor init 1/2
[PR122521]
fortran: array descriptor: Don't set the span in the scalar case
[PR122521]
fortran: array descriptor: Remove redundant array type unwrapping
[PR122521]
fortran: array descriptor: Factor scalar descriptor init 2/2
[PR122521]
fortran: array descriptor: Set polymorphic elem_len value [PR122521]
gcc/fortran/trans-descriptor.cc | 149 +++++++++++++-----
gcc/fortran/trans-descriptor.h | 6 +-
gcc/fortran/trans-expr.cc | 4 +-
gcc/fortran/trans-types.cc | 46 +++++-
gcc/testsuite/gfortran.dg/assumed_rank_26.f90 | 26 +++
.../gfortran.dg/coarray_collectives_18.f90 | 2 +-
gcc/testsuite/gfortran.dg/sizeof_7.f90 | 30 ++++
gcc/testsuite/gfortran.dg/sizeof_8.f90 | 33 ++++
libgfortran/intrinsics/associated.c | 5 +-
9 files changed, 252 insertions(+), 49 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/assumed_rank_26.f90
create mode 100644 gcc/testsuite/gfortran.dg/sizeof_7.f90
create mode 100644 gcc/testsuite/gfortran.dg/sizeof_8.f90
Mikael,
This patch set looks mostly good to go. See the attached review report for a few
minor comments.
With those addressed Part 7 OK to go.
Thanks for the much needed work.
Regards,
Jerry
Review of "array descriptor part 7: Scalar descriptor init tweaks"
[PR122521], patches 1-9.
Applied on top of part 6 and regression-tested on
x86_64-pc-linux-gnu: 77278 passes, 349 expected failures, no
unexpected failures. The three new tests pass at all optimization
levels, and assumed_rank_26 also passes under -fsanitize=address.
Overall this is a clear improvement and unifying the three
implementations is the right endpoint. Two items need resolving
before it goes in (the ordering on part 6 below, and patch 6's stale
libgfortran hunk); the rest are questions.
== Series: depends on part 6 ==
The series depends on part 6 ("Move scalar descriptor init",
2026-08-29), still unreviewed. Part 6 does apply cleanly to current
mainline, so this is purely a question of taking it first.
== Patch 1: three questions and a nit ==
1. Scope. gfc_get_dtype_rank_type builds the dtype for every
descriptor, not only scalar ones. Making BT_CLASS take its size
from the declared type changes elem_len for any descriptor whose
element type is a class container. The ChangeLog mentions only
the scalar assumed-rank case -- is the wider change intended?
2. Bisection window. For unlimited polymorphic, etype now comes out
void, size stays NULL, and the dtype constructor omits elem_len,
so it reads 0 until patch 9 sets it at run time. Harmless if the
series lands as a unit, but worth a note in patch 1.
3. Dropped assert. BT_VOID went from
gcc_assert (TREE_CODE (ptype) == POINTER_TYPE);
size = size_in_bytes (ptype);
to leaving size NULL when ptype is not a pointer. FUNCTION_TYPE
also maps to BT_VOID, so procedure-typed descriptors now silently
get elem_len 0 instead of tripping the assert. Would allowing
only VOID_TYPE through, and still asserting otherwise, be better?
Nit, same function: the explanatory comment sits between the break
and the fall-through marker, reading as if it belonged to the break:
case BT_CLASS:
if (VOID_TYPE_P (etype))
break;
/* For classes, the element length isn't a known constant, ... */
/* Fall through. */
default:
Clearer above the case label.
== Patch 2: OK ==
Dropping gfc_get_scalar_to_descriptor_type also drops the
gfc_expr_attr() input, which looked like it might lose information.
It does not: gfc_build_dtype_constructor never populates the
attribute field, and the only other effect of the round trip was
caching GFC_TYPE_ARRAY_DTYPE on a throwaway type. Good removal.
== Patch 3: OK ==
Good minimal reproducer, and dg-do compile is right for an ICE fix.
See patch 4 for a follow-up cleanup of the check it adds.
== Patch 4: gcc_unreachable, and duplicated ref walking ==
1. gcc_unreachable on the unexpected shape:
tree class_ref;
if (!is_polymorphic_ref (scalar, &class_ref))
gcc_unreachable ();
Patch 3, immediately before, exists precisely because the input is
not always a class container reference. Turning the remaining
unexpected shapes into a hard abort trades a wrong-but-working
result for an ICE. If the invariant does hold after patch 3,
gcc_checking_assert states it without aborting release compilers.
Also class_ref is left uninitialized on the false path, which some
configurations will flag as -Wmaybe-uninitialized.
2. Duplicated ref walking. After this patch, is_polymorphic_ref has
already located the container, yet the test patch 3 added
re-derives it:
if (GFC_CLASS_TYPE_P (TREE_TYPE (tmp))
|| (POINTER_TYPE_P (TREE_TYPE (tmp))
&& GFC_CLASS_TYPE_P (TREE_TYPE (TREE_TYPE (tmp)))))
tmp = gfc_class_data_get (tmp);
Deriving tmp from class_ref would drop it. Keeping patch 3
separate makes sense if it may want backporting, but patch 4 could
clean up.
== Patch 5: no comments ==
== Patch 6: stale libgfortran hunk; span left indeterminate ==
1. The libgfortran hunk is stale (needs resolving). Mainline
5e8f400f12c (PR126964, 2026-09-01) rewrote that check two days
before this posting. associated() now reads:
/* Require that the storage sequences are the same. */
if (GFC_DESCRIPTOR_SIZE (pointer) != GFC_DESCRIPTOR_SIZE (target)
&& GFC_DESCRIPTOR_SPAN (pointer) != GFC_DESCRIPTOR_SPAN (target))
return 0;
The line the patch deletes no longer exists, so patch 6 does not
apply. I tested with that hunk dropped; nothing in the testsuite
depended on it.
2. The span is left indeterminate, not zeroed. This is my main
concern. Removing the initialization does not set the span to
anything -- it leaves whatever the descriptor variable held.
gfc_conv_scalar_to_descriptor builds it with gfc_create_var, so
that is indeterminate stack contents, not zero. Confirmed by tree
dump for a scalar passed to an assumed-rank dummy:
before: desc.data = (void * restrict) &i;
desc.span = (integer(kind=8)) desc.0.dtype.elem_len;
after: desc.data = (void * restrict) &i;
Two reasons this matters more than "a scalar has one element":
- The library already defines a meaning for an unset span. See
stride_in_bytes() in associated.c:
index_type span = GFC_DESCRIPTOR_SPAN (desc);
if (span == 0)
span = GFC_DESCRIPTOR_SIZE (desc);
That contract wants 0, and 0 is exactly what the patch does
not write.
- There is a rank-0 span read that is not guarded by rank. In
libgfortran/caf/single.c:696:
size_t dsize = opt_dst_desc->span;
for (int i = 0; i < GFC_DESCRIPTOR_RANK (opt_dst_desc); ++i)
dsize *= GFC_DESCRIPTOR_EXTENT (opt_dst_desc, i);
memcpy (old_dst_data_ptr, opt_dst_desc->base_addr, dsize);
For rank 0 the loop body never executes, so dsize *is* the
span and becomes a memcpy length directly.
Also worth noting the blast radius: gfc_conv_scalar_to_descriptor
has 19 call sites across trans.cc, trans-array.cc, trans-expr.cc,
trans-decl.cc, trans-intrinsic.cc and trans-openmp.cc, so this is
not confined to the polymorphic-scalar case.
Suggestion: set the span explicitly to 0 in the scalar case rather
than not setting it. That honours the convention the library
already implements, costs one store, and makes the libgfortran
hunk in item 1 unnecessary.
I did not manage to build a failing testcase -- scalar and class
coarray transfers behave identically before and after in both
-fcoarray=single and -fcoarray=lib -- so this is latent rather
than demonstrated.
== Patch 7: OK ==
The unwrapping really is redundant. For pointer-to-array-of-character
both forms leave ptype as the ARRAY_TYPE, so the BT_CHARACTER assert
and length still hold.
== Patch 8: OK ==
Straightforward.
== Patch 9: unlimited polymorphic character ==
gfc_vptr_size_get gives the vptr _size with no _len scaling, so for
CLASS(*) holding a character the element length is the kind size, not
the character length:
character(len=7) :: c
class(*), allocatable :: y; allocate (y, source=c)
! passed on to a CLASS(*) :: a(..) dummy
sizeof(a) => 1, expected 7
This is pre-existing -- I measured the same value with and without
the series, so it is not a regression -- but since patch 9's purpose
is to get the polymorphic elem_len right, it looks like an
incompleteness. gfc_resize_class_size_with_len (trans.cc) exists for
this, and gfc_get_span already uses it on the equivalent path. Was
the unlimited polymorphic case meant to be covered here?
One thing I checked and withdrew: I initially thought the vptr load
could fault for an absent optional polymorphic actual, since the guard
tests the vptr value and loading it dereferences the container. It
cannot: gfc_conv_class_to_class wraps the whole block in a presence
COND_EXPR, so the load is already guarded. Verified at run time with
an absent optional class scalar passed to an assumed-rank dummy.
