Re: [Ping*3, Patch, Fortran, 77871, v1] Allow for class typed coarray parameter as dummy [PR77871]
Hi all,
this patch somehow slipped my attention. Anyone for a review? Third time ping!
Rebased to current mainline.
Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
Regards,
Andre
On Wed, 18 Sep 2024 12:30:23 +0200
Andre Vehreschild wrote:
> Hi all,
>
> back from my holidays and still no review. PING PING!
>
> Rebased to current mainline.
>
> Regtested ok on x86_64-pc-linux-gnu / F39. Ok for mainline?
>
> Regards,
> Andre
>
> On Wed, 21 Aug 2024 13:43:52 +0200
> Andre Vehreschild wrote:
>
> > Hi all,
> >
> > pinging this patch for the first time.
> >
> > Rebased and regtested ok on x86_64-pc-linux-gnu / Fedora 39. Ok for
> > mainline?
> >
> > - Andre
> >
> > On Thu, 15 Aug 2024 14:39:25 +0200
> > Andre Vehreschild wrote:
> >
> > > Hi all,
> > >
> > > attached patch fixes another regression on coarrays. This time for class
> > > typed coarrays as dummys.
> > >
> > > Regtested ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
> > >
> > > Regards,
> > > Andre
> > > --
> > > Andre Vehreschild * Email: vehre ad gmx dot de
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
--
Andre Vehreschild * Email: vehre ad gmx dot de
From 48e77542f0e3342c5da31ecce1b229fa3fbbdaa2 Mon Sep 17 00:00:00 2001
From: Andre Vehreschild
Date: Thu, 15 Aug 2024 13:49:49 +0200
Subject: [PATCH] [Fortran] Allow for class type coarray parameters. [PR77871]
gcc/fortran/ChangeLog:
PR fortran/77871
* trans-expr.cc (gfc_conv_derived_to_class): Assign token when
converting a coarray to class.
(gfc_get_tree_for_caf_expr): For classes get the caf decl from
the saved descriptor.
(gfc_get_caf_token_offset):Assert that coarray=lib is set and
cover more cases where the tree having the coarray token can be.
* trans-intrinsic.cc (gfc_conv_intrinsic_caf_get): Use unified
test for pointers.
gcc/testsuite/ChangeLog:
* gfortran.dg/coarray/dummy_3.f90: New test.
---
gcc/fortran/trans-expr.cc | 36 ---
gcc/fortran/trans-intrinsic.cc| 2 +-
gcc/testsuite/gfortran.dg/coarray/dummy_3.f90 | 33 +
3 files changed, 58 insertions(+), 13 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/coarray/dummy_3.f90
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index 9f223a1314a..4065ea2a735 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -810,6 +810,16 @@ gfc_conv_derived_to_class (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym,
/* Now set the data field. */
ctree = gfc_class_data_get (var);
+ if (flag_coarray == GFC_FCOARRAY_LIB && CLASS_DATA (fsym)->attr.codimension)
+{
+ tree token;
+ tmp = gfc_get_tree_for_caf_expr (e);
+ if (POINTER_TYPE_P (TREE_TYPE (tmp)))
+ tmp = build_fold_indirect_ref (tmp);
+ gfc_get_caf_token_offset (parmse, &token, nullptr, tmp, NULL_TREE, e);
+ gfc_add_modify (&parmse->pre, gfc_conv_descriptor_token (ctree), token);
+}
+
if (optional)
cond_optional = gfc_conv_expr_present (e->symtree->n.sym);
@@ -2344,6 +2354,10 @@ gfc_get_tree_for_caf_expr (gfc_expr *expr)
if (expr->symtree->n.sym->ts.type == BT_CLASS)
{
+ if (DECL_P (caf_decl) && DECL_LANG_SPECIFIC (caf_decl)
+ && GFC_DECL_SAVED_DESCRIPTOR (caf_decl))
+ caf_decl = GFC_DECL_SAVED_DESCRIPTOR (caf_decl);
+
if (expr->ref && expr->ref->type == REF_ARRAY)
{
caf_decl = gfc_class_data_get (caf_decl);
@@ -2408,16 +2422,12 @@ gfc_get_caf_token_offset (gfc_se *se, tree *token, tree *offset, tree caf_decl,
{
tree tmp;
+ gcc_assert (flag_coarray == GFC_FCOARRAY_LIB);
+
/* Coarray token. */
if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (caf_decl)))
-{
- gcc_assert (GFC_TYPE_ARRAY_AKIND (TREE_TYPE (caf_decl))
- == GFC_ARRAY_ALLOCATABLE
- || expr->symtree->n.sym->attr.select_type_temporary
- || expr->symtree->n.sym->assoc);
*token = gfc_conv_descriptor_token (caf_decl);
-}
- else if (DECL_LANG_SPECIFIC (caf_decl)
+ else if (DECL_P (caf_decl) && DECL_LANG_SPECIFIC (caf_decl)
&& GFC_DECL_TOKEN (caf_decl) != NULL_TREE)
*token = GFC_DECL_TOKEN (caf_decl);
else
@@ -2435,7 +2445,7 @@ gfc_get_caf_token_offset (gfc_se *se, tree *token, tree *offset, tree caf_decl,
&& (GFC_TYPE_ARRAY_AKIND (TREE_TYPE (caf_decl)) == GFC_ARRAY_ALLOCATABLE
|| GFC_TYPE_ARRAY_AKIND (TREE_TYPE (caf_decl)) == GFC_ARRAY_POINTER))
*offset = build_int_cst (gfc_array_index_type, 0);
- else if (DECL_LANG_SPECIFIC (caf_decl)
+ else if (DECL_P (caf_decl) && DECL_LANG_SPECIFIC (caf_decl)
&& GFC_DECL_CAF_OFFSET (caf_decl) != NULL_TREE)
*offset = GFC_DECL_CAF_OFFSET (caf_decl);
else if (GFC_TYPE_ARRAY_CAF_OFFSET (TREE_TYPE (caf_decl)) != NULL_TREE)
@@ -2502,11 +2512,13 @@ gfc_get_caf_token_offset (gfc_se *se, tree *token, tree *offset, tree caf_decl,
}
else if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (caf_decl)))
tmp = gfc_con
Re: [Fortran, Patch, PR51815, v3] Fix parsing of substring refs in coarrays.
Hi Andre,
On 10/7/24 11:04, Andre Vehreschild wrote:
Hi Harald,
thank you for your input. I still have some small nits to discuss to make
everyone happy. Therefore:
this seems to go into the right direction - except that I am not a
great fan of gfc_error_now, as that tries to paper over deficiencies
in error recovery.
Me either, but when I remove the gfc_error_now() and only do
if (gfc_peek_ascii_char () == '(')
return MATCH_ERROR;
as you proposed, then no error is given for:
character(:), allocatable :: x[:]
character(:), allocatable :: c
c = x(:)(2:5)
I.e. nothing at all.
hmmm, without the hunk in question I do get:
4 | c = x(:)(2:5)
| 1
Error: Unclassifiable statement at (1)
which is the same when doing a return MATCH_ERROR;
When I simply use:
if (gfc_peek_ascii_char () == '(')
{
gfc_error ("Unexpected array/substring ref at %C");
return MATCH_ERROR;
}
this already generates:
4 | c = x(:)(2:5)
| 1
Error: Unexpected array/substring ref at (1)
> Therefore at the moment I prefer to stick to the initial> solution
with the gfc_error_now, which not only gives an error in the
associate, but also when one just does an array/substring-ref outside of
parentheses. And I like the new error message, because I consider it more
helpful than just a syntax error or the invalid association target message.
What do you think?
The motivation for my asking is based on the following naive thinking
(assuming that x is of type character):
x(:)(2:5)! could be a rank mismatch when x is an array
x[1](:)(2:5) ! is always a syntax error
x(:)[1](2:5) ! could by diagnosed as a rank mismatch
That is of course wishful thinking on my side. No compiler
matches this completely, and diagnosing a syntax error is
certainly acceptable behavior. (Some other brand shows funny
diagnostics coming likely from the resolution phase).
Is there a reason that you do not check the return value of
gfc_match_array_ref?
What am I to do with the result? We are in an error case independent of the
result of gfc_match_array_ref. The intention of using that routine here was to
digest the unexpected input and allow for (easier|better) error recovery.
Do you have an example that shows the use of gfc_match_array_ref here?
Commenting it out doesn't seem to make a difference in the error case
here, unless I missed something.
> May> be I should just put a comment on it, to make it more clear. Or
is there
another way to help the parser recover from an error?
Well, I am not the expert to answer that. Without gfc_error_now,
we're more likely seeing errors coming from the parsing of the
associate, and here I would point to Paul as the one with the most
experience. I would hope that the parsing of associate would see
if an error was issued for the associate target and allow that error
to be emitted.
Sorry for the additional round. But this error has been around for so long,
that it doesn't matter, if we need another day to come up with a solution.
Indeed! :-)
Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
I am fine with your solution. Diagnostics can be improved later
any time...
Regards,
Andre
Thanks for your patience!
Harald
Indeed your suggestion (or the shortened version above) improves
the diagnostics ("user experience") also for this variant:
subroutine foo
character(:), allocatable :: x[:]
character(:), dimension(:), allocatable :: c[:]
type t
character(:), allocatable :: x[:]
character(:), dimension(:), allocatable :: c[:]
end type t
type(t) :: z
associate (y => x(:)(2:))
end associate
associate (a => c(:)(:)(2:))
end associate
associate (y => z%x(:)(2:))
end associate
associate (a => z%c(:)(:)(2:))
end associate
end
with several error messages of the kind
Error: Invalid association target at (1)
or
Error: Rank mismatch in array reference at (1) (1/0)
looking less technical than a parsing error.
I think this is as good as it can be.
So OK from my side with either your additional patch or my
shortened version.
Thanks for the patch!
Harald
Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Is this ok?
Regards and thanks for the review,
Andre
On Tue, 1 Oct 2024 23:31:11 +0200
Harald Anlauf wrote:
Hi Andre,
Am 01.10.24 um 09:43 schrieb Andre Vehreschild:
Hi all,
this rather old PR reported a parsing bug, when a coarray'ed character
substring ref is to be parsed, aka CHARACTER(:) :: str[:] ... str(2:5). In
this case the parser confused the substring ref with an array-ref, because
an array_spec was present. This patch fixes this by requesting only
coarray parsing from gfc_match_array_ref when no regular dimension is
present. The patch is not involved when an array of coarray'ed strings is
parsed (that worked beforehand).
while the patch address
Re: [r15-4104 Regression] FAIL: gfortran.dg/gomp/allocate-static.f90 -Os (test for excess errors) on Linux/x86_64
Hi Tobias!
On 2024-10-07T17:07:05+0200, Tobias Burnus wrote:
> haochen.jiang wrote:
>> On Linux/x86_64,
>> FAIL: gfortran.dg/gomp/allocate-static.f90 -O0 (test for excess errors)
>
> If anyone can reproduce this, I would be interested in the excess errors.
gfortran: fatal error: cannot read spec file 'libgomp.spec': No such file
or directory
> On two machines – with and without offloading configured – I cannot
> reproduce this neither with a bootsstrap nor non-bootstrap build,
> neither with the testsuite nor under valgrind and also not with -m32 vs.
> -m64.
Try again with build-tree (non-installed) testing. ;-)
On 2024-10-07T10:47:56+0200, Tobias Burnus wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/allocate-static.f90
> @@ -0,0 +1,62 @@
> +! { dg-do run }
Implicit linking here.
I already was about to 'git mv' the file into
'libgomp/testsuite/libgomp.fortran/' -- but then realized that we
probably also should get rid of this local 'module omp_lib_kinds':
> +module omp_lib_kinds
> + use iso_c_binding, only: c_int, c_intptr_t
> + implicit none
> + private :: c_int, c_intptr_t
> + integer, parameter :: omp_allocator_handle_kind = c_intptr_t
> +
> + integer (kind=omp_allocator_handle_kind), &
> + parameter :: omp_null_allocator = 0
> + [...]
> +end module
..., right?
> +[...]
Grüße
Thomas
Re: [Patch] OpenMP: Allocate directive for static vars, clean up
Hi Andre,
first, thanks a lot for all your proof reading of patches! That's indeed
helpful and reviewing (with offical LGTM stamp or as bystander) is a
problem, you help to reduce it! :-)
Andre Vehreschild wrote:
@@ -821,6 +821,23 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
+ if (sym->attr.omp_allocate && TREE_STATIC (decl))
+{
+ struct gfc_omp_namelist *n;
+ for (n = sym->ns->omp_allocate; n; n = n->next)
+ if (n->sym == sym)
+ break;
Theoretically n can be NULL here. This would then ICE. Or is there a guarantee,
that n is never NULL
+ tree alloc = gfc_conv_constant_to_tree (n->u2.allocator);
One should never say never, but I think it should never be NULL. In openmp.cc:
gfc_resolve_omp_allocate (gfc_namespace *ns, gfc_omp_namelist *list)
{
for (gfc_omp_namelist *n = list; n; n = n->next)
...
n->sym->attr.omp_allocate = 1;
And the caller is in resolve.cc:
if(ns->omp_allocate)
gfc_resolve_omp_allocate (ns, ns->omp_allocate);
Cheers,
Tobias
PS: If you wonder about modules: It is not saved in .mod files. As it
about allocating a variable, this property is only required where the
variable is actually defined/has storage not where it is only accessed.
Otherwise, that would be a loop hole.
Re: [Patch] OpenMP: Allocate directive for static vars, clean up
Now committed as r15-4104-ga8caeaacf499d5.
With a wording improvement in the commit log and avoiding an XPASS for
C++ by excluding c++98 from the xfail in dg-bogus... xfail.
Tobias
Tobias Burnus wrote:
'omp allocate' permits to use a different (specified) allocator and
alignment for both stack/automatic and static/saved variables; the latter
takes only predefined allocators. Currently, only C and Fortran are
support for stack/automatic variables; static variables are rejected
before the attached patch. (For them, only predefined allocators are
permitted.)
* * *
I happened to look at the 'allocate' directive recently and, doing so,
I stumbled over a couple of issues, which the attached patch addresses
(missing diagnostics for corner cases, not updated checks, unhelpful
documentation ['allocate' *clause*], ...). Doing so, I wondered whether:
Shouldn't we just accept 'omp allocate' for static
variables by just honoring the aligning and ignoring the actually
requested
allocator? - First, we do already the same for actual allocations as
not all
traits are supported. And for the host this seems to be the most
sensible to
do in any case.
[For some use cases, pointers + allocation in the constructor would be
better, but in general, not adding an indirection seems to be better and
has fewer corner-case usability issue.]
I guess we later want to honor the requested memory for nvptx and/or
gcn; at
least Nvidia GPUs could make use for constant memory (having
advantages for
reading the same memory by many threads/broadcasting it). I guess
OpenACC 2.7's
'readonly' modifier serves a similar purpose.
For now we don't, but the attribute is passed on to the backends,
which could
make use of them, if desired. ('groupprivate' directive vs. cgroup/thread
allocators are similar device-only features.)
As mentioned, this patch also fixes a few other issues here and there,
see
commit log and source code for details.
Code comments? Suggestions or remarks? - Before I apply this patch?
Tobias
PS: I am aware that C++ support is lacking. There is a pending patch
that needs
to be updated for this patch, probably some bitrotting, and in
particular for the
review comments, cf.
https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633782.html
and https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639929.htmlcommit a8caeaacf499d58ba7ceabc311b7b71ca806f740
Author: Tobias Burnus
Date: Mon Oct 7 10:45:14 2024 +0200
OpenMP: Allocate directive for static vars, clean up
For the 'allocate' directive, remove the sorry for static variables and
just keep using normal memory, but honor the requested alignment and set
a DECL_ATTRIBUTE in case a target may want to make use of this later on.
The documentation is updated accordingly.
The C diagnostic to check for predefined allocators (req. for static vars)
failed to accept GCC's ompx_gnu_... allocator, now fixed. (Fortran was
already okay; but both now use new common #defined value for checking.)
And while Fortran common block variables are still rejected, the check
has been improved as before the sorry diagnostic did not work for
common blocks in modules.
Finally, for 'allocate' clause on the target/task/taskloop directives,
there is now a warning for omp_thread_mem_alloc (i.e. predefined allocator
with access = thread), which is undefined behavior according to the
OpenMP specification.
And, last, testing showed that var decl + static_assert sets TREE_USED
but does not produce a statement list in C, which did run into an assert
in gimplify. This special case is now also handled.
gcc/c/ChangeLog:
* c-parser.cc (c_parser_omp_allocate): Set alignment for alignof;
accept static variables and fix predef allocator check.
gcc/fortran/ChangeLog:
* openmp.cc (is_predefined_allocator): Use gomp-constants.h consts.
* trans-common.cc (translate_common): Reject OpenMP allocate directives.
* trans-decl.cc (gfc_finish_var_decl): Handle allocate directive
for static variables.
(gfc_trans_deferred_vars): Update for the latter.
gcc/ChangeLog:
* gimplify.cc (gimplify_bind_expr): Fix corner case for OpenMP
allocate directive.
(gimplify_scan_omp_clauses): Warn if omp_thread_mem_alloc is used
as allocator with the target/task/taskloop directive.
include/ChangeLog:
* gomp-constants.h (GOMP_OMP_PREDEF_ALLOC_MAX,
GOMP_OMPX_PREDEF_ALLOC_MIN, GOMP_OMPX_PREDEF_ALLOC_MAX,
GOMP_OMP_PREDEF_ALLOC_THREADS): New defines.
libgomp/ChangeLog:
* allocator.c: Add static asserts for news
GOMP_OMP{,X}_PREDEF_ALLOC_{MIN,MAX} range values.
* libgomp.texi (OpenMP Impl. Status): Allocate directive for
static vars is now supported. Refer
Re: [Fortran, Patch, PR51815, v3] Fix parsing of substring refs in coarrays.
Hi Harald,
thank you for your input. I still have some small nits to discuss to make
everyone happy. Therefore:
> this seems to go into the right direction - except that I am not a
> great fan of gfc_error_now, as that tries to paper over deficiencies
> in error recovery.
Me either, but when I remove the gfc_error_now() and only do
> if (gfc_peek_ascii_char () == '(')
> return MATCH_ERROR;
as you proposed, then no error is given for:
character(:), allocatable :: x[:]
character(:), allocatable :: c
c = x(:)(2:5)
I.e. nothing at all. Therefore at the moment I prefer to stick to the initial
solution with the gfc_error_now, which not only gives an error in the
associate, but also when one just does an array/substring-ref outside of
parentheses. And I like the new error message, because I consider it more
helpful than just a syntax error or the invalid association target message.
What do you think?
> Is there a reason that you do not check the return value of
> gfc_match_array_ref?
What am I to do with the result? We are in an error case independent of the
result of gfc_match_array_ref. The intention of using that routine here was to
digest the unexpected input and allow for (easier|better) error recovery. May
be I should just put a comment on it, to make it more clear. Or is there
another way to help the parser recover from an error?
Sorry for the additional round. But this error has been around for so long,
that it doesn't matter, if we need another day to come up with a solution.
Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
Regards,
Andre
> Indeed your suggestion (or the shortened version above) improves
> the diagnostics ("user experience") also for this variant:
>
> subroutine foo
> character(:), allocatable :: x[:]
> character(:), dimension(:), allocatable :: c[:]
> type t
>character(:), allocatable :: x[:]
>character(:), dimension(:), allocatable :: c[:]
> end type t
> type(t) :: z
> associate (y => x(:)(2:))
> end associate
> associate (a => c(:)(:)(2:))
> end associate
> associate (y => z%x(:)(2:))
> end associate
> associate (a => z%c(:)(:)(2:))
> end associate
> end
>
> with several error messages of the kind
>
> Error: Invalid association target at (1)
>
> or
>
> Error: Rank mismatch in array reference at (1) (1/0)
>
> looking less technical than a parsing error.
> I think this is as good as it can be.
>
> So OK from my side with either your additional patch or my
> shortened version.
>
> Thanks for the patch!
>
> Harald
>
>
> > Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Is this ok?
> >
> > Regards and thanks for the review,
> > Andre
> >
> > On Tue, 1 Oct 2024 23:31:11 +0200
> > Harald Anlauf wrote:
> >
> >> Hi Andre,
> >>
> >> Am 01.10.24 um 09:43 schrieb Andre Vehreschild:
> >>> Hi all,
> >>>
> >>> this rather old PR reported a parsing bug, when a coarray'ed character
> >>> substring ref is to be parsed, aka CHARACTER(:) :: str[:] ... str(2:5). In
> >>> this case the parser confused the substring ref with an array-ref, because
> >>> an array_spec was present. This patch fixes this by requesting only
> >>> coarray parsing from gfc_match_array_ref when no regular dimension is
> >>> present. The patch is not involved when an array of coarray'ed strings is
> >>> parsed (that worked beforehand).
> >>
> >> while the patch addresses the issue mentioned in the PR,
> >>
> >>> I had to fix the dg-error clauses in the testcase pr102532 because now the
> >>> error of having to many refs is detected by the parsing stage and no
> >>> longer by the resolve stage. It has become a simple syntax error. I hope
> >>> this is ok.
> >>
> >> I find the error messages now less helpful to users: before the patch
> >> we got "Rank mismatch in array reference", which was more suitable
> >> than the newer version with more or less confusing syntax errors.
> >>
> >> I assume you tried to find a better solution - but Intel and NAG
> >> also give syntax errors - so basically I am fine with the patch.
> >>
> >> You may want to wait for a second opinion. If nobody else responds
> >> within the next 2 days, you may proceed nevertheless.
> >>
> >> Thanks,
> >> Harald
> >>
> >>> Regtests ok on x86_64-pc-linux-gnu / Fedora 39. Ok for mainline?
> >>>
> >>> Regards,
> >>> Andre
> >>> --
> >>> Andre Vehreschild * Email: vehre ad gmx dot de
> >>
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de
> >
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
From bf33a961a501e7a31f510518830e420a3f1e3b78 Mon Sep 17 00:00:00 2001
From: Andre Vehreschild
Date: Tue, 1 Oct 2024 09:30:59 +0200
Subject: [PATCH] Fix parsing of substring refs in coarrays. [PR51815]
The parser was greadily taking the substring ref as an array ref because
an array_spec was present. Fix this by only parsing the coarray (pseudo)
ref when no regular array is present.
gcc/fortran/ChangeLog:
PR fortran/5181
Re: [patch, fortran] Implement maxloc and minloc for unsigned
Hi Thomas, this patch looks very similar to previous ones. I therefore deem it ok for mainline. May be you want to wait for Jerry to voice his ok/findings before you apply it. Thanks for the patch. - Andre On Sun, 6 Oct 2024 08:58:30 -0700 Jerry Delisle wrote: > The iparm.m4 was what I was looking for. Thanks, > > Jerry > > On Sun, Oct 6, 2024, 5:26 AM Thomas Koenig wrote: > > > Am 05.10.24 um 22:52 schrieb Thomas Koenig: > > > Hi Jerry, > > > > > >> I see all the generated files in the patch, but I do not see the M4 > > >> macro or other mechanism which generated these. Was this in a > > >> previous submission that I missed? > > > > > > The "magic" in this case is mentioning them in Makefile.am and > > > (regenerated) in Makefile.in. > > > > > > The rest is done by the previous modification to m4/iparm.m4 in > > > 5d98fe096b5d17021875806ffc32ba41ea0e87b0 , which generates > > > the type from the file name, in this case containing the 'm' > > > as the type-specifying letter. > > > > To be a little bit more elaborate... > > > > Since the patch which removed some machinery warnings, you have to use > > --enable-maintainer-mode (and do the above). The generated files will > > then be in (assuming your build directory is trunk/bin and the > > architecture is AMD64 on Linux) > > trunk-bin/x86_64-pc-linux-gnu/libgfortran/generated > > and you then have to copy what's new over to > > trunk/libgfortran/generated , then do a second bootstrap, preferably in > > a separate directory) without --enable-maintainer-mode to see if > > everything works right. > > > > Best regards > > > > Thomas > > > > > > -- Andre Vehreschild * Email: vehre ad gmx dot de
Re: [Patch] OpenMP: Allocate directive for static vars, clean up
Hi Tobias,
just a question:
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 8231bd255d6..2586c6d7a79 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -821,6 +821,23 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
&& (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
set_decl_tls_model (decl, decl_default_tls_model (decl));
+ if (sym->attr.omp_allocate && TREE_STATIC (decl))
+{
+ struct gfc_omp_namelist *n;
+ for (n = sym->ns->omp_allocate; n; n = n->next)
+ if (n->sym == sym)
+ break;
Theoretically n can be NULL here. This would then ICE. Or is there a guarantee,
that n is never NULL
+ tree alloc = gfc_conv_constant_to_tree (n->u2.allocator);
I just looked at the fortran part.
Thanks for the patch and regards,
Andre
On Mon, 7 Oct 2024 10:47:56 +0200
Tobias Burnus wrote:
> Now committed as r15-4104-ga8caeaacf499d5.
>
> With a wording improvement in the commit log and avoiding an XPASS for
> C++ by excluding c++98 from the xfail in dg-bogus... xfail.
>
> Tobias
>
> Tobias Burnus wrote:
> > 'omp allocate' permits to use a different (specified) allocator and
> > alignment for both stack/automatic and static/saved variables; the latter
> > takes only predefined allocators. Currently, only C and Fortran are
> > support for stack/automatic variables; static variables are rejected
> > before the attached patch. (For them, only predefined allocators are
> > permitted.)
> >
> > * * *
> >
> > I happened to look at the 'allocate' directive recently and, doing so,
> > I stumbled over a couple of issues, which the attached patch addresses
> > (missing diagnostics for corner cases, not updated checks, unhelpful
> > documentation ['allocate' *clause*], ...). Doing so, I wondered whether:
> >
> > Shouldn't we just accept 'omp allocate' for static
> > variables by just honoring the aligning and ignoring the actually
> > requested
> > allocator? - First, we do already the same for actual allocations as
> > not all
> > traits are supported. And for the host this seems to be the most
> > sensible to
> > do in any case.
> > [For some use cases, pointers + allocation in the constructor would be
> > better, but in general, not adding an indirection seems to be better and
> > has fewer corner-case usability issue.]
> >
> > I guess we later want to honor the requested memory for nvptx and/or
> > gcn; at
> > least Nvidia GPUs could make use for constant memory (having
> > advantages for
> > reading the same memory by many threads/broadcasting it). I guess
> > OpenACC 2.7's
> > 'readonly' modifier serves a similar purpose.
> > For now we don't, but the attribute is passed on to the backends,
> > which could
> > make use of them, if desired. ('groupprivate' directive vs. cgroup/thread
> > allocators are similar device-only features.)
> >
> > As mentioned, this patch also fixes a few other issues here and there,
> > see
> > commit log and source code for details.
> >
> > Code comments? Suggestions or remarks? - Before I apply this patch?
> >
> > Tobias
> >
> > PS: I am aware that C++ support is lacking. There is a pending patch
> > that needs
> > to be updated for this patch, probably some bitrotting, and in
> > particular for the
> > review comments, cf.
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633782.html
> > and https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639929.html
--
Andre Vehreschild * Email: vehre ad gmx dot de
Re: [patch, fortran] Implement maxloc and minloc for unsigned
I think your good to go. Thanks for patch! On Mon, Oct 7, 2024, 2:12 AM Andre Vehreschild wrote: > Hi Thomas, > > this patch looks very similar to previous ones. I therefore deem it ok for > mainline. May be you want to wait for Jerry to voice his ok/findings > before you > apply it. > > Thanks for the patch. > > - Andre > > On Sun, 6 Oct 2024 08:58:30 -0700 > Jerry Delisle wrote: > > > The iparm.m4 was what I was looking for. Thanks, > > > > Jerry > > > > On Sun, Oct 6, 2024, 5:26 AM Thomas Koenig > wrote: > > > > > Am 05.10.24 um 22:52 schrieb Thomas Koenig: > > > > Hi Jerry, > > > > > > > >> I see all the generated files in the patch, but I do not see the M4 > > > >> macro or other mechanism which generated these. Was this in a > > > >> previous submission that I missed? > > > > > > > > The "magic" in this case is mentioning them in Makefile.am and > > > > (regenerated) in Makefile.in. > > > > > > > > The rest is done by the previous modification to m4/iparm.m4 in > > > > 5d98fe096b5d17021875806ffc32ba41ea0e87b0 , which generates > > > > the type from the file name, in this case containing the 'm' > > > > as the type-specifying letter. > > > > > > To be a little bit more elaborate... > > > > > > Since the patch which removed some machinery warnings, you have to use > > > --enable-maintainer-mode (and do the above). The generated files will > > > then be in (assuming your build directory is trunk/bin and the > > > architecture is AMD64 on Linux) > > > trunk-bin/x86_64-pc-linux-gnu/libgfortran/generated > > > and you then have to copy what's new over to > > > trunk/libgfortran/generated , then do a second bootstrap, preferably in > > > a separate directory) without --enable-maintainer-mode to see if > > > everything works right. > > > > > > Best regards > > > > > > Thomas > > > > > > > > > > > > -- > Andre Vehreschild * Email: vehre ad gmx dot de >
[committed] Move gfortran.dg/gomp/allocate-static.f90 to libgomp.fortran/ (was: [r15-4104 Regression] FAIL: gfortran.dg/gomp/allocate-static.f90 -Os (test for excess errors) on Linux/x86_64)
Committed as r15-4127-gb95ad25f9c9376
Hi Thomas,
Thomas Schwinge wrote:
On 2024-10-07T17:07:05+0200, Tobias Burnus wrote:
If anyone can reproduce this, I would be interested in the excess errors.
gfortran: fatal error: cannot read spec file 'libgomp.spec': No such file
or directory
Aha. Thanks! — I am in principle aware of it, but tend to forget it from
time to time, especially when only later turning a compile-time test
into a runtime one …
In principle, a compile-time only check would enough (as with the C
testcase) if the alignment were visible in the dump.
(It is, kind of, with C/C++, using alignof; but for Fortran it isn't. On
the other hand, checking it at runtime ensures that it really works.)
I already was about to 'git mv' the file into
'libgomp/testsuite/libgomp.fortran/' -- but then realized that we
probably also should get rid of this local 'module omp_lib_kinds':
Yes, if 'omp_lib_kinds.mod' is available, there is no point to define it
locally.
Tobias
commit b95ad25f9c9376575dcde4bcb529d3ca31b27359
Author: Tobias Burnus
Date: Mon Oct 7 23:57:42 2024 +0200
Move gfortran.dg/gomp/allocate-static.f90 to libgomp.fortran/
The testcase was turned into a 'dg-do run' check to check for the alignment,
but this only works in testsuite/gfortran.dg, causing link errors for
out-of-tree testing. The test was added in r15-4104-ga8caeaacf499d5.
gcc/testsuite/:
* gfortran.dg/gomp/allocate-static.f90: Move to libgomp/testsuite/.
libgomp/:
* testsuite/libgomp.fortran/allocate-static.f90: Moved from
gcc/testsuite/ as it is a dg-do run test; use real omp_lib_kinds
instead of local definition
---
.../testsuite/libgomp.fortran}/allocate-static.f90 | 28 --
1 file changed, 28 deletions(-)
diff --git a/gcc/testsuite/gfortran.dg/gomp/allocate-static.f90 b/libgomp/testsuite/libgomp.fortran/allocate-static.f90
similarity index 50%
rename from gcc/testsuite/gfortran.dg/gomp/allocate-static.f90
rename to libgomp/testsuite/libgomp.fortran/allocate-static.f90
index e43dae5793f..2789e39e19b 100644
--- a/gcc/testsuite/gfortran.dg/gomp/allocate-static.f90
+++ b/libgomp/testsuite/libgomp.fortran/allocate-static.f90
@@ -1,31 +1,3 @@
-! { dg-do run }
-
-module omp_lib_kinds
- use iso_c_binding, only: c_int, c_intptr_t
- implicit none
- private :: c_int, c_intptr_t
- integer, parameter :: omp_allocator_handle_kind = c_intptr_t
-
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_null_allocator = 0
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_default_mem_alloc = 1
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_large_cap_mem_alloc = 2
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_const_mem_alloc = 3
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_high_bw_mem_alloc = 4
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_low_lat_mem_alloc = 5
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_cgroup_mem_alloc = 6
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_pteam_mem_alloc = 7
- integer (kind=omp_allocator_handle_kind), &
- parameter :: omp_thread_mem_alloc = 8
-end module
-
module m
use iso_c_binding, only: c_intptr_t
use omp_lib_kinds, only: omp_default_mem_alloc
