Re: [Patch, fortran] PR114859 - [14/15 Regression] Seeing new segmentation fault in same_type_as since r14-9752
On Sun, Apr 28, 2024 at 10:37:06PM +0100, Paul Richard Thomas wrote: > Could this be looked at quickly? The timing of this regression is more than > a little embarrassing on the eve of the 14.1 release. The testcase and the > comment in gfc_trans_class_init_assign explain what this problem is all > about and how the patch fixes it. > > OK for 15-branch and backporting to 14-branch (hopefully to the RC as well)? The patch is ok for 14.1 if cherry-picked today. Jakub
Re: [Patch] OpenMP/Fortran: Fix defaultmap(none) issue with dummy procedures [PR114283]
On Mon, Mar 11, 2024 at 11:07:46AM +0100, Tobias Burnus wrote: > Using dummy procedures in a target region with 'defaultmap(none)' leads to: > > Error: 'g' not specified in enclosing 'target' > > and this cannot be fixed by using 'firstprivate' as non-pointer dummy routines > are rejected as "Error: Object 'g' is not a variable". > > Fixed by doing the same for mapping as for data sharing: using predetermined > firstprivate. > > BTW: Only since GCC 14, 'declare target indirect' makes it possible to > simply use dummy procedures and procedures pointers in a target region. So firstprivate clause handling remaps them then if declare target indirect is used? If so, the patch looks reasonable to me. Jakub
Re: [Patch] Fortran/Openmp: Use OPT_Wopenmp for gfc_match_omp_depobj warning
On Fri, Feb 23, 2024 at 12:25:54PM +0100, Tobias Burnus wrote: > When checking something else, I noticed that there was one warning in > openmp.cc that did not use OPT_Wopenmp. > > I intent to commit the attached patch later today as obvious. > > Tobias > Fortran/Openmp: Use OPT_Wopenmp for gfc_match_omp_depobj warning > > gcc/fortran/ChangeLog: > > * openmp.cc (gfc_match_omp_depobj): Use OPT_Wopenmp > as warning category in gfc_warning. LGTM. Jakub
Re: [PATCH] fortran: gfc_trans_subcomponent_assign fixes [PR113503]
On Sat, Feb 17, 2024 at 04:05:16PM +0100, Harald Anlauf wrote:
> > +program pr113503
> > + implicit none
> > + type :: T
> > +character(len=:), allocatable :: u
> > + end type
> > + character(len=20) :: us(1) = 'foobar'
> > + type(T) :: x
> > + x = T(u = trim (us(1))) ! { dg-bogus "is used uninitialized" }
> tab here not allowed in Fortran
>
> My newsreader shows a tab here, giving a warning when running the test.
> Also, applying your patch on top of r14-9045 I do not see the
> uninitialized warning, which could have been fixed by r14-8947.
> Please recheck and adjust accordingly.
I certainly see if I apply just the testsuite part of the patch to current
trunk
make check-gfortran RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}
dg.exp=pr113503*.f90'
...
FAIL: gfortran.dg/pr113503_1.f90 -O (test for bogus messages, line 12)
FAIL: gfortran.dg/pr113503_2.f90 -O (internal compiler error: in
gimplify_var_or_parm_decl, at gimplify.cc:3280)
FAIL: gfortran.dg/pr113503_2.f90 -O (test for excess errors)
=== gfortran Summary for unix/-m32 ===
# of expected passes1
# of unexpected failures3
...
FAIL: gfortran.dg/pr113503_1.f90 -O (test for bogus messages, line 12)
FAIL: gfortran.dg/pr113503_2.f90 -O (internal compiler error: in
gimplify_var_or_parm_decl, at gimplify.cc:3280)
FAIL: gfortran.dg/pr113503_2.f90 -O (test for excess errors)
=== gfortran Summary for unix/-m64 ===
# of expected passes1
# of unexpected failures3
and that is all gone after I apply the trans-expr.cc patch as well and
make before the above command.
I have replaced the tab with a space, but from what I can see, there is no
warning/error with the options it is compiled, warning with -Wtabs or -Wall
and error with -pedantic-errors, but those options aren't used.
Thanks.
Jakub
[PATCH] fortran: gfc_trans_subcomponent_assign fixes [PR113503]
1, this time after slen.1 got out of scope.
I really don't understand why the code modifies
expr2->ts.u.cl->backend_decl, expr2 isn't used there anywhere except for
expr2->ts.u.cl->backend_decl expressions, so hacks like save the previous
value, overwrite it temporarily over some call that will use expr2 and
restore afterwards aren't needed - there are no such calls, so the
following patch fixes it just by not messing up with
expr2->ts.u.cl->backend_decl, only set it to size variable and overwrite
that with a temporary if needed.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2024-02-17 Jakub Jelinek
PR fortran/113503
* trans-expr.cc (alloc_scalar_allocatable_subcomponent): Don't
overwrite expr2->ts.u.cl->backend_decl, instead set size to
expr2->ts.u.cl->backend_decl first and use size instead of
expr2->ts.u.cl->backend_decl.
(gfc_trans_subcomponent_assign): Emit se.pre into block
before calling alloc_scalar_allocatable_subcomponent instead of
after it.
* gfortran.dg/pr113503_1.f90: New test.
* gfortran.dg/pr113503_2.f90: New test.
--- gcc/fortran/trans-expr.cc.jj2024-02-14 14:26:19.764810614 +0100
+++ gcc/fortran/trans-expr.cc 2024-02-16 13:58:22.544770967 +0100
@@ -9059,13 +9059,10 @@ alloc_scalar_allocatable_subcomponent (s
if (cm->ts.type == BT_CHARACTER && cm->ts.deferred)
{
gcc_assert (expr2->ts.type == BT_CHARACTER);
- if (!expr2->ts.u.cl->backend_decl
- || !VAR_P (expr2->ts.u.cl->backend_decl))
- expr2->ts.u.cl->backend_decl = gfc_create_var (TREE_TYPE (slen),
- "slen");
- gfc_add_modify (block, expr2->ts.u.cl->backend_decl, slen);
-
size = expr2->ts.u.cl->backend_decl;
+ if (!size || !VAR_P (size))
+ size = gfc_create_var (TREE_TYPE (slen), "slen");
+ gfc_add_modify (block, size, slen);
gfc_deferred_strlen (cm, );
lhs_cl_size = fold_build3_loc (input_location, COMPONENT_REF,
@@ -9253,19 +9250,20 @@ gfc_trans_subcomponent_assign (tree dest
|| (cm->ts.type == BT_CLASS && CLASS_DATA (cm)->attr.allocatable
&& expr->ts.type != BT_CLASS)))
{
+ tree size;
+
gfc_init_se (, NULL);
gfc_conv_expr (, expr);
- tree size;
+ /* The remainder of these instructions follow the if (cm->attr.pointer)
+if (!cm->attr.dimension) part above. */
+ gfc_add_block_to_block (, );
/* Take care about non-array allocatable components here. The alloc_*
routine below is motivated by the alloc_scalar_allocatable_for_
assignment() routine, but with the realloc portions removed and
different input. */
alloc_scalar_allocatable_subcomponent (, dest, cm, expr,
se.string_length);
- /* The remainder of these instructions follow the if (cm->attr.pointer)
-if (!cm->attr.dimension) part above. */
- gfc_add_block_to_block (, );
if (expr->symtree && expr->symtree->n.sym->attr.proc_pointer
&& expr->symtree->n.sym->attr.dummy)
--- gcc/testsuite/gfortran.dg/pr113503_1.f90.jj 2024-02-16 14:16:17.937153094
+0100
+++ gcc/testsuite/gfortran.dg/pr113503_1.f902024-02-16 14:16:10.124258815
+0100
@@ -0,0 +1,18 @@
+! PR fortran/113503
+! { dg-do compile }
+! { dg-options "-O2 -fno-inline -Wuninitialized" }
+
+program pr113503
+ implicit none
+ type :: T
+character(len=:), allocatable :: u
+ end type
+ character(len=20) :: us(1) = 'foobar'
+ type(T) :: x
+ x = T(u = trim (us(1))) ! { dg-bogus "is used uninitialized" }
+ call foo
+contains
+ subroutine foo
+if (x%u /= 'foobar') stop 1
+ end subroutine
+end
--- gcc/testsuite/gfortran.dg/pr113503_2.f90.jj 2024-02-16 14:17:01.197567699
+0100
+++ gcc/testsuite/gfortran.dg/pr113503_2.f902024-02-16 14:16:51.215702778
+0100
@@ -0,0 +1,12 @@
+! PR fortran/113503
+! { dg-do compile }
+
+program pr113503
+ implicit none
+ type :: T
+character(len=:), allocatable :: u
+ end type
+ character(len=20) :: us(1) = 'foo'
+ type(T) :: x
+ x = T(u = us(1))
+end
Jakub
Re: [PATCH v2] libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD
On Thu, Jan 04, 2024 at 08:43:26PM -0500, Lipeng Zhu wrote: > This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is > not defined in dec_waiting_unlocked function. As io.h does > not include async.h, the WRLOCK and RWUNLOCK macros are > undefined. > > libgfortran/ChangeLog: > > * io/io.h (dec_waiting_unlocked): Use > __gthread_rwlock_wrlock/__gthread_rwlock_unlock or > __gthread_mutex_lock/__gthread_mutex_unlock functions > to replace WRLOCK and RWUNLOCK macros. > > Signed-off-by: Lipeng Zhu LGTM. Jakub
Re: [PATCH 1/8] OpenMP: lvalue parsing for map/to/from clauses (C++)
On Fri, Jan 05, 2024 at 12:23:26PM +, Julian Brown wrote: > * g++.dg/gomp/bad-array-section-10.C: New test. This test FAILs in C++23/C++26 modes, just try make check-g++ GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 RUNTESTFLAGS=gomp.exp=bad-array-section-10.C While in C++20 comma in array references was deprecated, in C++23 we implement multidimensional arrays, so the diagnostics there is different. See https://wg21.link/p2036r3 Jakub
Re: [PATCH v7] libgfortran: Replace mutex with rwlock
On Thu, Dec 14, 2023 at 01:29:01PM +0100, Thomas Schwinge wrote: > > Sure, I will look into that. > > > > BTW, I didn’t have the PowerPC in hands, do you mind granting the access of > > your > > test environment to me to help reproduce the issue? > > That's unfortunately not possible: it's behind company VPN, restricted > access. :-/ I'll later try to have at least a quick look where it's > hanging, or what it's doing. There is always https://gcc.gnu.org/wiki/CompileFarm There are e.g. 192, 160, 128, 80, 64 thread power machines. Whether any of them can actually reproduce it, I haven't tried. But shouldn't be that time consuming to reproduce, for this kind of thing one can just build .../configure --enable-languages=c,c++,fortran --disable-bootstrap --disable-libsanitizer make -jN compiler and just cd power*/libgomp make check RUNTESTFLAGS=fortran.exp=rwlock*.f90 repeatedly to see if it gets stuck. Jakub
[committed] testsuite: Fix up target-enter-data-1.c on 32-bit targets
On Wed, Nov 29, 2023 at 11:43:05AM +, Julian Brown wrote:
> * c-c++-common/gomp/target-enter-data-1.c: Adjust scan output.
struct bar { int num_vectors; double *vectors; };
is 16 bytes only on 64-bit targets, on 32-bit ones it is just 8 bytes,
so the explicit matching of the * 16 multiplication only works on the
former.
Tested on x86_64-linux -m32/-m64, committed to trunk.
2023-12-14 Jakub Jelinek
* c-c++-common/gomp/target-enter-data-1.c: Match also sizeof bar on
32-bit targets - 8 bytes - rather than just 16 bytes.
--- gcc/testsuite/c-c++-common/gomp/target-enter-data-1.c.jj2023-12-14
07:49:53.364577887 +0100
+++ gcc/testsuite/c-c++-common/gomp/target-enter-data-1.c 2023-12-14
08:08:59.680964433 +0100
@@ -22,4 +22,4 @@ void func (struct foo *f, int n, int m)
}
/* { dg-final { scan-tree-dump-times {map\(struct:\*f \[len: 1\]\)
map\(alloc:[a-z0-9\._]+->vectors \[len: 0\]\) map\(to:\*_[0-9]+ \[len:
_[0-9]+\]\) map\(attach:[a-z0-9\._]+->vectors \[bias: [^\]]+\]\)
map\(attach:\*_[0-9]+ \[bias: _[0-9]+\]\)} 1 "gimple" } } */
-/* { dg-final { scan-tree-dump-times {map\(struct:\*\(f->bars \+ \(sizetype\)
\(\([^\)]+\) n \* 16\)\) \[len: 1\]\) map\(alloc:[a-z0-9\._]+->vectors \[len:
0\]\) map\(to:\*_[0-9]+ \[len: _[0-9]+\]\) map\(attach:[a-z0-9\._]+->vectors
\[bias: [^\]]+\]\)} 2 "gimple" } } */
+/* { dg-final { scan-tree-dump-times {map\(struct:\*\(f->bars \+ \(sizetype\)
\(\([^\)]+\) n \* (?:16|8)\)\) \[len: 1\]\) map\(alloc:[a-z0-9\._]+->vectors
\[len: 0\]\) map\(to:\*_[0-9]+ \[len: _[0-9]+\]\)
map\(attach:[a-z0-9\._]+->vectors \[bias: [^\]]+\]\)} 2 "gimple" } } */
Jakub
Re: [PATCH v7] libgfortran: Replace mutex with rwlock
On Sat, Dec 09, 2023 at 10:39:45AM -0500, Lipeng Zhu wrote: > This patch try to introduce the rwlock and split the read/write to > unit_root tree and unit_cache with rwlock instead of the mutex to > increase CPU efficiency. In the get_gfc_unit function, the percentage > to step into the insert_unit function is around 30%, in most instances, > we can get the unit in the phase of reading the unit_cache or unit_root > tree. So split the read/write phase by rwlock would be an approach to > make it more parallel. > > BTW, the IPC metrics can gain around 9x in our test > server with 220 cores. The benchmark we used is > https://github.com/rwesson/NEAT > > libgcc/ChangeLog: > > * gthr-posix.h (__GTHREAD_RWLOCK_INIT): New macro. > (__gthrw): New function. > (__gthread_rwlock_rdlock): New function. > (__gthread_rwlock_tryrdlock): New function. > (__gthread_rwlock_wrlock): New function. > (__gthread_rwlock_trywrlock): New function. > (__gthread_rwlock_unlock): New function. > > libgfortran/ChangeLog: > > * io/async.c (DEBUG_LINE): New macro. > * io/async.h (RWLOCK_DEBUG_ADD): New macro. > (CHECK_RDLOCK): New macro. > (CHECK_WRLOCK): New macro. > (TAIL_RWLOCK_DEBUG_QUEUE): New macro. > (IN_RWLOCK_DEBUG_QUEUE): New macro. > (RDLOCK): New macro. > (WRLOCK): New macro. > (RWUNLOCK): New macro. > (RD_TO_WRLOCK): New macro. > (INTERN_RDLOCK): New macro. > (INTERN_WRLOCK): New macro. > (INTERN_RWUNLOCK): New macro. > * io/io.h (struct gfc_unit): Change UNIT_LOCK to UNIT_RWLOCK in > a comment. > (unit_lock): Remove including associated internal_proto. > (unit_rwlock): New declarations including associated internal_proto. > (dec_waiting_unlocked): Use WRLOCK and RWUNLOCK on unit_rwlock > instead of __gthread_mutex_lock and __gthread_mutex_unlock on > unit_lock. > * io/transfer.c (st_read_done_worker): Use WRLOCK and RWUNLOCK on > unit_rwlock instead of LOCK and UNLOCK on unit_lock. > (st_write_done_worker): Likewise. > * io/unit.c: Change UNIT_LOCK to UNIT_RWLOCK in 'IO locking rules' > comment. Use unit_rwlock variable instead of unit_lock variable. > (get_gfc_unit_from_unit_root): New function. > (get_gfc_unit): Use RDLOCK, WRLOCK and RWUNLOCK on unit_rwlock > instead of LOCK and UNLOCK on unit_lock. > (close_unit_1): Use WRLOCK and RWUNLOCK on unit_rwlock instead of > LOCK and UNLOCK on unit_lock. > (close_units): Likewise. > (newunit_alloc): Use RWUNLOCK on unit_rwlock instead of UNLOCK on > unit_lock. > * io/unix.c (find_file): Use RDLOCK and RWUNLOCK on unit_rwlock > instead of LOCK and UNLOCK on unit_lock. > (flush_all_units): Use WRLOCK and RWUNLOCK on unit_rwlock instead > of LOCK and UNLOCK on unit_lock. Ok for trunk, thanks. Jakub
Re: [patch] OpenMP/Fortran: Implement omp allocators/allocate for ptr/allocatables
On Sat, Dec 09, 2023 at 12:19:10PM +0100, Thomas Schwinge wrote:
> > --- a/gcc/omp-builtins.def
> > +++ b/gcc/omp-builtins.def
> > @@ -467,6 +467,9 @@ DEF_GOMP_BUILTIN
> > (BUILT_IN_GOMP_WORKSHARE_TASK_REDUCTION_UNREGISTER,
> > DEF_GOMP_BUILTIN (BUILT_IN_GOMP_ALLOC,
> > "GOMP_alloc", BT_FN_PTR_SIZE_SIZE_PTRMODE,
> > ATTR_ALLOC_WARN_UNUSED_RESULT_SIZE_2_NOTHROW_LIST)
> > +DEF_GOMP_BUILTIN (BUILT_IN_GOMP_REALLOC,
> > + "omp_realloc", BT_FN_PTR_PTR_SIZE_PTRMODE_PTRMODE,
> > + ATTR_ALLOC_WARN_UNUSED_RESULT_SIZE_2_NOTHROW_LEAF_LIST)
> > DEF_GOMP_BUILTIN (BUILT_IN_GOMP_FREE,
> > "GOMP_free", BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST)
> > DEF_GOMP_BUILTIN (BUILT_IN_GOMP_WARNING, "GOMP_warning",
>
> Should this either be 'BUILT_IN_OMP_REALLOC' ('OMP' instead of 'GOMP'),
> or otherwise a 'GOMP_realloc' be added to 'libgomp/allocator.c', like for
> 'GOMP_alloc', 'GOMP_free'; 'ialias_call'ing the respective 'omp_[...]'
> functions? (Verbatim 'omp_realloc' also mentioned in the comment before
> 'gcc/fortran/trans-openmp.cc:gfc_omp_call_is_alloc'.)
There were 3 reasons to add GOMP_alloc (and 1 for GOMP_free):
1) when it was added, omp_aligned_alloc was still not exported from the
library because we thought we shouldn't expose 5.1 features until we
have 5.0 implemented (then changed mind)
2) unline omp_aligned_alloc, GOMP_alloc issues fatal error on allocation
failure
3) the omp_* functions have omp_allocator_handle_t arguments, which is hard
to provide for builtins (I think this is the only reason for GOMP_free
addition, maybe together with wanting those to be paired)
Now, 1) is a non-issue anymore, I don't know what Fortran wants for
allocation failures, if it is better to have diagnostics on the libgomp side
or if wants to emit it inline. And yes, 3) would be an argument to add
GOMP_realloc.
Jakub
Re: [Patch] OpenMP: Support acquires/release in 'omp require atomic_default_mem_order'
On Tue, Nov 28, 2023 at 12:28:05PM +0100, Tobias Burnus wrote:
> I stumbled over this omission when looking at Sandra's patch. It turned out
> that this is
> a new OpenMP 5.2 feature - probably added to simplify/unify the syntax. I
> guess the reason
> that release/acquire wasn't added before is that it cannot be universally be
> used - read/write
> do only accept one of them.
I thought when this was discussed that it was meant to behave right (choose
some more appropriate memory model if one was not allowed), but reading 5.2
I think that is not what ended up in the spec, because [213:11-13] says that
atomic_default_mem_order is as if the argument appeared on any atomic
directive without explicit mem-order clause and atomic directive has the
[314:9-10] restrictions.
I'd bring this to omp-lang whether it was really meant that
#pragma omp requires atomic_default_mem_order (release)
int foo (int *p) {
int t;
#pragma omp atomic read
t = *p;
return t;
}
and
#pragma omp requires atomic_default_mem_order (acquire)
void bar (int *p) {
#pragma omp atomic write
*p = 0;
}
are meant to be invalid.
Another comment, atomic_default_mem_order arguments aren't handled
just in the requires parsing, but also in context selectors.
So, the additions would need to be reflected in
omp_check_context_selector and omp_context_selector_matches
as well.
Jakub
Re: [patch] OpenMP: Add uses_allocators support
On Mon, Nov 20, 2023 at 11:42:02AM +0100, Tobias Burnus wrote:
> 2023-11-19 Tobias Burnus
> Chung-Lin Tang
>
> gcc/ChangeLog:
>
> * builtin-types.def (BT_FN_VOID_PTRMODE):
> (BT_FN_PTRMODE_PTRMODE_INT_PTR): Add.
> * gimplify.cc (gimplify_bind_expr): Diagnose missing
> uses_allocators clause.
> (gimplify_scan_omp_clauses, gimplify_adjust_omp_clauses,
> gimplify_omp_workshare): Handle uses_allocators.
> * omp-builtins.def (BUILT_IN_OMP_INIT_ALLOCATOR,
> BUILT_IN_OMP_DESTROY_ALLOCATOR): Add.
> * omp-low.cc (scan_sharing_clauses):
Missing description.
> +static tree
> +c_parser_omp_clause_uses_allocators (c_parser *parser, tree list)
> +{
> + location_t clause_loc = c_parser_peek_token (parser)->location;
> + tree t = NULL_TREE, nl = list;
> + matching_parens parens;
> + if (!parens.require_open (parser))
> +return list;
> +
> + tree memspace_expr = NULL_TREE;
> + tree traits_var = NULL_TREE;
> +
> + struct item_tok
> + {
> +location_t loc;
> +tree id;
> +item_tok (void) : loc (UNKNOWN_LOCATION), id (NULL_TREE) {}
> + };
> + struct item { item_tok name, arg; };
> + auto_vec *modifiers = NULL, *allocators = NULL;
> + auto_vec *cur_list = new auto_vec (4);
This is certainly the first time I've seen pointers to auto_vec,
normally one uses just vec in such cases, auto_vec is used typically
on automatic variables to make sure the destruction is done.
But I think all the first parse it as a token soup without checking
anything and only in the second round actually check it is something
we've never done before in exactly the same situations.
The usual way would be to quickly peek at tokens to see if there is
: ahead and decide based on that.
See e.g. c_parser_omp_clause_allocate clause.
That has_modifiers check could be basically copied over with
the names of modifiers changed, or could be done in a loop, or
could be moved into a helper function which could be used
by c_parser_omp_clause_allocate and this function and perhaps
others, pass it the list of modifiers and have it return whether
there are modifiers or not.
c_parser_omp_clause_linear does this too (though, that has one of
the modifiers without arguments).
I'm afraid if parsing of every clause does the parsing so significantly
differently it will be a maintainance nightmare.
> @@ -23648,7 +23861,8 @@ c_parser_omp_target_exit_data (location_t loc,
> c_parser *parser,
> | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IN_REDUCTION) \
> | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_THREAD_LIMIT) \
> | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR)\
> - | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR))
> + | (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_HAS_DEVICE_ADDR)\
Space before \ please (and also in the line above.
> + if (strcmp (IDENTIFIER_POINTER (DECL_NAME (t)),
> + "omp_null_allocator") == 0)
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "% cannot be used in "
> + "% clause");
> + break;
> + }
> +
> + if (OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c)
> + || OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "modifiers cannot be used with pre-defined "
> + "allocators");
> + break;
> + }
> + }
> + t = OMP_CLAUSE_USES_ALLOCATORS_MEMSPACE (c);
> + if (t != NULL_TREE
> + && (TREE_CODE (t) != CONST_DECL
> + || TREE_CODE (TREE_TYPE (t)) != ENUMERAL_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (TREE_TYPE
> (t))),
> + "omp_memspace_handle_t") != 0))
> + {
> + error_at (OMP_CLAUSE_LOCATION (c), "memspace modifier must be "
Maybe % ?
> + "constant enum of % type");
> + remove = true;
> + break;
> + }
> + t = OMP_CLAUSE_USES_ALLOCATORS_TRAITS (c);
> + if (t != NULL_TREE)
> + {
> + bool type_err = false;
> +
> + if (TREE_CODE (TREE_TYPE (t)) != ARRAY_TYPE
> + || DECL_SIZE (t) == NULL_TREE)
> + type_err = true;
> + else
> + {
> + tree elem_t = TREE_TYPE (TREE_TYPE (t));
> + if (TREE_CODE (elem_t) != RECORD_TYPE
> + || strcmp (IDENTIFIER_POINTER (TYPE_IDENTIFIER (elem_t)),
> + "omp_alloctrait_t") != 0
> + || !TYPE_READONLY (elem_t))
> + type_err = true;
> + }
> + if (type_err)
> + {
> + if (TREE_CODE (t) != ERROR_MARK)
t != error_mark_node
> + error_at (OMP_CLAUSE_LOCATION (c), "traits array %qE must "
> +
Re: [patch] OpenMP/Fortran: Implement omp allocators/allocate for ptr/allocatables
On Wed, Nov 08, 2023 at 05:58:10PM +0100, Tobias Burnus wrote: > --- a/gcc/builtins.cc > +++ b/gcc/builtins.cc > @@ -11739,6 +11739,7 @@ builtin_fnspec (tree callee) > return ".cO "; >/* Realloc serves both as allocation point and deallocation point. */ >case BUILT_IN_REALLOC: > + case BUILT_IN_GOMP_REALLOC: > return ".Cw "; I must say I've never been sure if one needs to specify those ". " for integral arguments for which nothing is known; if they would need to be, then also all the BUILT_IN_GOMP_* other cases would be wrong, but not just those, also BUILT_IN_*_CHK (which have extra size_t argument) or here BUILT_IN_REALLOC itself. So, let's hope it is ok as is. Otherwise, the middle-end changes look just fine to me, and for Fortran FE I'm afraid you know it much more than I do. Jakub
Re: [PATCH v6] libgfortran: Replace mutex with rwlock
On Fri, Aug 18, 2023 at 11:18:19AM +0800, Zhu, Lipeng wrote:
> From: Lipeng Zhu
>
> This patch try to introduce the rwlock and split the read/write to
> unit_root tree and unit_cache with rwlock instead of the mutex to
> increase CPU efficiency. In the get_gfc_unit function, the percentage
> to step into the insert_unit function is around 30%, in most instances,
> we can get the unit in the phase of reading the unit_cache or unit_root
> tree. So split the read/write phase by rwlock would be an approach to
> make it more parallel.
>
> BTW, the IPC metrics can gain around 9x in our test
> server with 220 cores. The benchmark we used is
> https://github.com/rwesson/NEAT
>
> libgcc/ChangeLog:
>
> * gthr-posix.h (__GTHREAD_RWLOCK_INIT): New macro
> (__gthrw): New function
> (__gthread_rwlock_rdlock): New function
> (__gthread_rwlock_tryrdlock): New function
> (__gthread_rwlock_wrlock): New function
> (__gthread_rwlock_trywrlock): New function
> (__gthread_rwlock_unlock): New function
>
> libgfortran/ChangeLog:
>
> * io/async.c (DEBUG_LINE): New
> * io/async.h (RWLOCK_DEBUG_ADD): New macro
> (CHECK_RDLOCK): New macro
> (CHECK_WRLOCK): New macro
> (TAIL_RWLOCK_DEBUG_QUEUE): New macro
> (IN_RWLOCK_DEBUG_QUEUE): New macro
> (RDLOCK): New macro
> (WRLOCK): New macro
> (RWUNLOCK): New macro
> (RD_TO_WRLOCK): New macro
> (INTERN_RDLOCK): New macro
> (INTERN_WRLOCK): New macro
> (INTERN_RWUNLOCK): New macro
> * io/io.h (internal_proto): Define unit_rwlock
> * io/transfer.c (st_read_done_worker): Relace unit_lock with unit_rwlock
> (st_write_done_worker): Relace unit_lock with unit_rwlock
> * io/unit.c (get_gfc_unit): Relace unit_lock with unit_rwlock
> (if): Relace unit_lock with unit_rwlock
> (close_unit_1): Relace unit_lock with unit_rwlock
> (close_units): Relace unit_lock with unit_rwlock
> (newunit_alloc): Relace unit_lock with unit_rwlock
> * io/unix.c (flush_all_units): Relace unit_lock with unit_rwlock
The changeLog entries are all incorrect.
1) they should be indented by a tab, not 4 spaces, and should end with
a dot
2) when several consecutive descriptions have the same text, especially
when it is long, it should use Likewise. for the 2nd and following
3) (internal_proto) is certainly not what you've changed, from what I can
see in io.h you've done:
* io/io.h (struct gfc_unit): Change UNIT_LOCK to UNIT_RWLOCK in
a comment.
(unit_lock): Remove including associated internal_proto.
(unit_rwlock): New declarations including associated internal_proto.
(dec_waiting_unlocked): Use WRLOCK and RWUNLOCK on unit_rwlock
instead of __gthread_mutex_lock and __gthread_mutex_unlock on
unit_lock.
(if) is certainly not what you've changed either, always find what
function or macro the change was in, or if you remove something, state
it, if you add something, state it.
4) all the
Replace unit_lock with unit_rwlock. descriptions only partially match
reality, you've also changed the operations on those variables.
> --- a/libgfortran/io/async.h
> +++ b/libgfortran/io/async.h
> @@ -207,9 +207,132 @@
> INTERN_LOCK (_queue_lock); \
> MUTEX_DEBUG_ADD (mutex); \
> INTERN_UNLOCK (_queue_lock);
> \
> -DEBUG_PRINTF ("%s" DEBUG_RED "ACQ:" DEBUG_NORM " %-30s %78p\n",
> aio_prefix, #mutex, mutex); \
> +DEBUG_PRINTF ("%s" DEBUG_RED "ACQ:" DEBUG_NORM " %-30s %78p\n",
> aio_prefix, #mutex, \
> + mutex); \
Why are you changing this at all?
> +#define RD_TO_WRLOCK(rwlock) \
> + RWUNLOCK (rwlock);\
At least a space before \ (or better tab
> +#define RD_TO_WRLOCK(rwlock) \
> + RWUNLOCK (rwlock);\
Likewise.
> + WRLOCK (rwlock);
> +#endif
> +#endif
> +
> +#ifndef __GTHREAD_RWLOCK_INIT
> +#define RDLOCK(rwlock) LOCK (rwlock)
> +#define WRLOCK(rwlock) LOCK (rwlock)
> +#define RWUNLOCK(rwlock) UNLOCK (rwlock)
> +#define RD_TO_WRLOCK(rwlock) {}
do {} while (0)
instead of {}
?
> #endif
>
> #define INTERN_LOCK(mutex) T_ERROR (__gthread_mutex_lock, mutex);
>
> #define INTERN_UNLOCK(mutex) T_ERROR (__gthread_mutex_unlock, mutex);
>
> +#define INTERN_RDLOCK(rwlock) T_ERROR (__gthread_rwlock_rdlock, rwlock);
> +#define INTERN_WRLOCK(rwlock) T_ERROR (__gthread_rwlock_wrlock, rwlock);
> +#define INTERN_RWUNLOCK(rwlock) T_ERROR (__gthread_rwlock_unlock, rwlock);
Admittedly preexisting issue, but I wonder why the ; at the end, all the
uses of RDLOCK etc. I've seen were followed by ;
> --- a/libgfortran/io/unit.c
> +++ b/libgfortran/io/unit.c
> @@ -33,34 +33,36 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
> If not, see
>
>
> /* IO locking rules:
> - UNIT_LOCK is a master lock, protecting UNIT_ROOT tree and UNIT_CACHE.
> + UNIT_RWLOCK is a master lock, protecting
Re: [PATCH] libgfortran: Fix -Wincompatible-pointer-types errors
On Tue, Dec 05, 2023 at 10:57:50AM +, Richard Earnshaw wrote: > On 05/12/2023 10:51, Jakub Jelinek wrote: > > On Tue, Dec 05, 2023 at 10:47:34AM +, Richard Earnshaw wrote: > > > > The following patch makes libgfortran build on i686-linux after hacking > > > > up > > > > --- kinds.h.xx 2023-12-05 00:23:00.133365064 +0100 > > > > +++ kinds.h 2023-12-05 11:19:24.409679808 +0100 > > > > @@ -10,8 +10,8 @@ typedef GFC_INTEGER_2 GFC_LOGICAL_2; > > > >#define HAVE_GFC_LOGICAL_2 > > > >#define HAVE_GFC_INTEGER_2 > > > > -typedef int32_t GFC_INTEGER_4; > > > > -typedef uint32_t GFC_UINTEGER_4; > > > > +typedef long GFC_INTEGER_4; > > > > +typedef unsigned long GFC_UINTEGER_4; > > > > > > That doesn't look right for a 64-bit processor. Presumably 4 means 4 > > > bytes, > > > > i686-linux is an ILP32 target, which I chose exactly because I regularly > > build > > it, had a tree with it around and because unlike 64-bit targets there are 2 > > standard 32-bit signed integer types. Though, normally int32_t there is > > int rather than long int and so the errors only appeared after this hack. > > > > My point is that on aarch64/x86_64 etc, this will make GFC_INTEGER_4 a > 64-bit type, whereas previously it was 32-bit. Sure. The above patch is a hack for a generated header. I'm not proposing that as a change, just explaining how I've verified the actual patch on i686-linux with such a hack. Jakub
Re: [PATCH] libgfortran: Fix -Wincompatible-pointer-types errors
On Tue, Dec 05, 2023 at 10:47:34AM +, Richard Earnshaw wrote: > > The following patch makes libgfortran build on i686-linux after hacking up > > --- kinds.h.xx 2023-12-05 00:23:00.133365064 +0100 > > +++ kinds.h 2023-12-05 11:19:24.409679808 +0100 > > @@ -10,8 +10,8 @@ typedef GFC_INTEGER_2 GFC_LOGICAL_2; > > #define HAVE_GFC_LOGICAL_2 > > #define HAVE_GFC_INTEGER_2 > > -typedef int32_t GFC_INTEGER_4; > > -typedef uint32_t GFC_UINTEGER_4; > > +typedef long GFC_INTEGER_4; > > +typedef unsigned long GFC_UINTEGER_4; > > That doesn't look right for a 64-bit processor. Presumably 4 means 4 bytes, i686-linux is an ILP32 target, which I chose exactly because I regularly build it, had a tree with it around and because unlike 64-bit targets there are 2 standard 32-bit signed integer types. Though, normally int32_t there is int rather than long int and so the errors only appeared after this hack. Jakub
[PATCH] libgfortran: Fix -Wincompatible-pointer-types errors
Hi!
On Tue, Dec 05, 2023 at 10:46:02AM +0100, Florian Weimer wrote:
> Presumably the fixes will look like this?
>
> diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
> index db3330060ce..4fcc77dbf83 100644
> --- a/libgfortran/io/list_read.c
> +++ b/libgfortran/io/list_read.c
> @@ -2987,13 +2987,13 @@ nml_read_obj (st_parameter_dt *dtp, namelist_info
> *nl, index_type offset,
> /* If this object has a User Defined procedure, call it. */
> if (nl->dtio_sub != NULL)
> {
> - int unit = dtp->u.p.current_unit->unit_number;
> + GFC_INTEGER_4 unit = dtp->u.p.current_unit->unit_number;
> char iotype[] = "NAMELIST";
> gfc_charlen_type iotype_len = 8;
> char tmp_iomsg[IOMSG_LEN] = "";
> char *child_iomsg;
> gfc_charlen_type child_iomsg_len;
> - int noiostat;
> + GFC_INTEGER_4 noiostat;
> int *child_iostat = NULL;
> gfc_full_array_i4 vlist;
> formatted_dtio dtio_ptr = (formatted_dtio)nl->dtio_sub;
That seems insufficient.
The following patch makes libgfortran build on i686-linux after hacking up
--- kinds.h.xx 2023-12-05 00:23:00.133365064 +0100
+++ kinds.h 2023-12-05 11:19:24.409679808 +0100
@@ -10,8 +10,8 @@ typedef GFC_INTEGER_2 GFC_LOGICAL_2;
#define HAVE_GFC_LOGICAL_2
#define HAVE_GFC_INTEGER_2
-typedef int32_t GFC_INTEGER_4;
-typedef uint32_t GFC_UINTEGER_4;
+typedef long GFC_INTEGER_4;
+typedef unsigned long GFC_UINTEGER_4;
typedef GFC_INTEGER_4 GFC_LOGICAL_4;
#define HAVE_GFC_LOGICAL_4
#define HAVE_GFC_INTEGER_4
in the build dir to emulate what newlib aarch64 is doing:
2023-12-05 Florian Weimer
Jakub Jelinek
* io/list_read.c (list_formatted_read_scalar) :
Change types of unit and noiostat to GFC_INTEGER_4 from int, change
type of child_iostat from to GFC_INTEGER_4 * from int *, formatting
fixes.
(nml_read_obj): Likewise.
* io/write.c (list_formatted_write_scalar) : Likewise.
(nml_write_obj): Likewise.
* io/transfer.c (unformatted_read, unformatted_write): Likewise.
--- libgfortran/io/list_read.c.jj 2023-05-09 00:07:26.161168737 +0200
+++ libgfortran/io/list_read.c 2023-12-05 11:25:31.837426653 +0100
@@ -2189,14 +2189,14 @@ list_formatted_read_scalar (st_parameter
break;
case BT_CLASS:
{
- int unit = dtp->u.p.current_unit->unit_number;
+ GFC_INTEGER_4 unit = dtp->u.p.current_unit->unit_number;
char iotype[] = "LISTDIRECTED";
gfc_charlen_type iotype_len = 12;
char tmp_iomsg[IOMSG_LEN] = "";
char *child_iomsg;
gfc_charlen_type child_iomsg_len;
- int noiostat;
- int *child_iostat = NULL;
+ GFC_INTEGER_4 noiostat;
+ GFC_INTEGER_4 *child_iostat = NULL;
gfc_full_array_i4 vlist;
GFC_DESCRIPTOR_DATA() = NULL;
@@ -2204,8 +2204,8 @@ list_formatted_read_scalar (st_parameter
/* Set iostat, intent(out). */
noiostat = 0;
- child_iostat = (dtp->common.flags & IOPARM_HAS_IOSTAT) ?
- dtp->common.iostat :
+ child_iostat = ((dtp->common.flags & IOPARM_HAS_IOSTAT)
+ ? dtp->common.iostat : );
/* Set iomsge, intent(inout). */
if (dtp->common.flags & IOPARM_HAS_IOMSG)
@@ -2987,14 +2987,14 @@ nml_read_obj (st_parameter_dt *dtp, name
/* If this object has a User Defined procedure, call it. */
if (nl->dtio_sub != NULL)
{
- int unit = dtp->u.p.current_unit->unit_number;
+ GFC_INTEGER_4 unit = dtp->u.p.current_unit->unit_number;
char iotype[] = "NAMELIST";
gfc_charlen_type iotype_len = 8;
char tmp_iomsg[IOMSG_LEN] = "";
char *child_iomsg;
gfc_charlen_type child_iomsg_len;
- int noiostat;
- int *child_iostat = NULL;
+ GFC_INTEGER_4 noiostat;
+ GFC_INTEGER_4 *child_iostat = NULL;
gfc_full_array_i4 vlist;
formatted_dtio dtio_ptr = (formatted_dtio)nl->dtio_sub;
@@ -3006,8 +3006,8 @@ nml_read_obj (st_parameter_dt *dtp, name
/* Set iostat, intent(out). */
noiostat = 0;
- child_iostat = (dtp->common.flags & IOPARM_HAS_IOSTAT) ?
- dtp->common.iostat :
+ child_iostat = ((dtp->common.flags & IOPARM_HAS_IOSTAT)
+ ? dtp->common.iostat : );
/* Set iomsg, intent(inout). */
if (d
Re: [patch] OpenMP: Add -Wopenmp and use it
On Mon, Nov 27, 2023 at 11:20:20AM +0100, Christophe Lyon wrote: > On Fri, 24 Nov 2023 at 15:08, Jakub Jelinek wrote: > > > Comments or remarks before I commit it? > > > > LGTM, thanks for working on it. > > > > Jakub > > > > I think the lack of final '.' in: > gcc/c-family/c.opt > + Warn about suspicious OpenMP code Tobias has fixed that a few commits later: r14-5835-g6eb1507107dee3e67e3a136e2917b93cdffba7c4 Sorry for missing that during patch review. Jakub
Re: [patch] OpenMP: Add -Wopenmp and use it
On Fri, Nov 24, 2023 at 02:51:28PM +0100, Tobias Burnus wrote: > Following the general trend to add a "[-W...]" to the warning messages > for both better grouping of the warnings and - more importantly - for > providing > a means to silence such a warning (or to -Werror= them explicitly), this patch > replaces several '0' by OPT_Wopenmp. > > Comments or remarks before I commit it? LGTM, thanks for working on it. Jakub
Re: [Patch] OpenMP: Accept argument to depobj's destroy clause
On Thu, Nov 23, 2023 at 04:59:16PM +0100, Tobias Burnus wrote:
> > There is also OEP_LEXICOGRAPHIC which could be used in addition to that.
> > The question is if we want to consider say
> > #pragma depobj (a[++i]) destroy (a[++i])
> > as same or different (similarly a[foo ()] in both cases).
>
> I don't think that we want to permit those; I think there is (a) the
> question whether both expressions have to be evaluated or not and (b),
> if so, in which order and (c), if the run-time result is different,
> whether both have to be 'destory'ed or only one of them (which one?).
Well, we don't need to destroy two, because it would be UB if the two
aren't the same. This is just about diagnostics if user messed stuff
up unintentionally.
The function call case can be the same very easily, just
int foo () { return 0; }
omp_depend_t a[2];
...
#pragma omp depobj (a[foo ()]) destroy (a[foo ()])
or
int i = 0;
#pragma omp depobj (a[((++i) * 2) & 1]) destroy (a[((++i) * 2) & 1])
The former may evaluate the function call multiple times, but user arranges
for it to do the same thing in each case, in the second case while there
are side-effects, they don't really matter for the value, just in whether
i after this pragma has value of 0, 1, 2 or something else (but if again
nothing cares about that value afterwards...).
The question is if same (I admit I haven't looked up the exact wording now)
means lexically same, or anything that has the same value, etc.
Because e.g.
omp_depend_t a;
...
omp_depend_t *p =
#pragma omp depobj (a) destroy (p[0])
is the same value but not lexically same.
IMHO the argument to destroy clause shouldn't have ever been allowed, it is
only unnecessary extra pain.
Jakub
Re: [Patch] OpenMP: Accept argument to depobj's destroy clause
On Thu, Nov 23, 2023 at 04:21:50PM +0100, Tobias Burnus wrote:
> @@ -21663,7 +21666,25 @@ c_parser_omp_depobj (c_parser *parser)
> clause = error_mark_node;
> }
>else if (!strcmp ("destroy", p))
> - kind = OMP_CLAUSE_DEPEND_LAST;
> + {
> + matching_parens c_parens;
> + kind = OMP_CLAUSE_DEPEND_LAST;
> + if (c_parser_next_token_is (parser, CPP_OPEN_PAREN)
> + && c_parens.require_open (parser))
> + {
> + tree destobj = c_parser_expr_no_commas (parser, NULL).value;
> + if (!lvalue_p (destobj))
> + error_at (EXPR_LOC_OR_LOC (destobj, c_loc),
> + "% expression is not lvalue expression");
> + else if (depobj != error_mark_node
> +&& !operand_equal_p (destobj, depobj,
> + OEP_MATCH_SIDE_EFFECTS))
There is also OEP_LEXICOGRAPHIC which could be used in addition to that.
The question is if we want to consider say
#pragma depobj (a[++i]) destroy (a[++i])
as same or different (similarly a[foo ()] in both cases).
A function could at least in theory return the same value, for other
side-effects there is some wiggle room in unspecified number of times how
many the side-effects of clauses are evaluated (and for destroy we really
don't intend to evaluate them at all for the clause, just for the directive
argument).
Jakub
Re: [Patch] OpenMP: Accept argument to depobj's destroy clause
On Thu, Nov 23, 2023 at 03:21:41PM +0100, Tobias Burnus wrote:
> I stumbled over this trivial omission which blocks some testcases.
>
> I am not sure whether I have solved the is-same-expr most elegantly,
> but I did loosely follow the duplicated-entry check for 'map'. As that's
> a restriction to the user, we don't have to catch all and I hope the code
> catches the most important violations, doesn't ICE and does not reject
> valid code. At least for all real-world code it should™ work, but I
> guess for lvalue expressions involving function calls it probably doesn't.
>
> Thoughts, comments?
>
> Tobias
>
> PS: GCC accepts an lvalue expression in C/C++ and only a identifier
> for a scalar variable in Fortran, i.e. neither array elements nor
> structure components.
>
> Which variant is right depends whether one reads OpenMP 5.1 (lvalue expr,
> scalar variable) or 5.2 (variable without permitting array sections or
> structure components) - whereas TR12 has the same but talks about
> locator list items in one restriction. For the OpenMP mess, see spec
> issue #3739.
> -
> 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
> OpenMP: Accept argument to depobj's destroy clause
>
> Since OpenMP 5.2, the destroy clause takes an depend argument as argument;
> for the depobj directive, it the new argument is optional but, if present,
> it must be identical to the directive's argument.
>
> gcc/c/ChangeLog:
>
> * c-parser.cc (c_parser_omp_depobj): Accept optionally an argument
> to the destroy clause.
>
> gcc/cp/ChangeLog:
>
> * parser.cc (cp_parser_omp_depobj): Accept optionally an argument
> to the destroy clause.
>
> gcc/fortran/ChangeLog:
>
> * openmp.cc (gfc_match_omp_depobj): Accept optionally an argument
> to the destroy clause.
>
> libgomp/ChangeLog:
>
> * libgomp.texi (5.2 Impl. Status): An argument to the destroy clause
> is now supported.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/gomp/depobj-3.c: New test.
> * gfortran.dg/gomp/depobj-3.f90: New test.
>
> gcc/c/c-parser.cc | 57 ++-
> gcc/cp/parser.cc| 60
> -
> gcc/fortran/openmp.cc | 15 +++-
> gcc/testsuite/c-c++-common/gomp/depobj-3.c | 40 +++
> gcc/testsuite/gfortran.dg/gomp/depobj-3.f90 | 18 +
> libgomp/libgomp.texi| 2 +-
> 6 files changed, 188 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> index 371dd29557b..378647c1a67 100644
> --- a/gcc/c/c-parser.cc
> +++ b/gcc/c/c-parser.cc
> @@ -21605,6 +21605,9 @@ c_parser_omp_critical (location_t loc, c_parser
> *parser, bool *if_p)
> destroy
> update (dependence-type)
>
> + OpenMP 5.2 additionally:
> + destroy ( depobj )
> +
> dependence-type:
> in
> out
> @@ -21663,7 +21666,59 @@ c_parser_omp_depobj (c_parser *parser)
> clause = error_mark_node;
> }
>else if (!strcmp ("destroy", p))
> - kind = OMP_CLAUSE_DEPEND_LAST;
> + {
> + matching_parens c_parens;
> + kind = OMP_CLAUSE_DEPEND_LAST;
> + if (c_parser_next_token_is (parser, CPP_OPEN_PAREN)
> + && c_parens.require_open (parser))
> + {
> + tree destobj = c_parser_expr_no_commas (parser, NULL).value;
> + /* OpenMP requires that the two expressions are identical; catch
> + the most common mismatches. */
> + if (!lvalue_p (destobj))
> + error_at (EXPR_LOC_OR_LOC (destobj, c_loc),
> + "% expression is not lvalue expression");
> + else if (depobj != error_mark_node)
> + {
> + tree t = depobj;
> + tree t2 = build_unary_op (EXPR_LOC_OR_LOC (destobj, c_loc),
> + ADDR_EXPR, destobj, false);
> + if (t2 != error_mark_node)
> + t2 = build_indirect_ref (EXPR_LOC_OR_LOC (t2, c_loc),
> + t2, RO_UNARY_STAR);
Please watch indentation, seems there is a mix of 8 spaces vs. tabs:
> + while (TREE_CODE (t) == COMPONENT_REF
> + || TREE_CODE (t) == ARRAY_REF)
> +{
> + t = TREE_OPERAND (t, 0);
> + if (TREE_CODE (t) == MEM_REF || INDIRECT_REF_P (t))
> + {
> + t = TREE_OPERAND (t, 0);
> + STRIP_NOPS (t);
> + if (TREE_CODE (t) == POINTER_PLUS_EXPR)
> + t = TREE_OPERAND (t, 0);
> +}
> + }
> +
Re: [Patch] OpenMP: Avoid ICE with LTO and 'omp allocate (was: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars)
On Wed, Oct 18, 2023 at 12:56:01PM +0200, Tobias Burnus wrote: > On 18.10.23 11:36, Jakub Jelinek wrote: > > On Wed, Oct 18, 2023 at 11:12:44AM +0200, Thomas Schwinge wrote: > > > +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (internal compiler > > > error: tree code 'statement_list' is not supported in LTO streams) > > Any references to GENERIC code in clauses etc. should have been gimplified > > or cleared during gimplification, we shouldn't support STATEMENT_LIST > > in LTO streaming. > > We only needed the statement list as aid during the gimplify.cc handling > of GOMP_alloc/GOMP_free for Fortran. How about just remove_attribute it > in that case? As discussed, as DECL_ATTRIBUTE gets added from the left > to the DECL_CHAIN, there shouldn't be a problem of introducing shared > trees; note that 'omp allocate' itself is added per DECL, i.e. it does > not introduce sharing itself, either. > > Tested with x86-64-gnu-linux. > > 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 > OpenMP: Avoid ICE with LTO and 'omp allocate' > > gcc/ChangeLog: > > * gimplify.cc (gimplify_bind_expr): Remove "omp allocate" attribute > to avoid that auxillary statement list reaches to LTO. > > gcc/testsuite/ChangeLog: > > * gfortran.dg/gomp/allocate-13a.f90: New test. LGTM. Jakub
Re: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars
On Wed, Oct 18, 2023 at 11:12:44AM +0200, Thomas Schwinge wrote: > Hi Tobias! > > On 2023-10-13T15:29:52+0200, Tobias Burnus wrote: > > => Updated patch attached > > When cherry-picking this commit 2d3dbf0eff668bed5f5f168b3cafd8590c54 > "Fortran: Support OpenMP's 'allocate' directive for stack vars" on top of > slightly older GCC sources (mentioning that just in case that's > relevant), in a configuration with offloading enabled (only), I see: > > +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (internal compiler error: > tree code 'statement_list' is not supported in LTO streams) > +FAIL: gfortran.dg/gomp/allocate-13.f90 -O (test for excess errors) Any references to GENERIC code in clauses etc. should have been gimplified or cleared during gimplification, we shouldn't support STATEMENT_LIST in LTO streaming. Jakub
Re: [Patch] Fortran: Support OpenMP's 'allocate' directive for stack vars
On Tue, Oct 10, 2023 at 06:46:35PM +0200, Tobias Burnus wrote:
> * parse.cc (check_omp_allocate_stmt): Permit procedure pointers
> here (rejected later) for less mislreading diagnostic.
s/misl/mis/
> libgomp/ChangeLog:
>
> * libgomp.texi:
Fill in something here.
> @@ -7220,8 +7227,7 @@ gfc_resolve_omp_allocate (gfc_namespace *ns,
> gfc_omp_namelist *list)
>>where);
> continue;
> }
> - if (ns != n->sym->ns || n->sym->attr.use_assoc
> - || n->sym->attr.host_assoc || n->sym->attr.imported)
> + if (ns != n->sym->ns || n->sym->attr.use_assoc ||
> n->sym->attr.imported)
s/ n/ n/
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -1400,23 +1400,53 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> "region must specify an % clause", t);
> /* Skip for omp_default_mem_alloc (= 1),
>unless align is present. */
> - else if (!errorcount
> -&& (align != NULL_TREE
> -|| alloc == NULL_TREE
> -|| !integer_onep (alloc)))
> + else if (errorcount
> +|| (align == NULL_TREE
> +&& alloc != NULL_TREE
> +&& integer_onep (alloc)))
> + DECL_ATTRIBUTES (t) = remove_attribute ("omp allocate",
> + DECL_ATTRIBUTES (t));
Probably already preexisting, by I wonder how safe remove_attribute is.
Aren't attributes shared in some cases
(like __attribute__((attr1, attr2, attr3)) int a, b, c, d;)?
Not really sure if something unshares them afterwards.
If they are shared, adding new attributes is fine, that will make the new
additions not shared and the tail shared, but remove_attribute could remove
it from all of them at once. Perhaps I'm wrong, haven't verified.
Otherwise LGTM (though, I didn't spot anything about allocatables in the
patch, am I just looking wrong or are they still unsupported)?
Jakub
Re: Test with an lto-build of libgfortran.
On Thu, Sep 28, 2023 at 09:03:39PM +0200, Toon Moene wrote:
> > > The full question of "lto-ing" run time libraries is more
> > > complicated than just "whether it works" as those who attended the
> > > BoF will recall.
> >
> > I didn't attend the Cauldron (but that discussion would have been
> > very interesting). I think for libgfortran, a first step would be
> > additional work to get declarations on both sides to agree (which is
> > worth doing anyway).
> >
> > Best regards
> >
> > Thomas
>
> The big problem in *distributing* GCC (i.e., the collection) with lto'd
> run-time libraries is that the format of the lto structure changes with
> releases. If a compiler (by accident) picks up a run time library with
> non-matching lto objects, it might crash (or "introduce subtle errors in a
> once working program").
It is worse than that, usually the LTO format changes e.g. any time any
option or parameter is added on a release branch (several times a year) and
at other times as well.
Though, admittedly GCC is the single package that actually could get away
with LTO in lib*.a libraries, at least in some packagings (if the static
libraries are in gcc specific subdirectories rather than say /usr/lib{,64}
or similar and if the packaging of gcc updates both the compiler and
corresponding static libraries in a lock-step. Because in that case LTO
in there will be always used only by the same snapshot from the release
branch and so should be compatible with the LTO in it.
Jakub
Re: Test with an lto-build of libgfortran.
On Thu, Sep 28, 2023 at 01:00:41PM +0200, Tobias Burnus wrote: > I am not aware of any logigal/integer/real(+comples)/character kind > 16, > except for this PPC one. And complex numbers are pairs of BT_REAL. > > Thus, I think that patch should be fine - except: > > > Does anything error earlier if it is larger? I mean, say user calling > > _gfortan_transfer_integer by hand with kind 1024? > > I think this will fail. We have various ways to deal with this in libgfortran; > I see some cases where the switch "default:" sets the length to 0; we have > other places where we use an "assert", I think we have other places were > we run into UB. > > Thus, one option would be to either 'assert(len <= 16)' or > 'assert((size_t)len < GFC_OTOA_BUF_SIZE - 1)' instead. > > Or we could handle it as len=0 and silently ignore the output or ... > > I am fine with either of the many options - except that I like something > explicit involving 'len' and a comparison (unreachable, assert, regarding as > len = 0) > better than the existing warning suppression which is too indirect for > me. (Besides: it does not work for LTO.) Preferences? Tobias Let's go with the __builtin_unreachable, ok for trunk. Jakub
Re: Test with an lto-build of libgfortran.
On Thu, Sep 28, 2023 at 09:29:02AM +0200, Tobias Burnus wrote: > the following works for me. I have only tried a normal build (where it > does silence the same warning) and not an LTO build and I just believed > the comment - see attached patch. Comments? > > On 28.09.23 08:25, Richard Biener via Fortran wrote: > > > This particular place in libgfortran has > > > >/* write_z, which calls xtoa_big, is called from transfer.c, > > formatted_transfer_scalar_write. There it is passed the kind as > > argument, which means a maximum of 16. The buffer is large > > enough, but the compiler does not know that, so shut up the > > warning here. */ > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored "-Wstringop-overflow" > >*q = '\0'; > > #pragma GCC diagnostic pop > > > > so obviously the #pragma doesn't survive through LTO. Somehow I think > > this is a known bug, but maybe I misremember (I think we are not streaming > > any of the ad-hoc location parts). > > I have replaced it now by the assert that "len <= 16", i.e. > > + if (len > 16) > +__builtin_unreachable (); > > Build + tested on x86-64-gnu-linux > Comment? OK for mainline? Is it just that in correct programs len can't be > 16, or that it is really impossible for it being > 16? I mean, we have that artificial kind 17 for powerpc which better should be turned into length of 16, but isn't e.g. _gfortran_transfer_integer etc. just called with a kind argument? Does anything error earlier if it is larger? I mean, say user calling _gfortan_transfer_integer by hand with kind 1024? Sure, we could still say it is UB to do that kind of thing and __builtin_unreachable () would be a way to turn that UB into manifestly reproducable UB. Jakub
Re: [Patch] OpenMP/Fortran: Reject not strictly nested target -> teams [PR110725, PR71065]
On Mon, Jul 24, 2023 at 09:43:10PM +0200, Tobias Burnus wrote:
> This patch adds diagnostic for additional code alongside a nested teams
> in a target region.
>
> The diagnostic is happening soon after parsing such that expressions
> in clauses are not yet expanded - those would end up before TEAMS
> and can be very complicated (e.g. assume an allocatable-returning function).
>
> (The patch diagnoses it in openmp.cc; after trans-openmp.cc it would
> already be to late.)
>
> Comments, remarks, suggestions?
Thanks for working on this. The fuzzy thing on the Fortran side is
if e.g. multiple nested BLOCK statements can appear sandwiched in between
target and teams (of course without declarations in them), or if e.g.
extra empty BLOCK; END BLOCK could appear next to it etc.
And on C/C++ side similarly with {}s, ; is an empty statement, so
#pragma omp target
{
;
#pragma omp teams
;
;
}
etc. would be invalid.
Jakub
[committed] fortran: Fix ICE on pr96024.f90 on big-endian hosts [PR96024]
Hi!
The pr96024.f90 testcase ICEs on big-endian hosts. The problem is
that length->val.integer is accessed after checking
length->expr_type == EXPR_CONSTANT, but it is a CHARACTER constant
which uses length->val.character union member instead and on big-endian
we end up reading constant 0x1 rather than some small number
on little-endian and if target doesn't have enough memory for 4 times
that (i.e. 16GB allocation), it ICEs.
Fixed thusly, bootstrapped/regtested on
{x86_64,i686,powerpc64le,aarch64,s390x}-linux, preapproved in bugzilla
by Harald, committed to trunk and 13, 12, 11 and 10 release branches.
2023-06-09 Jakub Jelinek
PR fortran/96024
* primary.cc (gfc_convert_to_structure_constructor): Only do
constant string ctor length verification and truncation/padding
if constant length has INTEGER type.
--- gcc/fortran/primary.cc.jj 2023-05-20 15:31:09.183661713 +0200
+++ gcc/fortran/primary.cc 2023-06-08 11:49:39.354875373 +0200
@@ -3188,10 +3188,11 @@ gfc_convert_to_structure_constructor (gf
goto cleanup;
/* For a constant string constructor, make sure the length is
-correct; truncate of fill with blanks if needed. */
+correct; truncate or fill with blanks if needed. */
if (this_comp->ts.type == BT_CHARACTER && !this_comp->attr.allocatable
&& this_comp->ts.u.cl && this_comp->ts.u.cl->length
&& this_comp->ts.u.cl->length->expr_type == EXPR_CONSTANT
+ && this_comp->ts.u.cl->length->ts.type == BT_INTEGER
&& actual->expr->ts.type == BT_CHARACTER
&& actual->expr->expr_type == EXPR_CONSTANT)
{
Jakub
Re: [PATCH 0/7] openmp: OpenMP 5.1 loop transformation directives
On Wed, May 17, 2023 at 01:55:00PM +0200, Frederik Harwath wrote: > Thanks for the explanation. But actually doing this would require a > complete rewrite which would almost certainly imply that mainline GCC > would not support the loop transformations for a long time. I don't think it needs complete rewrite, the change to use OMP_UNROLL/OMP_TILE should actually simplify stuff when you already have some other extra construct to handle the clauses if it isn't nested into something else, so I wouldn't expect it needs more than 2-3 hours of work. It is true that doing the transformation on trees rather than high gimple is something different, but again it doesn't require everything to be rewritten and we have code to do code copying both on trees and high and low gimple in tree-inline.cc, so the unrolling can just use different APIs to perform it. I'd still prefer to do it like that, I think it will pay back in maintainance costs. If you don't get to this within say 2 weeks, I'll try to do the conversion myself. Jakub
Re: [PATCH 0/7] openmp: OpenMP 5.1 loop transformation directives
On Tue, May 16, 2023 at 11:45:16AM +0200, Frederik Harwath wrote:
> The place where different compilers implement the loop transformations
> was discussed in an OpenMP loop transformation meeting last year. Two
> compilers (another one and GCC with this patch series) transformed the loops
> in the middle end after the handling of data sharing, one planned to do so.
> Yet another vendor had not yet decided where it will be implemented. Clang
> currently does everything in the front end, but it was mentioned that this
> might change in the future e.g. for code sharing with Flang. Implementing
> the loop transformations late could potentially
> complicate the implementation of transformations which require adjustments
> of the data sharing clauses, but this is known and consequentially, no such
When already in the FE we determine how many canonical loops a particular
loop transformation creates, I think the primary changes I'd like to see is
really have OMP_UNROLL/OMP_TILE GENERIC statements (see below) and consider
where is the best spot to lower it. I believe for data sharing it is best
done during gimplification before the containing loops are handled, it is
already shared code among all the FEs, I think will make it easier to handle
data sharing right and gimplification is also where doacross processing is
done. While there is restriction that ordered clause is incompatible with
generated loops from tile construct, there isn't one for unroll (unless
"The ordered clause must not appear on a worksharing-loop directive if the
associated loops
include the generated loops of a tile directive."
means unroll partial implicitly because partial unroll tiles the loop, but
it doesn't say it acts as if it was a tile construct), so we'd have to handle
#pragma omp for ordered(2)
for (int i = 0; i < 64; i++)
#pragma omp unroll partial(4)
for (int j = 0; j < 64; j++)
{
#pragma omp ordered depend (sink: i - 1, j - 2)
#pragma omp ordered depend (source)
}
and I think handling it after gimplification is going to be increasingly
harder. Of course another possibility is ask lang committee to clarify
unless it has been clarified already in 6.0 (but in TR11 it is not).
Also, I think creating temporaries is easier to be done during
gimplification than later.
Another option is as you implemented a separate pre-omp-lowering pass,
and another one would be do it in the omplower pass, which has actually
several subpasses internally, do it in the scan phase. Disadvantage of
a completely separate pass is that we have to walk the whole IL again,
while doing it in the scan phase means we avoid that cost. We already
do there similar transformations, scan_omp_simd transforms simd constructs
into if (...) simd else simt and then we process it with normal scan_omp_for
on what we've created. So, if you insist doing it after gimplification
perhaps for compatibility with other non-LLVM compilers, I'd prefer to
do it there rather than in a completely separate pass.
> transformations are planned for OpenMP 6.0. In particular, the "apply"
> clause therefore only permits loop-transforming constructs to be applied to
> the loops generated from other loop
> transformations in TR11.
>
> > The normal loop constructs (OMP_FOR, OMP_SIMD, OMP_DISTRIBUTE, OMP_LOOP)
> > already need to know given their collapse/ordered how many loops they are
> > actually associated with and the loop transformation constructs can change
> > that.
> > So, I think we need to do the loop transformations in the FEs, that doesn't
> > mean we need to write everything 3 times, once for each frontend.
> > Already now, e.g. various stuff is shared between C and C++ FEs in c-family,
> > though how much can be shared between c-family and Fortran is to be
> > discovered.
> > Or at least partially, to the extent that we compute how many canonical
> > loops the loop transformations result in, what artificial iterators they
> > will use etc., so that during gimplification we can take all that into
> > account and then can do the actual transformations later.
>
> The patches in this patch series already do compute how many canonical
> loop nests result from the loop transformations in the front end.
Good.
> This is necessary to represent the loop nest that is affected by the
> loop transformations by a single OMP_FOR to meet the expectations
> of all later OpenMP code transformations. This is also the major
> reason why the loop transformations are represented by clauses
> instead of representing them as "OMP_UNROLL/OMP_TILE as
> GENERIC constructs like OMP_FOR" as you suggest below. Since the
I really don't see why. We try to represent what we see in the source
as OpenMP constructs as those constructs. We already have a precedent
with composite loop constructs, where for the combined constructs which
aren't innermost we temporarily use NULL OMP_FOR_{INIT,COND,INCR,ORIG_DECLS}
vectors to stand for this will be some loop, but the details for it
Re: [PATCH 0/7] openmp: OpenMP 5.1 loop transformation directives
On Mon, May 15, 2023 at 12:19:00PM +0200, Jakub Jelinek via Gcc-patches wrote:
> For C++ in templates we obviously need to defer that until instantiations,
> the constants in the clauses etc. could be template parameters etc.
Even in C++ the how many canonical loop nest form loops does this
transformation generate can be probably answered during parsing at least
for the 5.1/5.2 loop transformations.
I think we don't really allow
template
void foo ()
{
#pragma omp for collapse(2)
#pragma omp tile sizes(args...)
for (int i = 0; i < 64; i++)
for (int j = 0; j < 64; j++)
for (int k = 0; k < 64; k++)
;
}
there how many arguments sizes clause has would be determined only
after instantiation. Of course, we don't know the exact values...
Jakub
Re: [PATCH 0/7] openmp: OpenMP 5.1 loop transformation directives
On Fri, Mar 24, 2023 at 04:30:38PM +0100, Frederik Harwath wrote:
> this patch series implements the OpenMP 5.1 "unroll" and "tile"
> constructs. It includes changes to the C,C++, and Fortran front end
> for parsing the new constructs and a new middle-end
> "omp_transform_loops" pass which implements the transformations in a
> source language agnostic way.
I'm afraid we can't do it this way, at least not completely.
The OpenMP requirements and what is being discussed for further loop
transformations pretty much requires parts of it to be done as soon as possible.
My understanding is that that is where other implementations implement that
too and would also prefer GCC not to be the only implementation that takes
significantly different decision in that case from other implementations
like e.g. in the offloading case (where all other implementations
preprocess/parse etc. source multiple times compared to GCC splitting stuff
only at IPA time; this affects what can be done with metadirectives,
declare variant etc.).
Now, e.g. data sharing is done almost exclusively during gimplification,
the proposed pass is later than that; it needs to be done before the data
sharing. Ditto doacross handling.
The normal loop constructs (OMP_FOR, OMP_SIMD, OMP_DISTRIBUTE, OMP_LOOP)
already need to know given their collapse/ordered how many loops they are
actually associated with and the loop transformation constructs can change
that.
So, I think we need to do the loop transformations in the FEs, that doesn't
mean we need to write everything 3 times, once for each frontend.
Already now, e.g. various stuff is shared between C and C++ FEs in c-family,
though how much can be shared between c-family and Fortran is to be
discovered.
Or at least partially, to the extent that we compute how many canonical
loops the loop transformations result in, what artificial iterators they
will use etc., so that during gimplification we can take all that into
account and then can do the actual transformations later.
For C, I think the lowering of loop transformation constructs or at least
determining what it means can be done right after we actually parse it and
before we finalize the OMP_FOR eetc. that wraps it if any. As discussed last
week at F2F, I think we want to remember in OMP_FOR_ORIG_DECLS the user
iterators on the loop transformation constructs and take it into account
for data sharing purposes.
For C++ in templates we obviously need to defer that until instantiations,
the constants in the clauses etc. could be template parameters etc.
For Fortran during resolving.
> The "unroll" and "tile" directives are
> internally implemented as clauses. This fits the representation of
So perhaps just use OMP_UNROLL/OMP_TILE as GENERIC constructs like
OMP_FOR etc. but with some argument where from the early loop
transformation analysis you can remember the important stuff, whether
does the loop transformation result in a canonical loop nest or not
and in the former case with how many nested loops.
And then handle the actual transformation IMHO best at gimplification
time, find them in the OMP_FOR etc. body if they are nested in there,
let the transformation happen on GENERIC before the containing OMP_FOR
etc. if any is actually finalized and from the transformation remember
the original user decls and what should happen with them for data sharing
(e.g. lastprivate/lastprivate conditional).
>From the slides I saw last week, a lot of other transformations are in the
planning, like loop reversal etc.
And, I think even in OpenMP 5.1 nothing prevents e.g.
#pragma omp for collapse(3) // etc.
#pragma omp tile sizes (4, 2, 2)
#pragma omp tile sizes (4, 8, 16)
for (int i = 0; i < 64; ++i)
for (int j = 0; j < 64; ++j)
for (int k = 0; k < 64; ++k)
body;
where the inner tile takes the i and j loops and makes
for (int i1 = 0; i1 < 64; i1 += 4)
for (int j1 = 0; j1 < 64; j1 += 8)
for (int k1 = 0; k1 < 64; k1 += 16)
for (int i2 = 0; i2 < 4; i2++)
{
int i = i1 + i2;
for (int j2 = 0; j2 < 8; j2++)
{
int j = j1 + j2;
for (int k2 = 0; k2 < 16; k2++)
{
int k = k1 + k2;
body;
}
}
}
out of it with 3 outer loops which have canonical loop form (the rest
doesn't). And then the second tile takes the outermost 3 of those generated
loops and tiles them again, making it into again 3 canonical loop form
loops plus stuff inside of it.
Or one can replace the
#pragma omp for collapse(3) // etc.
with
#pragma omp for
#pragma omp unroll partial(2)
which furthermore unrolls the outermost generated loop from the outer tile
turning it into 1 canonical loop form loop plus stuff in it.
Or of course as you have in your testcases, some loop transformation
constructs could be used on more nested loops, not necessarily before
the outermost one. But still, in all cases you need to know quite early
how
Re: [patch, Fortran] Enable -fwrapv for -std=legacy
On Fri, Mar 10, 2023 at 06:54:10PM +0100, Thomas Koenig via Gcc-patches wrote: > Hello world, here's the patch that was discussed. > > Regression-tested. OK for trunk? > > Since this appeared only in gcc13, I see no need for a backport. > I will also document this in the changes file. > > Best regards > > Thomas > > Set -frapv if -std=legacy is set. s/frapv/fwrapv/ > Fortran legacy codes sometimes contain linear congruential > seudorandom number generators. These generators implicitly depend > on wrapping behavior on integer overflow, which is illegal Fortran, > but the best they could to at the time. > > A gcc13 change exposed this in rnflow, part of the Polyhedron > benchmark, with -O3. Rather than "regress" on such code, this patch > enables -fwrapv if -std=legacy is enabled. This allows the benchmark > to run successfully, and presumably lots of other code as well. I think it certainly shouldn't overwrite it, it can adjust the default, but if user asks for -std=legacy -fno-wrapv or -fno-wrapv -std=legacy, it should honor that. > gcc/fortran/ChangeLog: > > PR fortran/109075 > * options.cc (gfc_handle_option): If -std=legacy is set, s/ / / > also set -frwapv. s/rwapv/wrapv/ > * invoke.texi: Document the change. > --- a/gcc/fortran/options.cc > +++ b/gcc/fortran/options.cc > @@ -797,6 +797,8 @@ gfc_handle_option (size_t scode, const char *arg, > HOST_WIDE_INT value, > case OPT_std_legacy: >set_default_std_flags (); >gfc_option.warn_std = 0; > + /* -std=legacy implies -fwapv, but the user can override it. */ > + flag_wrapv = 1; >break; > > case OPT_fshort_enums: So, I think it should be done later, after option processing, say in gfc_post_options if it is possible to determine if -std=legacy has been specified (and not say -std=legacy -std=f2018 etc.), and using SET_OPTION_IF_UNSET. Jakub
Re: [Patch] OpenMP/Fortran: Fix handling of optional is_device_ptr + bind(C) [PR108546]
On Tue, Feb 28, 2023 at 05:18:25PM +0100, Tobias Burnus wrote: > OpenMP/Fortran: Fix handling of optional is_device_ptr + bind(C) [PR108546] > > For is_device_ptr, optional checks should only be done before calling > libgomp, afterwards they are NULL either because of absent or, by > chance, because it is unallocated or unassociated (for pointers/allocatables). > > Additionally, it fixes an issue with explicit mapping for 'type(c_ptr)'. > > PR middle-end/108546 > > gcc/fortran/ChangeLog: > > * trans-openmp.cc (gfc_trans_omp_clauses): Fix mapping of > type(C_ptr) variables. > > gcc/ChangeLog: > > * omp-low.cc (lower_omp_target): Remove optional handling > on the receiver side, i.e. inside target (data), for > use_device_ptr. > > libgomp/ChangeLog: > > * testsuite/libgomp.fortran/is_device_ptr-3.f90: New test. > * testsuite/libgomp.fortran/use_device_ptr-optional-4.f90: New test. Ok, thanks. Jakub
Re: [Patch] Fortran: Avoid SAVE_EXPR for deferred-len char types
On Tue, Feb 21, 2023 at 08:30:50AM +0100, Richard Biener wrote: > > I think this mostly helps with access inside the FE of the type 'size = > > TYPE_SIZE_UNIT(type)', which is used surprisingly often and is often > > directly evaluated (i.e. assigned to a temporary). > > that's what I thought So, either we can do a temporary hack where we stick the non-SAVE_EXPR in there but somehow mark those types in type_lang_specific (if they aren't yet) and clear that when passing the type from FE to the middle-end. Or, stick some bogus value into TYPE_SIZE_UNIT (error_mark_node or something worse that triggers ICEs all around, say VOID_CST) and fix up what breaks, say add a short function which will replace TYPE_SIZE_UNIT (type) and do if (TYPE_LANG_SPECIFIC (type) && deferred_len (type)) return some_type_lang_specific_field (type); else return TYPE_SIZE_UNIT (type) and replace those. Or mass replace TYPE_SIZE_UNIT (type) in the FE with the new function. Though, there surely are spots for which deferred-len types may never appear... Jakub
Re: [Patch] Fortran: Avoid SAVE_EXPR for deferred-len char types
On Mon, Feb 20, 2023 at 12:48:38PM +0100, Tobias Burnus wrote: > On 20.02.23 12:15, Jakub Jelinek wrote: > > On Mon, Feb 20, 2023 at 12:07:43PM +0100, Tobias Burnus wrote: > > > As mentioned in the TODO for 'deferred', I think we really want > > > to have NULL as upper value for the domain for the type, but that > > > requires literally hundred of changes to the compiler, which > > > I do not want to due during Stage 4, but that are eventually > > > required.* — In any case, this patch fixes some of the issues > > > in the meanwhile. > > Yeah, the actual len can be in some type's lang_specific member. > > Actually, I think it should be bound to the DECL and not to the TYPE, > i.e. lang_decl not type_lang. > > I just see that, the latter already has a 'tree stringlen' (for I/O) > which probably could be reused for this purpose. I'd drop the && TREE_CODE (TYPE_SIZE (type)) == SAVE_EXPR and assert == SAVE_EXPR part, with SAVE_EXPRs one never knows if they are added around the whole expression or say some subexpression has it and then some trivial arithmetics happens on the SAVE_EXPR tree. > > Anyway, for the patch for now, I'd probably instead of stripping > > SAVE_EXPR overwrite the 2 sizes with newly built expressions. > > What I now did. (Unchanged otherwise, except that I now also mention > GFC_DECL_STRING_LEN in the TODO.) > > OK for mainline? If Richard doesn't object. Jakub
Re: [Patch] Fortran: Avoid SAVE_EXPR for deferred-len char types
On Mon, Feb 20, 2023 at 12:07:43PM +0100, Tobias Burnus wrote: > As mentioned in the TODO for 'deferred', I think we really want > to have NULL as upper value for the domain for the type, but that > requires literally hundred of changes to the compiler, which > I do not want to due during Stage 4, but that are eventually > required.* — In any case, this patch fixes some of the issues > in the meanwhile. Yeah, the actual len can be in some type's lang_specific member. Anyway, for the patch for now, I'd probably instead of stripping SAVE_EXPR overwrite the 2 sizes with newly built expressions. Jakub
Re: [Patch][v2] OpenMP/Fortran: Fix loop-iter var privatization with !$OMP LOOP [PR108512]
On Fri, Feb 10, 2023 at 12:52:47PM +0100, Tobias Burnus wrote:
> > I'm afraid this is needed but insufficient.
> > I think
> > case EXEC_OMP_MASKED_TASKLOOP:
> > case EXEC_OMP_MASKED_TASKLOOP_SIMD:
> > case EXEC_OMP_MASTER_TASKLOOP:
> > case EXEC_OMP_MASTER_TASKLOOP_SIMD:
> > case EXEC_OMP_PARALLEL_LOOP:
> > case EXEC_OMP_TARGET_PARALLEL_LOOP:
> > case EXEC_OMP_TARGET_TEAMS_LOOP:
> > case EXEC_OMP_TARGET_SIMD:
> > case EXEC_OMP_TEAMS_LOOP:
> > should be in the list above (of course alphabetically sorted in between the
> > others)
> > gfc_resolve_omp_parallel_blocks (code, ns);
>
> I think 'TARGET_SIMD' shouldn't be resolved though parallel blocks but
You're right, we use gfc_resolve_omp_parallel_blocks for
parallel, teams, task but not for target alone.
> can call directly call gfc_resolve_omp_do_blocks (as
> currently/previously implemented). The masked version were already
> handled inside gfc_resolve_omp_parallel_blocks but missing in
> gfc_resolve_code, while the 'loop' ones had to be added to both.
>
> (I did not extend the testcase, but I updated two to add additional
> dg-error to the same line.)
> gcc/fortran/ChangeLog:
>
> PR fortran/108512
> * openmp.cc (gfc_resolve_omp_parallel_blocks): Handle combined 'loop'
> directives.
> (gfc_resolve_do_iterator): Set a source location for added
> 'private'-clause arguments.
> * resolve.cc (gfc_resolve_code): Call gfc_resolve_omp_do_blocks
> also for EXEC_OMP_LOOP and gfc_resolve_omp_parallel_blocks for
> combined directives with loop + '{masked,master} taskloop (simd)'.
>
> gcc/testsuite/ChangeLog:
>
> PR fortran/108512
> * gfortran.dg/gomp/loop-5.f90: New test.
> * gfortran.dg/gomp/loop-2.f90: Update dg-error.
> * gfortran.dg/gomp/taskloop-2.f90: Update dg-error.
LGTM, thanks.
Jakub
Re: [Patch][v2] OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
On Thu, Feb 09, 2023 at 03:46:35PM +0100, Tobias Burnus wrote: > I think the test coverage should be sufficient. Any further test idea? > Otherwise, I would commit it now. LGTM, thanks. Jakub
Re: [Patch] Fortran/OpenMP: Fix -fopenmp-simd for 'omp assume(s)'
On Thu, Feb 09, 2023 at 09:56:09AM +0100, Tobias Burnus wrote: > Found by chance recently; I thought about a couple of ways to handle it > but then settled to the proposed solution. > > OK for mainline? > > 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 > Fortran/OpenMP: Fix -fopenmp-simd for 'omp assume(s)' > > While 'omp assume' is enabled by -fopenmp-simd, 'omp assumes' is not; > however, due to the way parsing works in Fortran (esp. for fixed-form > source code), 'assumes' was parsed by 'assume' which then stumbled over > the tailing 's'. > > gcc/fortran/ > > * parse.cc (decode_omp_directive): Really ignore 'assumes' with > -fopenmp-simd. > > gcc/testsuite/ > > * gfortran.dg/gomp/openmp-simd-8.f90: New test. Ok. Jakub
[PATCH] fortran: Fix up hash table usage in gfc_trans_use_stmts [PR108451]
Hi!
The first testcase in the PR (which I haven't included in the patch because
it is unclear to me if it is supposed to be valid or not) ICEs since extra
hash table checking has been added recently. The problem is that
gfc_trans_use_stmts does
tree *slot = entry->decls->find_slot_with_hash (rent->use_name, hash,
INSERT);
if (*slot == NULL)
and later on doesn't store anything into *slot and continues. Another spot
a few lines later correctly clears the slot if it decides not to use the
slot, so the following patch does the same.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2023-02-03 Jakub Jelinek
PR fortran/108451
* trans-decl.cc (gfc_trans_use_stmts): Call clear_slot before
doing continue.
--- gcc/fortran/trans-decl.cc.jj2023-01-16 11:52:16.146733136 +0100
+++ gcc/fortran/trans-decl.cc 2023-02-03 14:41:40.503322954 +0100
@@ -5350,7 +5350,11 @@ gfc_trans_use_stmts (gfc_namespace * ns)
/* Sometimes, generic interfaces wind up being over-ruled by a
local symbol (see PR41062). */
if (!st->n.sym->attr.use_assoc)
- continue;
+ {
+ *slot = error_mark_node;
+ entry->decls->clear_slot (slot);
+ continue;
+ }
if (st->n.sym->backend_decl
&& DECL_P (st->n.sym->backend_decl)
Jakub
Re: libquadmath, glibc, and the Q format flag
On Wed, Feb 01, 2023 at 12:29:02PM +0100, Florian Weimer via Gcc wrote: > >> This impacts most (all?) Fortran code on GNU/Linux because libgfortran > >> depends on libquadmath. > > > > Not anymore. > > If GCC is configured against new enough glibc (with _Float128 support) > > libgfortran.so.5 is no longer linked against -lquadmath, and -lquadmath > > is added only --as-needed to ld command line, so if all the Fortran object > > files that need real(kind=16) (or quad complex) are built by such configured > > GCC, -lquadmath is not linked in (just needs to be present). > > Hmm, I missed that recent change. > > Would it make sense to drop the libgfortran RPM dependency on > libquadmath in Fedora rawhide? This is a downstream question. We could drop it from libgfortran (package that contains just the shared library), but we still need it on gcc-gfortran (as the library is used during linking for backwards compatibility reasons). Jakub
Re: libquadmath, glibc, and the Q format flag
On Wed, Feb 01, 2023 at 11:56:42AM +0100, Florian Weimer via Gcc wrote: > I recently discovered that libquadmath registers custom printf callbacks > on load. As far as I can tell, this is done so that the Q format flag > can be used to print floating point numbers, using format strings such > as "%Qf". To enable Q flag processing, libquadmath has to register > replacements for all floating point format specifiers (aAeEfFgG). > > Unfortunately, this has the side effect that even with out the Q flag, > printing any floating point number enters a generic, slow path in glibc, > prior to the number formatting itself. The effect is quite pronounced. > For example this admittedly silly benchmarking script > > for i=1,500 do > print(i, i * math.pi) > end > > runs for 5.8 seconds without libquadmath.so.0 loaded on my x86-64 > system. With libquadmath.so.0 from GCC 12 loaded, it runs for 6.3 > seconds. > > This impacts most (all?) Fortran code on GNU/Linux because libgfortran > depends on libquadmath. Not anymore. If GCC is configured against new enough glibc (with _Float128 support) libgfortran.so.5 is no longer linked against -lquadmath, and -lquadmath is added only --as-needed to ld command line, so if all the Fortran object files that need real(kind=16) (or quad complex) are built by such configured GCC, -lquadmath is not linked in (just needs to be present). And, even for C, users can just use the standard glibc _Float128 support (if they don't mind strfromf128) if they target new enough glibc. So, libquadmath should be left over for non-glibc uses or uses with old glibc. > Would it make sense to transplant the implementation of the Q specifier > from libquadmath to glibc, and disable the registration code in > libquadmath if glibc is recent enough? At least for the targets that > today have float128 support in glibc? Thus I'm not sure it is worth it. Jakub
Re: [Patch] Fortran: Extend align-clause checks of OpenMP's allocate clause
On Tue, Dec 13, 2022 at 05:38:22PM +0100, Tobias Burnus wrote: > I missed that 'align' needs to be a power of 2 - contrary to 'aligned', > which does not have this restriction for some odd reason. Yeah, odd. The C and C++ FEs indeed diagnose non-pow2p constants for align (and not for aligned clause). > > OK for mainline? Yes, thanks. Sorry for the delay. > Fortran: Extend align-clause checks of OpenMP's allocate directive > > gcc/fortran/ChangeLog: > > * openmp.cc (resolve_omp_clauses): Check also for > power of two. > > libgomp/ChangeLog: > > * testsuite/libgomp.fortran/allocate-3.f90: Fix ALIGN > usage, remove unused -fdump-tree-original. > * testsuite/libgomp.fortran/allocate-4.f90: New. Jakub
Re: [Patch][v2] OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
On Wed, Jan 25, 2023 at 03:47:18PM +0100, Tobias Burnus wrote:
> updated patch included, i.e. avoiding 'count' for 'j' when a 'j.0' would
> do (i.e. only local var without the different step calculation). I also
> now reject if there is a non-unit step on the loop using an outer var.
>
> Eventually still to be done: replace the 'sorry' by working code, i.e.
> implement the suggestions to handle some/all non-unit iteration steps as
> proposed in this thread.
>
> On 20.01.23 18:39, Jakub Jelinek wrote:
> > I think instead of non-unity etc. it is better to talk about constant
> > step 1 or -1.
>
> I concur.
>
>
> > The actual problem with non-simple loops for non-rectangular loops is
> > both in case it is an inner loop which uses some outer loop's iterator,
> > or if it is outer loop whose iterator is used, both of those cases
> > will not be handled properly.
>
> I have now added a check for the other case as well.
>
> Just to confirm, the following is fine, isn't it?
>
> !$omp simd collapse(4)
> do i = 1, 10, 2
> do outer_var = 1, 10 ! step = + 1
> do j = 1, 10, 2
> do inner_var = 1, outer_var ! step = 1
>
> i.e. both the inner_var and outer_var have 'step = 1',
> even if other loops in the 'collapse' have step != 1.
> I think it should be fine.
Yes, the loops which don't define outer_var for other loops nor
use outer_var from other loops can be in any form, we can compute
their number of iterations before the whole loop nest for them,
so for the non-rectangular iterations computations we can ignore
those except for multiplication by the pre-computed count.
> OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
>
> This patch ensures that loop bounds depending on outer loop vars use the
> proper TREE_VEC format. It additionally gives a sorry if such an outer
> var has a non-one/non-minus-one increment as currently a count variable
> is used in this case (see PR).
>
> Finally, it avoids 'count' and just uses a local loop variable if the
> step increment is +/-1.
>
> PR fortran/107424
>
> gcc/fortran/ChangeLog:
>
> * trans-openmp.cc (struct dovar_init_d): Add 'sym' and
> 'non_unit_incr' members.
> (gfc_nonrect_loop_expr): New.
> (gfc_trans_omp_do): Call it; use normal loop bounds
> for unit stride - and only create local loop var.
>
> libgomp/ChangeLog:
>
> * testsuite/libgomp.fortran/non-rectangular-loop-1.f90: New test.
> * testsuite/libgomp.fortran/non-rectangular-loop-1a.f90: New test.
> * testsuite/libgomp.fortran/non-rectangular-loop-2.f90: New test.
> * testsuite/libgomp.fortran/non-rectangular-loop-3.f90: New test.
> * testsuite/libgomp.fortran/non-rectangular-loop-4.f90: New test.
> * testsuite/libgomp.fortran/non-rectangular-loop-5.f90: New test.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/goacc/privatization-1-compute-loop.f90: Update dg-note.
> * gfortran.dg/goacc/privatization-1-routine_gang-loop.f90: Likewise.
>
> +static bool
> +gfc_nonrect_loop_expr (stmtblock_t *pblock, gfc_se *sep, int loop_n,
> +gfc_code *code, gfc_expr *expr, vec *inits,
> +int simple, gfc_expr *curr_loop_var)
> +{
> + int i;
> + for (i = 0; i < loop_n; i++)
> +{
> + gcc_assert (code->ext.iterator->var->expr_type == EXPR_VARIABLE);
> + if (gfc_find_sym_in_expr (code->ext.iterator->var->symtree->n.sym,
> expr))
> + break;
> + code = code->block->next;
> +}
> + if (i >= loop_n)
> +return false;
> +
> + /* Canonic format: TREE_VEC with [var, multiplier, offset]. */
I think we use everywhere Canonical rather than Canonic
> + gfc_symbol *var = code->ext.iterator->var->symtree->n.sym;
> +
> + tree tree_var = NULL_TREE;
> + tree a1 = integer_one_node;
> + tree a2 = integer_zero_node;
> +
> + if (!simple)
> +{
> + /* FIXME: Handle non-unit iter steps, cf. PR fortran/107424. */
> + sorry_at (gfc_get_location (_loop_var->where),
> + "non-rectangular loop nest with step other than constant 1 "
> + "or -1 for %qs", curr_loop_var->symtree->n.sym->name);
> + return false;
> +}
> +
> + dovar_init *di;
> + unsigned ix;
> + FOR_EACH_VEC_ELT (*inits, ix, di)
> +if (di->sym == var && !di->non_unit_iter)
> + {
> + tree_var = di->init;
> + gcc_assert (DECL_P (tree_var));
> + break;
> + }
> +else if (di->sym == var)
> + {
> + /* FIXME: Handle non-unit
Re: [Patch] OpenMP/Fortran: Fix loop-iter var privatization with !$OMP LOOP [PR108512]
On Tue, Jan 24, 2023 at 04:24:07PM +0100, Tobias Burnus wrote:
> gcc/fortran/ChangeLog:
>
> PR fortran/108512
> * openmp.cc (gfc_resolve_omp_do_blocks): Don't check 'inscan'
> restrictions for loop as rejected elsewhere.
> (gfc_resolve_do_iterator): Set a source location for added
> 'private'-clause arguments.
> * resolve.cc (gfc_resolve_code): Call gfc_resolve_omp_do_blocks
> also for EXEC_OMP_LOOP.
>
> gcc/testsuite/ChangeLog:
>
> PR fortran/108512
> * gfortran.dg/gomp/loop-5.f90: New test.
>
> gcc/fortran/openmp.cc | 5 +-
> gcc/fortran/resolve.cc| 1 +
> gcc/testsuite/gfortran.dg/gomp/loop-5.f90 | 84
> +++
> 3 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
> index cc1eab90b8c..7673a52249f 100644
> --- a/gcc/fortran/openmp.cc
> +++ b/gcc/fortran/openmp.cc
> @@ -9056,7 +9056,9 @@ gfc_resolve_omp_do_blocks (gfc_code *code,
> gfc_namespace *ns)
> }
>if (i < omp_current_do_collapse || omp_current_do_collapse <= 0)
> omp_current_do_collapse = 1;
> - if (code->ext.omp_clauses->lists[OMP_LIST_REDUCTION_INSCAN])
> + if (code->op == EXEC_OMP_LOOP)
> + ; /* Already rejected in resolve_omp_clauses. */
I don't understand why is this needed. Sure, the vast majority of
constructs don't allow reduction(inscan, ...), do we need to list them all?
Is EXEC_OMP_LOOP somehow reusing that list for something else? What about
EXEC_OMP_*_LOOP? If not, how does that differ say from EXEC_OMP_DISTRIBUTE
or EXEC_OMP_TASKLOOP and many others?
If it is rejected earlier, then perhaps we should free/clear the list
after we diagnose it if it causes harm later.
> + else if (code->ext.omp_clauses->lists[OMP_LIST_REDUCTION_INSCAN])
> {
> locus *loc
> = >ext.omp_clauses->lists[OMP_LIST_REDUCTION_INSCAN]->where;
> @@ -9224,6 +9226,7 @@ gfc_resolve_do_iterator (gfc_code *code, gfc_symbol
> *sym, bool add_clause)
>
>p = gfc_get_omp_namelist ();
>p->sym = sym;
> + p->where = omp_current_ctx->code->loc;
>p->next = omp_clauses->lists[OMP_LIST_PRIVATE];
>omp_clauses->lists[OMP_LIST_PRIVATE] = p;
> }
Ok.
> diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
> index 94213cd3cd4..bd2a749776d 100644
> --- a/gcc/fortran/resolve.cc
> +++ b/gcc/fortran/resolve.cc
> @@ -11950,6 +11950,7 @@ gfc_resolve_code (gfc_code *code, gfc_namespace *ns)
> case EXEC_OMP_DISTRIBUTE_SIMD:
> case EXEC_OMP_DO:
> case EXEC_OMP_DO_SIMD:
> + case EXEC_OMP_LOOP:
> case EXEC_OMP_SIMD:
> case EXEC_OMP_TARGET_SIMD:
> gfc_resolve_omp_do_blocks (code, ns);
I'm afraid this is needed but insufficient.
I think
case EXEC_OMP_MASKED_TASKLOOP:
case EXEC_OMP_MASKED_TASKLOOP_SIMD:
case EXEC_OMP_MASTER_TASKLOOP:
case EXEC_OMP_MASTER_TASKLOOP_SIMD:
case EXEC_OMP_PARALLEL_LOOP:
case EXEC_OMP_TARGET_PARALLEL_LOOP:
case EXEC_OMP_TARGET_TEAMS_LOOP:
case EXEC_OMP_TARGET_SIMD:
case EXEC_OMP_TEAMS_LOOP:
should be in the list above (of course alphabetically sorted in between the
others)
gfc_resolve_omp_parallel_blocks (code, ns);
(the non-parallel-workshare one).
Went through the c-family/c-omp.cc list in comment above splitting
function and checked all appropriate constructs there...
> diff --git a/gcc/testsuite/gfortran.dg/gomp/loop-5.f90
> b/gcc/testsuite/gfortran.dg/gomp/loop-5.f90
> new file mode 100644
> index 000..1948e782653
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/loop-5.f90
> @@ -0,0 +1,84 @@
> +! { dg-additional-options "-fdump-tree-original" }
> +!
> +! PR fortran/108512
> +
> +! The problem was that the context wasn't reset for the 'LOOP'
> +! such that the clauses of the loops weren't seen when adding
> +! PRIVATE clauses.
> +!
> +! In the following, only the loop variable of the non-OpenMP loop
> +! in 'subroutine four' should get a front-end addded PRIVATE clause
> +
> +implicit none
> +integer :: x, a(10), b(10), n
> +n = 10
> +a = -42
> +b = [(2*x, x=1,10)]
> +
> +! { dg-final { scan-tree-dump-times "#pragma omp target map\\(tofrom:a\\)
> map\\(tofrom:b\\) map\\(tofrom:x\\)\[\r\n\]" 1 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp parallel\[\r\n\]" 2
> "original" } }
> +! ^- shows up twice; checked only here.
> +! { dg-final { scan-tree-dump-times "#pragma omp loop
> lastprivate\\(x\\)\[\r\n\]" 1 "original" } }
> +
> +!$omp target parallel map(tofrom: a, b, x)
> +!$omp loop lastprivate(x)
> +DO x = 1, n
> + a(x) = a(x) + b(x)
> +END DO
> +!$omp end loop
> +!$omp end target parallel
> +if (x /= 11) error stop
> +if (any (a /= [(2*x - 42, x=1,10)])) error stop
> +call two()
>
Re: [Patch] OpenMP/Fortran: Fix has_device_addr clause splitting [PR108558]
On Fri, Jan 27, 2023 at 10:19:42AM +0100, Tobias Burnus wrote:
> Rather obvious fix. Hence, I intent to commit it later as obvious,
> unless there are any comments.
Yeah, this is obviously correct.
Have you checked the function if we don't miss other clauses in there
(e.g. compared to the C implementation)?
> OpenMP/Fortran: Fix has_device_addr clause splitting [PR108558]
>
> gcc/fortran/ChangeLog:
>
> PR fortran/108558
> * trans-openmp.cc (gfc_split_omp_clauses): Handle has_device_addr.
>
> libgomp/ChangeLog:
>
> PR fortran/108558
> * testsuite/libgomp.fortran/has_device_addr.f90: New test.
>
> gcc/fortran/trans-openmp.cc| 2 +
> .../testsuite/libgomp.fortran/has_device_addr.f90 | 59
> ++
> 2 files changed, 61 insertions(+)
>
> diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
> index 87213de0918..5283d0ce5f3 100644
> --- a/gcc/fortran/trans-openmp.cc
> +++ b/gcc/fortran/trans-openmp.cc
> @@ -6205,6 +6205,8 @@ gfc_split_omp_clauses (gfc_code *code,
> = code->ext.omp_clauses->lists[OMP_LIST_MAP];
> clausesa[GFC_OMP_SPLIT_TARGET].lists[OMP_LIST_IS_DEVICE_PTR]
> = code->ext.omp_clauses->lists[OMP_LIST_IS_DEVICE_PTR];
> + clausesa[GFC_OMP_SPLIT_TARGET].lists[OMP_LIST_HAS_DEVICE_ADDR]
> + = code->ext.omp_clauses->lists[OMP_LIST_HAS_DEVICE_ADDR];
> clausesa[GFC_OMP_SPLIT_TARGET].device
> = code->ext.omp_clauses->device;
> clausesa[GFC_OMP_SPLIT_TARGET].thread_limit
> diff --git a/libgomp/testsuite/libgomp.fortran/has_device_addr.f90
> b/libgomp/testsuite/libgomp.fortran/has_device_addr.f90
> new file mode 100644
> index 000..95cc7788f2d
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.fortran/has_device_addr.f90
> @@ -0,0 +1,59 @@
> +! { dg-additional-options "-fdump-tree-original" }
> +
> +!
> +! PR fortran/108558
> +!
> +
> +! { dg-final { scan-tree-dump-times "#pragma omp target
> has_device_addr\\(x\\) has_device_addr\\(y\\)" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp target data
> map\\(tofrom:x\\) map\\(tofrom:y\\)" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp target data
> use_device_addr\\(x\\) use_device_addr\\(y\\)" 1 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp target update from\\(y\\)"
> 1 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp target data
> map\\(tofrom:x\\) map\\(tofrom:y\\) use_device_addr\\(x\\)
> use_device_addr\\(y\\)" 1 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp teams" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp distribute" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp parallel" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "#pragma omp for nowait" 2 "original" } }
> +
> +module m
> +contains
> +subroutine vectorAdd(x, y, N)
> + implicit none
> + integer :: N
> + integer(4) :: x(N), y(N)
> + integer :: i
> +
> + !$omp target teams distribute parallel do has_device_addr(x, y)
> + do i = 1, N
> +y(i) = x(i) + y(i)
> + end do
> +end subroutine vectorAdd
> +end module m
> +
> +program main
> + use m
> + implicit none
> + integer, parameter :: N = 9876
> + integer(4) :: x(N), y(N)
> + integer :: i
> +
> + x(:) = 1
> + y(:) = 2
> +
> + !$omp target data map(x, y)
> +!$omp target data use_device_addr(x, y)
> + call vectorAdd(x, y, N)
> +!$omp end target data
> +!$omp target update from(y)
> +if (any (y /= 3)) error stop
> + !$omp end target data
> +
> + x = 1
> + y = 2
> + !$omp target data map(x, y) use_device_addr(x, y)
> +!$omp target teams distribute parallel do has_device_addr(x, y)
> +do i = 1, N
> + y(i) = x(i) + y(i)
> +end do
> + !$omp end target data
> + if (any (y /= 3)) error stop
> +end program
Jakub
Re: [Patch] OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
On Fri, Jan 20, 2023 at 07:00:18PM +0100, Jakub Jelinek via Gcc-patches wrote:
> Though, I wonder if we shouldn't for GCC 13 just sorry_at about
> steps other than constant 1/-1 (in both outer loop with var-outer referenced
> in inner loop and on inner loop that references it) and for the !VAR_P case
> actually handle it if step 1/-1 by using simple like translation just with
> an artificial iterator.
As for the steps other than constant 1/-1, we have 5 cases:
do i = x, y, 25
or
do i = 12, 72, z
or
do i = x, y, -42
or
do i = 42, -10, z
or
do i = x, y, z
The 1st and 3rd are with constant step, 2nd and 4th with constant lower and
upper bounds and the last one has step and at least one of the bounds
non-constant.
I wonder if in the light of e.g. PR108431 which says that
do i = -huge(i), huge(i) is invalid (well, that one would be very wrong
even from OpenMP POV because computing number of iterations definitely
overflows) and the fact that we handle step 1 and -1 the simple way
do do i = huge(i) - 10, huge(i) will not work either, I wonder if even
do i = huge(i) - 5, huge(i) - 1, 2 is undefined (similar reasoning, if
i after loop needs to be set to the huge(i) + 1 it is signed integer
overflow). If yes, then perhaps at least the first 4 cases could be easily
handled (perhaps for GCC 13 just if clauses->non_rectangular only) as
for (i = x; i <= y; i += 25)
or
for (i = 12; i <= 72; i += z)
or
for (i = x; i >= y; i -= 42)
or
for (i = 42; i >= -10; i += z)
If those give equivalent behavior, then that would mean a sorry
only for the last case - the problem is that we then don't know at compile
time the direction.
Though perhaps even for that case we could play tricks, handle
do i = x, y, z
as
if (z > 0)
a = x, b = y, c = z;
else
a = INT_MIN, b = too_lazy_to_compute_that_now, c = -z;
for (counter = a; counter <= b; counter += c)
{
if (z > 0)
i = counter;
else
i = counter - (unsigned) INT_MAX;
}
If that works, we'd need to figure also out how to handle that
in the non-rect cases. But the m1 * var-outer + a1 and m2 * var-outer + a2
factors can be non-constant invariants, so again we could compute something
for them depending on if the outer or inner step was positive or negative.
Jakub
Re: [Patch] OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
On Fri, Jan 20, 2023 at 06:39:04PM +0100, Jakub Jelinek via Gcc-patches wrote:
> > + The issue is that for those a 'count' variable is used. */
> > +dovar_init *di;
> > +unsigned ix;
> > +tree t = tree_var;
> > +while (TREE_CODE (t) == INDIRECT_REF)
> > + t = TREE_OPERAND (t, 0);
> > +FOR_EACH_VEC_ELT (*inits, ix, di)
> > + {
> > + tree t2 = di->var;
> > + while (TREE_CODE (t2) == INDIRECT_REF)
> > + t2 = TREE_OPERAND (t2, 0);
>
> The actual problem with non-simple loops for non-rectangular loops is
> both in case it is an inner loop which uses some outer loop's iterator,
> or if it is outer loop whose iterator is used, both of those cases
> will not be handled properly. The former case because instead of
> having lb and ub expressions in canonicalized form var-outer * m + a
> lb will be 0 (that is fine) and ub will be
> (var-outer * m2 + a2 + step - var-outer * m1 - a1) / step
> or so (sure, we can simplify that to
> (var-outer * (m1 - m2) + (a2 + step - a1)) / step
> but the division remains. And the latter case is bad because we
> need var-outer but we actually compute some artificial count iterator
> and var-outer is only initialized in the body of the loop.
> These sorry_at seems to handle just one of those, when the outer
> loop whose var-outer is referenced is not simple, no?
Though, I wonder if we shouldn't for GCC 13 just sorry_at about
steps other than constant 1/-1 (in both outer loop with var-outer referenced
in inner loop and on inner loop that references it) and for the !VAR_P case
actually handle it if step 1/-1 by using simple like translation just with
an artificial iterator.
Say for:
subroutine foo (x, y, z)
integer :: x, y, z
!$omp do private (x)
do x = y, z
end do
end subroutine foo
we right now in *.original dump have:
D.4265 = *y;
D.4266 = *z;
D.4267 = (1 - D.4265) + D.4266;
#pragma omp for private(count.0) private(x)
for (count.0 = 0; count.0 < D.4267; count.0 = count.0 + 1)
{
*x = D.4265 + NON_LVALUE_EXPR ;
L.1:;
}
What I'd suggest is:
D.4265 = *y;
D.4266 = *z;
#pragma omp for private(x)
for (x.0 = D.4265; x.0 <= D.4266; x.0 = x.0 + 1)
{
*x = x.0;
L.1:;
}
or so. This could be done independently from the non-rect stuff,
as a first change.
Jakub
Re: [Patch] OpenMP/Fortran: Partially fix non-rect loop nests [PR107424]
On Thu, Jan 19, 2023 at 03:40:19PM +0100, Tobias Burnus wrote:
> + gfc_symbol *var = code->ext.iterator->var->symtree->n.sym;
> +
> + gfc_se se;
> + tree tree_var, a1, a2;
> + a1 = integer_one_node;
> + a2 = integer_zero_node;
> +
> + gfc_init_se (, NULL);
> + gfc_conv_expr_lhs (, code->ext.iterator->var);
> + gfc_add_block_to_block (pblock, );
> + tree_var = se.expr;
> +
> + {
> +/* FIXME: Handle non-unity iterations, cf. PR fortran/107424.
I think instead of non-unity etc. it is better to talk about
constant step 1 or -1.
> + The issue is that for those a 'count' variable is used. */
> +dovar_init *di;
> +unsigned ix;
> +tree t = tree_var;
> +while (TREE_CODE (t) == INDIRECT_REF)
> + t = TREE_OPERAND (t, 0);
> +FOR_EACH_VEC_ELT (*inits, ix, di)
> + {
> + tree t2 = di->var;
> + while (TREE_CODE (t2) == INDIRECT_REF)
> + t2 = TREE_OPERAND (t2, 0);
The actual problem with non-simple loops for non-rectangular loops is
both in case it is an inner loop which uses some outer loop's iterator,
or if it is outer loop whose iterator is used, both of those cases
will not be handled properly. The former case because instead of
having lb and ub expressions in canonicalized form var-outer * m + a
lb will be 0 (that is fine) and ub will be
(var-outer * m2 + a2 + step - var-outer * m1 - a1) / step
or so (sure, we can simplify that to
(var-outer * (m1 - m2) + (a2 + step - a1)) / step
but the division remains. And the latter case is bad because we
need var-outer but we actually compute some artificial count iterator
and var-outer is only initialized in the body of the loop.
These sorry_at seems to handle just one of those, when the outer
loop whose var-outer is referenced is not simple, no?
I wonder if it wouldn't be cleaner and easier to simply remember for
each loop in XALLOCAVEC array whether it was simple or not and why
(from the:
if (VAR_P (dovar))
{
if (integer_onep (step))
simple = 1;
else if (tree_int_cst_equal (step, integer_minus_one_node))
simple = -1;
}
else
dovar_decl
= gfc_trans_omp_variable (code->ext.iterator->var->symtree->n.sym,
false);
remember if it was simple (1/-1) or VAR_P !simple (then we would
if needed for non-rect sorry_at about step not being constant 1 or -1)
or if it is the !VAR_P case.
And then the non-rect sorry can be emitted for both the cases easily
(especially if you precompute the:
if (VAR_P (dovar))
{
if (integer_onep (step))
simple_loop[i] = 1;
else if (tree_int_cst_equal (step, integer_minus_one_node))
simple_loop[i] = -1;
else
simple_loop[i] = 0;
}
else
simple_loop[i] = 2;
early) and in this function check it for both loop_n and i.
> + if (t == t2)
> + {
> + HOST_WIDE_INT intval;
> + if (gfc_extract_hwi (code->ext.iterator->step, , 0) == 0
> + && intval != 1 && intval != -1)
> + sorry_at (gfc_get_location (>loc),
> + "non-rectangular loop nest with non-unit loop iteration"
> + " step for %qs", var->name);
I'd say step other than constant 1 or -1.
> + ! Use 'i' or 'j', unite stride on 'i' or on 'j' -> 4 loops
unit ?
> + ! Then same, execpt use nonunit stride for 'k'
except, non-unit ?
> + ! Use 'i' or 'j', unite stride on 'i' or on 'j' -> 4 loops
> + ! Then same, execpt use nonunit stride for 'k'
2x again
(and some more later).
Jakub
Re: [Patch] Fortran/OpenMP: Reject non-scalar 'holds' expr in 'omp assume(s)' [PR107424]
On Thu, Jan 12, 2023 at 11:22:40AM +0100, Tobias Burnus wrote: > Rather obvious fix for that ICE. > > Comments? If there are none, I will commit it later as obvious. I think the spec should be clarified, unlike clauses like if, novariants, nocontext, indirect, final clause operands where we specify the argument to be expression of logical type and glossary term says that OpenMP logical expression is scalar expression for C/C++ and scalar logical expression for Fortran. But for the holds clause, all we say is that holds clause isn't inarguable and "the program guarantees that the listed expression evaluates to true in the assumption scope. The effect of the clause does not include an observable evaluation of the expression." so I think making it clear that holds argument is expression of logical type would be useful. That said, the patch is ok, a rank > 1 expression can't be considered to evaluate to true... Jakub
[PATCH] fortran: Fix up function types for realloc and sincos{,f,l} builtins [PR108349]
Hi!
As reported in the PR, the FUNCTION_TYPE for __builtin_realloc in the
Fortran FE is wrong since r0-100026-gb64fca63690ad which changed
- tmp = tree_cons (NULL_TREE, pvoid_type_node, void_list_node);
- tmp = tree_cons (NULL_TREE, size_type_node, tmp);
- ftype = build_function_type (pvoid_type_node, tmp);
+ ftype = build_function_type_list (pvoid_type_node,
+size_type_node, pvoid_type_node,
+NULL_TREE);
gfc_define_builtin ("__builtin_realloc", ftype, BUILT_IN_REALLOC,
"realloc", false);
The return type is correct, void *, but the first argument should be
void * too and only second one size_t, while the above change changed
realloc to be void *__builtin_realloc (size_t, void *);
I went through all other changes from that commit and found that
__builtin_sincos{,f,l} got broken as well, instead of the former
void __builtin_sincos{,f,l} (ftype, ftype *, ftype *);
where ftype is {double,float,long double} it is now incorrectly
void __builtin_sincos{,f,l} (ftype *, ftype *);
The following patch fixes that, plus some formatting issues around
the spots I've changed.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2023-01-11 Jakub Jelinek
PR fortran/108349
* f95-lang.cc (gfc_init_builtin_function): Fix up function types
for BUILT_IN_REALLOC and BUILT_IN_SINCOS{F,,L}. Formatting fixes.
--- gcc/fortran/f95-lang.cc.jj 2022-11-15 22:57:18.247210671 +0100
+++ gcc/fortran/f95-lang.cc 2023-01-10 11:31:43.787266346 +0100
@@ -714,31 +714,34 @@ gfc_init_builtin_functions (void)
float_type_node, NULL_TREE);
func_cdouble_double = build_function_type_list (double_type_node,
- complex_double_type_node,
- NULL_TREE);
+ complex_double_type_node,
+ NULL_TREE);
func_double_cdouble = build_function_type_list (complex_double_type_node,
- double_type_node, NULL_TREE);
+ double_type_node, NULL_TREE);
- func_clongdouble_longdouble =
-build_function_type_list (long_double_type_node,
- complex_long_double_type_node, NULL_TREE);
-
- func_longdouble_clongdouble =
-build_function_type_list (complex_long_double_type_node,
- long_double_type_node, NULL_TREE);
+ func_clongdouble_longdouble
+= build_function_type_list (long_double_type_node,
+ complex_long_double_type_node, NULL_TREE);
+
+ func_longdouble_clongdouble
+= build_function_type_list (complex_long_double_type_node,
+ long_double_type_node, NULL_TREE);
ptype = build_pointer_type (float_type_node);
- func_float_floatp_floatp =
-build_function_type_list (void_type_node, ptype, ptype, NULL_TREE);
+ func_float_floatp_floatp
+= build_function_type_list (void_type_node, float_type_node, ptype, ptype,
+ NULL_TREE);
ptype = build_pointer_type (double_type_node);
- func_double_doublep_doublep =
-build_function_type_list (void_type_node, ptype, ptype, NULL_TREE);
+ func_double_doublep_doublep
+= build_function_type_list (void_type_node, double_type_node, ptype,
+ ptype, NULL_TREE);
ptype = build_pointer_type (long_double_type_node);
- func_longdouble_longdoublep_longdoublep =
-build_function_type_list (void_type_node, ptype, ptype, NULL_TREE);
+ func_longdouble_longdoublep_longdoublep
+= build_function_type_list (void_type_node, long_double_type_node, ptype,
+ ptype, NULL_TREE);
/* Non-math builtins are defined manually, so they're not included here. */
#define OTHER_BUILTIN(ID,NAME,TYPE,CONST)
@@ -992,9 +995,8 @@ gfc_init_builtin_functions (void)
"calloc", ATTR_NOTHROW_LEAF_MALLOC_LIST);
DECL_IS_MALLOC (builtin_decl_explicit (BUILT_IN_CALLOC)) = 1;
- ftype = build_function_type_list (pvoid_type_node,
-size_type_node, pvoid_type_node,
-NULL_TREE);
+ ftype = build_function_type_list (pvoid_type_node, pvoid_type_node,
+ size_type_node, NULL_TREE);
gfc_define_builtin ("__builtin_realloc", ftype, BUILT_IN_REALLOC,
"realloc", ATTR_NOTHROW_LEAF_LIST);
Jakub
[committed] testsuite: Fix up pr107397.f90 test [PR107397]
On Sat, Dec 17, 2022 at 09:12:43AM -0800, Jerry D via Gcc-patches wrote:
> The attached patch fixes a regression and is a patch from Steve. I have
> regression tested it and provided a test case. It is fairly simple and I
> will commit under the "simple" rule in a little while.
>
> Thanks Steve for Patch. Thanks Harald for helping me get back up to speed on
> the git magic.
The pr107397.f90 test FAILs for me, one problem was that the
added diagnostics has an indefinite article before BOZ, but
the test dg-error didn't. The other problem was that on the
other dg-error there was no space between the string and closing
}, so it was completely ignored and the error was an excess
error.
2022-12-19 Jakub Jelinek
PR fortran/107397
* gfortran.dg/pr107397.f90: Adjust expected diagnostic wording and
add space between dg-error string and closing }.
--- gcc/testsuite/gfortran.dg/pr107397.f90.jj 2022-12-19 11:09:13.793166473
+0100
+++ gcc/testsuite/gfortran.dg/pr107397.f90 2022-12-19 11:23:02.981322107
+0100
@@ -4,6 +4,6 @@ program p
type t
real :: a = 1.0
end type
- type(t), parameter :: x = z'1' ! { dg-error "incompatible with BOZ" }
- x%a = x%a + 2 ! { dg-error "has no IMPLICIT type"}
+ type(t), parameter :: x = z'1' ! { dg-error "incompatible with a BOZ" }
+ x%a = x%a + 2 ! { dg-error "has no IMPLICIT type" }
end
Jakub
Re: [Patch] Fortran/OpenMP: align/allocator modifiers to the allocate clause
On Fri, Dec 09, 2022 at 09:14:55PM +0100, Tobias Burnus wrote: > Implementing the 5.1 syntax inside the 'allocate' clause. That's a > fallout of working on something else... > > OK for mainline? > > 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 > Fortran/OpenMP: align/allocator modifiers to the allocate clause > > gcc/fortran/ChangeLog: > > * dump-parse-tree.cc (show_omp_namelist): Improve OMP_LIST_ALLOCATE > output. > * gfortran.h (struct gfc_omp_namelist): Add 'align' to 'u'. > (gfc_free_omp_namelist): Add bool arg. > * match.cc (gfc_free_omp_namelist): Likewise; free 'u.align'. > * openmp.cc (gfc_free_omp_clauses, gfc_match_omp_clause_reduction, > gfc_match_omp_flush): Update call. > (gfc_match_omp_clauses): Match 'align/allocate modifers in > 'allocate' clause. > (resolve_omp_clauses): Resolve align. > * st.cc (gfc_free_statement): Update call > * trans-openmp.cc (gfc_trans_omp_clauses): Handle 'align'. > > libgomp/ChangeLog: > > * libgomp.texi (5.1 Impl. Status): Split allocate clause/directive > item about 'align'; mark clause as 'Y' and directive as 'N'. > * testsuite/libgomp.fortran/allocate-2.f90: New test. > * testsuite/libgomp.fortran/allocate-3.f90: New test. LGTM, thanks. Jakub
Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++)
On Tue, Nov 08, 2022 at 02:36:17PM +, Julian Brown wrote:
> @@ -3258,6 +3273,7 @@ c_omp_address_inspector::get_origin (tree t)
> || TREE_CODE (t) == SAVE_EXPR)
> t = TREE_OPERAND (t, 0);
>else if (TREE_CODE (t) == INDIRECT_REF
> +&& TREE_TYPE (TREE_OPERAND (t, 0))
> && TREE_CODE (TREE_TYPE (TREE_OPERAND (t, 0))) == REFERENCE_TYPE)
> t = TREE_OPERAND (t, 0);
>else
> @@ -3274,7 +3290,9 @@ c_omp_address_inspector::get_origin (tree t)
> tree
> c_omp_address_inspector::maybe_unconvert_ref (tree t)
> {
> + /* The TREE_TYPE can be null if we had an earlier error. */
>if (TREE_CODE (t) == INDIRECT_REF
> + && TREE_TYPE (TREE_OPERAND (t, 0))
>&& TREE_CODE (TREE_TYPE (TREE_OPERAND (t, 0))) == REFERENCE_TYPE)
> return TREE_OPERAND (t, 0);
>
I'd prefer avoiding changes like the above.
If we had an earlier error, it should be error_mark_node or have
error_mark_node type, not NULL.
NULL type means something different in the C++ FE, in particular that
something is type dependent and the type will be only figured out after
instantiation.
Other than that LGTM.
Jakub
Re: GCC 13.0.0 Status Report (2022-11-14), Stage 3 in effect now
On Tue, Nov 15, 2022 at 01:49:36PM +0100, Martin Liška wrote: > On 11/15/22 11:07, Jakub Jelinek wrote: > > On Tue, Nov 15, 2022 at 11:02:53AM +0100, Martin Liška wrote: > >>> Is it allowed to merge libsanitizer from LLVM in stage 3? If not I'd > >>> like to cherry pick some commits from LLVM [to fix some stupid errors > >>> I've made in LoongArch libasan :(]. > >> > >> I'm sorry but I was really busy with the porting of the documentation to > >> Sphinx. > >> > >> Anyway, yes, we should make one one libsanitizer merge, but RM should > >> likely > >> approve it: Richi, Jakub, do you support it? > > > > Could you please prepare a patch, so that we can see how much actually > > changed and decide based on that whether to go for a merge or cherry-picking > > one or more commits? > > Sure, there it is. There's a minor change in output format that I address in > 0003 patch. > > Apart from that, I was able to run all tests on x86_64-linux-gnu. > Patch statistics: > 46 files changed, 524 insertions(+), 252 deletions(-) > > I'm running build on ppc64le and if you're fine, I'm going to finish > a proper libsanitizer testing procedure. Ok. Jakub
Re: GCC 13.0.0 Status Report (2022-11-14), Stage 3 in effect now
On Tue, Nov 15, 2022 at 11:02:53AM +0100, Martin Liška wrote: > > Is it allowed to merge libsanitizer from LLVM in stage 3? If not I'd > > like to cherry pick some commits from LLVM [to fix some stupid errors > > I've made in LoongArch libasan :(]. > > I'm sorry but I was really busy with the porting of the documentation to > Sphinx. > > Anyway, yes, we should make one one libsanitizer merge, but RM should likely > approve it: Richi, Jakub, do you support it? Could you please prepare a patch, so that we can see how much actually changed and decide based on that whether to go for a merge or cherry-picking one or more commits? I think last merge was done by you at the end of August, so we have 2.5 months of changes to potentially merge. Jakub
Re: [Patch] OpenMP/Fortran: 'target update' with DT components (was: [Patch] OpenMP/Fortran: 'target update' with strides + DT components)
On Thu, Nov 03, 2022 at 02:35:03PM +0100, Tobias Burnus wrote: > On 03.11.22 13:44, Jakub Jelinek wrote: > > [...] > > Otherwise LGTM, assuming it actually works correctly. > > > > I don't remember support for non-contiguous copying to/from devices > > being actually added, [...] And I think it is not ok to copy bytes > > that aren't requested to be copied. > > I have now removed that stride support and only kept the bug fix and the > DT component parts of the patch. > > The only code change is to remove the stride check disabling in > openmp.cc and in one testcase, to remove the stride part. > > I will commit it as attached, unless there are further comments (or the > just started reg testing shows that something does not work). > > Tobias > > PS: For strides, I now filed: PR middle-end/107517 "[OpenMP][5.0] > 'target update' with strides — for C/C++ and Fortran" > https://gcc.gnu.org/PR107517 > - > 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 > OpenMP/Fortran: 'target update' with DT components > > OpenMP 5.0 permits to use arrays with derived type components for the list > items to the 'from'/'to' clauses of the 'target update' directive. > > gcc/fortran/ChangeLog: > > * openmp.cc (gfc_match_omp_clauses): Permit derived types for > the 'to' and 'from' clauses of 'target update'. > * trans-openmp.cc (gfc_trans_omp_clauses): Fixes for > derived-type changes; fix size for scalars. > > libgomp/ChangeLog: > > * testsuite/libgomp.fortran/target-11.f90: New test. > * testsuite/libgomp.fortran/target-13.f90: New test. LGTM, thanks. Jakub
Re: [Patch] OpenMP/Fortran: 'target update' with strides + DT components
On Mon, Oct 31, 2022 at 03:46:25PM +0100, Tobias Burnus wrote: > OpenMP/Fortran: 'target update' with strides + DT components > > OpenMP 5.0 permits to use arrays with strides and derived > type components for the list items to the 'from'/'to' clauses > of the 'target update' directive. > > gcc/fortran/ChangeLog: > > * openmp.cc (gfc_match_omp_clauses): Permit derived types. > (resolve_omp_clauses):Accept noncontiguous > arrays. Formatting. Missing space before Accept and arrays. could fit on the same line. > * trans-openmp.cc (gfc_trans_omp_clauses): Fixes for > derived-type changes; fix size for scalars. > > libgomp/ChangeLog: > > * testsuite/libgomp.fortran/target-11.f90: New test. > * testsuite/libgomp.fortran/target-13.f90: New test. Otherwise LGTM, assuming it actually works correctly. I don't remember support for non-contiguous copying to/from devices being actually added, on the library side we certainly have omp_target_memcpy_rect which under the hood just does multiple copies of the contiguous subparts, but I don't remember something similar done in GOMP_target_update. And I think it is not ok to copy bytes that aren't requested to be copied. Jakub
Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++)
On Wed, Nov 02, 2022 at 12:20:11PM +, Julian Brown wrote: > > But we can't forbid lambdas inside of the map clause expressions, > > they are certainly valid in OpenMP, and IMNSHO shouldn't disallow > > statement expressions, people might not even know they use a > > statement expression, they could just use some standard macro which > > uses a statement expression under the hood. Though your testcases > > look good. > > I meant "forbid array sections within lambdas and statement > expressions" -- FAOD, does that seem reasonable? Technically it might Yeah, my response was to the wording you wrote above the patch, not what is inside of the patch which looked ok. > not be that hard to support e.g. a statement expression with an array > section on the final expression, but that doesn't work at the moment. And I think we want to keep it that way. Jakub
Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++)
Hi! Thanks for working on this! On Tue, Nov 01, 2022 at 09:50:38PM +, Julian Brown wrote: > > I think we should figure out when we should temporarily disable > > parser->omp_array_section_p = false; > > and restore it afterwards to a saved value. E.g. > > cp_parser_lambda_expression seems like a good candidate, the fact that > > OpenMP array sections are allowed say in map clause doesn't mean they > > are allowed inside of lambdas and it would be especially hard when > > the lambda is defining a separate function and the search for > > OMP_ARRAY_SECTION probably wouldn't be able to discover those. > > Other spots to consider might be statement expressions, perhaps type > > definitions etc. > > I've had a go at doing this -- several expression types now forbid > array-section syntax (see new "bad-array-section-*" tests added). I'm > afraid my C++ isn't quite up to figuring out how it's possible to > define a type inside an expression (inside a map clause) if we forbid > lambdas and statement expressions though -- can you give an example? But we can't forbid lambdas inside of the map clause expressions, they are certainly valid in OpenMP, and IMNSHO shouldn't disallow statement expressions, people might not even know they use a statement expression, they could just use some standard macro which uses a statement expression under the hood. Though your testcases look good. > > This shouldn't be done just for OMP_CLAUSE_MAP, but for all the > > other clauses that accept array sections, including > > OMP_CLAUSE_DEPEND, OMP_CLAUSE_AFFINITY, OMP_CLAUSE_MAP, OMP_CLAUSE_TO, > > OMP_CLAUSE_FROM, OMP_CLAUSE_INCLUSIVE, OMP_CLAUSE_EXCLUSIVE, > > OMP_CLAUSE_USE_DEVICE_ADDR, OMP_CLAUSE_HAS_DEVICE_ADDR, > > OMP_CLAUSE_*REDUCTION. > > I'm not too sure about all of those -- Tobias points out that > "INCLUSIVE", "EXCLUSIVE", *DEVICE* and *REDUCTION* take "variable list" > item types, not "locator list", though sometimes with an array section > being permitted (in OpenMP 5.2+). That is true. For the clauses that don't use locator lists but variable lists but accept array sections there are strict restrictions on what one can use, basically one can only have varname or varname[...] or varname[...][...] etc. where ... is the normal array element or array section syntax. So, we probably should continue to parse them as now, but we can use OMP_ARRAY_SECTION to hold what we've parsed or even share code with parsing array sections and the [...] on those clauses. > Tested (alongside next patch) with offloading to NVPTX -- with my > previously-posted "address tokenization" patch also applied. > 2022-11-01 Julian Brown > > gcc/c-family/ > * c-omp.cc (c_omp_address_inspector::map_supported_p): Handle > OMP_ARRAY_SECTION. > > gcc/cp/ > * constexpr.cc (potential_consant_expression_1): Handle > OMP_ARRAY_SECTION. > * error.cc (dump_expr): Handle OMP_ARRAY_SECTION. > * parser.cc (cp_parser_new): Initialize parser->omp_array_section_p. > (cp_parser_statement_expr): Disallow array sections. > (cp_parser_postfix_open_square_expression): Support OMP_ARRAY_SECTION > parsing. > (cp_parser_parenthesized_expression_list, cp_parser_lambda_expression, > cp_parser_braced_list): Disallow array sections. > (cp_parser_omp_var_list_no_open): Remove ALLOW_DEREF parameter, add > MAP_LVALUE in its place. Supported generalised lvalue parsing for > OpenMP map, to and from clauses. > (cp_parser_omp_var_list): Remove ALLOW_DEREF parameter, add > MAP_LVALUE. > Pass to cp_parser_omp_var_list_no_open. > (cp_parser_oacc_data_clause, cp_parser_omp_all_clauses): Update calls > to cp_parser_omp_var_list. > (cp_parser_omp_clause_map): Add sk_omp scope around > cp_parser_omp_var_list_no_open call. > * parser.h (cp_parser): Add omp_array_section_p field. > * semantics.cc (handle_omp_array_sections_1): Handle more types of map > expression. > (handle_omp_array_section): Handle non-DECL_P attachment points. > (finish_omp_clauses): Check for supported types of expression. > > gcc/ > * tree-pretty-print.c (dump_generic_node): Support OMP_ARRAY_SECTION. > * tree.def (OMP_ARRAY_SECTION): New tree code. > > gcc/testsuite/ > * c-c++-common/gomp/map-6.c: Update expected output. > * g++.dg/gomp/bad-array-section-1.C: New test. > * g++.dg/gomp/bad-array-section-2.C: New test. > * g++.dg/gomp/bad-array-section-3.C: New test. > * g++.dg/gomp/bad-array-section-4.C: New test. > * g++.dg/gomp/bad-array-section-5.C: New test. > * g++.dg/gomp/bad-array-section-6.C: New test. > * g++.dg/gomp/bad-array-section-7.C: New test. > * g++.dg/gomp/bad-array-section-8.C: New test. > * g++.dg/gomp/bad-array-section-9.C: New test. > * g++.dg/gomp/has_device_addr-non-lvalue-1.C: New test. > *
Re: [PATCH 2/5] [gfortran] Translate allocate directive (OpenMP 5.0).
On Tue, Oct 11, 2022 at 04:15:25PM +0200, Jakub Jelinek wrote: > Well, it can use a weak symbol, if not linked against libgomp, the bit > that it is OpenMP shouldn't be set and so realloc/free will be used > and do > if (arrdescr.gomp_alloced_bit) > GOMP_free (arrdescr.data, 0); > else > free (arrdescr.data); > and similar. And I think we can just document that we do this only for > -fopenmp compiled code. > But do we have a place to store that bit? I presume in array descriptors > there could be some bit for it, but what to do about scalar allocatables, > or allocatable components etc.? > In theory we could use ugly stuff like if all the allocations would be > guaranteed to have at least 2 byte alignment use LSB bit of the pointer > to mark GOMP_alloc allocated memory for the scalar allocatables etc. but > then would need in -fopenmp compiled code to strip it away. > > As for pinned memory, if it is allocated through libgomp allocators, that > should just work if GOMP_free/GOMP_realloc is used, that is why we have > those extra data in front of the allocations where we store everything we > need. But those also make the OpenMP allocations incompatible with > malloc/free allocations. Yet another option would be to change the way our OpenMP allocators work, instead of having allocation internal data before the allocated memory have them somewhere on the side and use some data structures mapping ranges of virtual memory to the allocation data. We'd either need to use mmap to have better control on where exactly we allocate stuff so that the on the side data structures wouldn't need to be for every allocation, or do those for every allocation perhaps with merging of adjacent allocations or something similar. Disadvantage is that it would be slower and might need more locking etc., advantage is that it could be then malloc/free compatible, any not tracked address would be forwarded from GOMP_free to free etc. And we'd not waste e.g. precious pinned etc. memory especially when doing allocations with very high alignment, where the data before allocation means we can waste up to max (32, alignment - 1) of extra memory. And gfortran inline emitted reallocation/deallocation could just emit GOMP_realloc/free always for -fopenmp. The way GOMP_ allocators are currently written, it is our internal choice if we do it the current way or the on the side way or some other way, but if we'd guarantee free compatibility we'd make it part of the ABI. CCing DJ and Carlos if they have thoughts about this. The OpenMP spec essentially requires that allocations through its allocator remember somewhere with which allocator (and its exact properties) each allocation has been done, so that it can be taken into account during reallocation or freeing. Jakub
Re: [PATCH 2/5] [gfortran] Translate allocate directive (OpenMP 5.0).
On Tue, Oct 11, 2022 at 03:22:02PM +0200, Tobias Burnus wrote:
> Hi Jakub,
>
> On 11.10.22 14:24, Jakub Jelinek wrote:
>
> There is another issue besides what I wrote in my last review,
> and I'm afraid I don't know what to do about it, hoping Tobias
> has some ideas.
> The problem is that without the allocate-stmt associated allocate directive,
> Fortran allocatables are easily always allocated with malloc and freed with
> free. The deallocation can be implicit through reallocation, or explicit
> deallocate statement etc.
> ...
> But when some allocatables are now allocated with a different
> allocator (when allocate-stmt associated allocate directive is used),
> some allocatables are allocated with malloc and others with GOMP_alloc
> but we need to free them with the corresponding allocator based on how
> they were allocated, what has been allocated with malloc should be
> deallocated with free, what has been allocated with GOMP_alloc should be
> deallocated with GOMP_free.
>
>
>
> I think the most common case is:
>
> integer, allocatable :: var(:)
> !$omp allocators allocator(my_alloc) ! must be in same scope as decl of 'var'
> ...
> ! optionally: deallocate(var)
> end ! of scope: block/subroutine/... - automatic deallocation
So you talk here about the declarative directive the patch does sorry on,
or about the executable one above allocate stmt?
Anyway, even this simple case has the problem that one can have
subroutine foo (var)
integer, allocatable:: var(:)
var = [1, 2, 3] ! reallocate
end subroutine
and call foo (var) above.
> Those can be easily handled. It gets more complicated with control flow:
>
> if (...) then
> !$omp allocators allocator(...)
> allocate(...)
> else
> allocate (...)
> endif
>
>
>
> However, the problem is really that there is is no mandatory
> '!$omp deallocators' and also the wording like:
>
> "If any operation of the base language causes a reallocation of
> an array that is allocated with a memory allocator then that
> memory allocator will be used to release the current memory
> and to allocate the new memory." (OpenMP 5.0 wording)
>
> There has been some attempt to relax the rules a bit, e.g. by
> adding the wording:
> "For allocated allocatable components of such variables, the allocator that
> will be used for the deallocation and allocation is unspecified."
>
> And some wording change (→issues 3189) to clarify related component issues.
>
> But nonetheless, there is still the issue of:
>
> (a) explicit DEALLOCATE in some other translation unit
> (b) some intrinsic operation which reallocate the memory, either via libgomp
> or in the source code:
> a = [1,2,3] ! possibly reallocates
> str = trim(str) ! possibly reallocates
> where the first one calls 'realloc' directly in the code and the second one
> calls 'libgomp' for that.
>
> * * *
>
> I don't see a good solution – and there is in principle the same issue with
> unified-shared memory (USM) on hardware that does not support transparently
> accessing all host memory on the device.
>
> Compilers support this case by allocating memory in some special memory,
> which is either accessible from both sides ('pinned') or migrates on the
> first access from the device side - but remains there until the accessing
> device kernel ends ('managed memory').
>
> Newer hardware (+ associated Linux kernel support) permit accessing all
> memory in a somewhat fast way, avoiding this issue (and special handling
> is then left to the user.) For AMDGCN, my understanding is that all hardware
> supported by GCC supports this - but glacial speed until the last hardware
> architectures. For Nvidia, this is supported since Pascal (I think for Titan
> X,
> P100, i.e. sm_5.2/sm_60) - but I believe not for all Pascal/Kepler hardware.
>
> I mention this because the USM implementation at
> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597976.html
> suffers from this.
> And https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601059.html
> tries to solve the the 'trim' example issue above - i.e. the case where
> libgomp reallocates pinned/managed (pseudo-)USM memory.
>
> * * *
>
> The deallocation can be done in a completely different TU from where it has
> been allocated, in theory it could be also not compiled with -fopenmp, etc.
> So, I'm afraid we need to store somewhere whether we used malloc or
> GOMP_alloc for the allocation (say somewhere in the array descriptor and for
> other stuff somewhere on the side?) and slow down all code that needs
> deallocation to check that bit (or say we don't support
> deallocation/reallocation of OpenMP allocated allocatables without -fopenmp
>
Re: [PATCH 2/5] [gfortran] Translate allocate directive (OpenMP 5.0).
On Thu, Jan 13, 2022 at 02:53:17PM +, Hafiz Abid Qadeer wrote: > gcc/fortran/ChangeLog: > > * trans-openmp.c (gfc_trans_omp_clauses): Handle OMP_LIST_ALLOCATOR. > (gfc_trans_omp_allocate): New function. > (gfc_trans_omp_directive): Handle EXEC_OMP_ALLOCATE. > > gcc/ChangeLog: > > * tree-pretty-print.c (dump_omp_clause): Handle OMP_CLAUSE_ALLOCATOR. > (dump_generic_node): Handle OMP_ALLOCATE. > * tree.def (OMP_ALLOCATE): New. > * tree.h (OMP_ALLOCATE_CLAUSES): Likewise. > (OMP_ALLOCATE_DECL): Likewise. > (OMP_ALLOCATE_ALLOCATOR): Likewise. > * tree.c (omp_clause_num_ops): Add entry for OMP_CLAUSE_ALLOCATOR. > > gcc/testsuite/ChangeLog: > > * gfortran.dg/gomp/allocate-6.f90: New test. There is another issue besides what I wrote in my last review, and I'm afraid I don't know what to do about it, hoping Tobias has some ideas. The problem is that without the allocate-stmt associated allocate directive, Fortran allocatables are easily always allocated with malloc and freed with free. The deallocation can be implicit through reallocation, or explicit deallocate statement etc. But when some allocatables are now allocated with a different allocator (when allocate-stmt associated allocate directive is used), some allocatables are allocated with malloc and others with GOMP_alloc but we need to free them with the corresponding allocator based on how they were allocated, what has been allocated with malloc should be deallocated with free, what has been allocated with GOMP_alloc should be deallocated with GOMP_free. The deallocation can be done in a completely different TU from where it has been allocated, in theory it could be also not compiled with -fopenmp, etc. So, I'm afraid we need to store somewhere whether we used malloc or GOMP_alloc for the allocation (say somewhere in the array descriptor and for other stuff somewhere on the side?) and slow down all code that needs deallocation to check that bit (or say we don't support deallocation/reallocation of OpenMP allocated allocatables without -fopenmp on the deallocation TU and only slow down -fopenmp compiled code)? Tobias, thoughts on this? Jakub
Re: [PATCH 1/5] [gfortran] Add parsing support for allocate directive (OpenMP 5.0).
On Thu, Jan 13, 2022 at 02:53:16PM +, Hafiz Abid Qadeer wrote: > Currently we only make use of this directive when it is associated > with an allocate statement. Sorry for the delay. I'll start with a comment that allocate directive in 5.0/5.1 for Fortran is a complete mess that has been fixed only in 5.2 by splitting the directive into the allocators construct and allocate directive. The problem with 5.0/5.1 is that it is just ambiguous whether !$omp allocate (list) optional-clauses is associated with an allocate statement or not. When it is not associated with allocate statement, it is a declarative directive that should appear only in the specification part, when it is associated with a allocate stmt, it should appear only in the executable part. And a mess starts when it is on the boundary between the two. Now, how exactly to differentiate between the 2 I'm afraid depends on the exact OpenMP version. 1) if we are p->state == ORDER_EXEC already, it must be associated with allocate-stmt (and we should error whenever it violates restrictions for those) 2) if (list) is missing, it must be associated with allocate-stmt 3) for 5.0 only, if allocator clause isn't specified, it must be not associated with allocate-stmt, but in 5.1 the clauses are optional also for one associated with it; if align clause is specified, it must be 5.1 4) all the allocate directives after one that must be associated with allocate-stmt must be also associated with allocate-stmt 5) if variables in list are allocatable, it must be associated with allocate-stmt, if they aren't allocatable, it must not be associated with allocate-stmt In your patch, you put ST_OMP_ALLOCATE into case_executable define, I'm afraid due to the above we need to handle ST_OMP_ALLOCATE manually whenever case_executable/case_omp_decl appear in parse.cc and be prepared that it could be either declarative directive or executable construct and resolve based on the 1-5 above into which category it belongs (either during parsing or during resolving). And certainly have testsuite coverage for cases like: integer :: i, j integer, allocatable :: k(:), l(:) !$omp allocate (i) allocator (alloc1) !$omp allocate (j) allocator (alloc2) !$omp allocate (k) allocator (alloc3) !$omp allocate (l) allocator (alloc4) allocate (k(14), l(23)) where I think the first 2 are declarative directives and the last 2 bind to allocate-stmt (etc., cover all the cases mentioned above). On the other side, 5.1 has: "An allocate directive that is associated with an allocate-stmt and specifies a list must be preceded by an executable statement or OpenMP construct." restriction, so if we implement that, the ambiguity decreases. We wouldn't need to worry about 3) and 5), would decide on 1) and 2) and 4) only. > gcc/fortran/ChangeLog: > > * dump-parse-tree.c (show_omp_node): Handle EXEC_OMP_ALLOCATE. > (show_code_node): Likewise. > * gfortran.h (enum gfc_statement): Add ST_OMP_ALLOCATE. > (OMP_LIST_ALLOCATOR): New enum value. > (enum gfc_exec_op): Add EXEC_OMP_ALLOCATE. > * match.h (gfc_match_omp_allocate): New function. > * openmp.c (enum omp_mask1): Add OMP_CLAUSE_ALLOCATOR. > (OMP_ALLOCATE_CLAUSES): New define. > (gfc_match_omp_allocate): New function. > (resolve_omp_clauses): Add ALLOCATOR in clause_names. > (omp_code_to_statement): Handle EXEC_OMP_ALLOCATE. > (EMPTY_VAR_LIST): New define. > (check_allocate_directive_restrictions): New function. > (gfc_resolve_omp_allocate): Likewise. > (gfc_resolve_omp_directive): Handle EXEC_OMP_ALLOCATE. > * parse.c (decode_omp_directive): Handle ST_OMP_ALLOCATE. > (next_statement): Likewise. You didn't change next_statement, but case_executable macro. But see above. > (gfc_ascii_statement): Likewise. > * resolve.c (gfc_resolve_code): Handle EXEC_OMP_ALLOCATE. > * st.c (gfc_free_statement): Likewise. > * trans.c (trans_code): Likewise > > gcc/testsuite/ChangeLog: > > * gfortran.dg/gomp/allocate-4.f90: New test. > * gfortran.dg/gomp/allocate-5.f90: New test. > --- > --- a/gcc/fortran/openmp.c > +++ b/gcc/fortran/openmp.c > @@ -921,6 +921,7 @@ enum omp_mask1 >OMP_CLAUSE_FAIL, /* OpenMP 5.1. */ >OMP_CLAUSE_WEAK, /* OpenMP 5.1. */ >OMP_CLAUSE_NOWAIT, > + OMP_CLAUSE_ALLOCATOR, I don't see how can you add OMP_CLAUSE_ALLOCATOR to enum omp_mask1. OMP_MASK1_LAST is already 64, so I think the gcc_checking_assert (OMP_MASK1_LAST <= 64 && OMP_MASK2_LAST <= 64); assertion would fail. OMP_MASK2_LAST is on the other side just 30, and allocate directive takes just allocator or in 5.1 align clauses, so both should go to the enum omp_mask2 block. And for newly added clauses, we add the /* OpenMP 5.0. */ etc. comments when the clause appeared first (5.0 for allocator, 5.1 for align). >/* This must come last. */ >OMP_MASK1_LAST > }; > @@ -3568,6 +3569,7 @@
Re: [PATCH v4 4/4] OpenMP/OpenACC: Unordered/non-constant component offset struct mapping
On Sun, Oct 09, 2022 at 02:51:37PM -0700, Julian Brown wrote:
> This patch adds support for non-constant component offsets in "map"
> clauses for OpenMP (and the equivalants for OpenACC), which are not able
> to be sorted into order at compile time. Normally struct accesses in
> such clauses are gathered together and sorted into increasing address
> order after a "GOMP_MAP_STRUCT" node: if we have variable indices,
> that is no longer possible.
>
> This patch adds support for such mappings by adding a new variant of
> GOMP_MAP_STRUCT that does not require the list of nodes following to
> be in sorted order. This passes right down to the runtime: the list is
> sorted in libgomp according to the dynamic values of the offsets after
> the newly-introduced GOMP_MAP_STRUCT_UNORD node.
>
> This mostly affects arrays of structs indexed by variables in C and C++,
> but can also affect derived-type arrays with constant indexes when they
> have an array descriptor.
I don't understand why this is needed.
We have a restriction that ought to make all such cases invalid:
"If multiple list items are explicitly mapped on the same construct and have
the same containing
array or have base pointers that share original storage, and if any of the list
items do not have
corresponding list items that are present in the device data environment prior
to a task
encountering the construct, then the list items must refer to the same array
elements of either the
containing array or the implicit array of the base pointers."
So, all those
#pragma omp target map(t->a[i].p, t->a[j].p) etc. cases are invalid unless
i == j, so IMNSHO one doesn't need to worry about unordered cases.
> +#if defined(_GNU_SOURCE) || defined(__GNUC__)
> +static int
> +compare_addr_r (const void *a, const void *b, void *data)
> +{
> + void **hostaddrs = (void **) data;
> + int ai = *(int *) a, bi = *(int *) b;
> + if (hostaddrs[ai] < hostaddrs[bi])
> +return -1;
> + else if (hostaddrs[ai] > hostaddrs[bi])
> +return 1;
> + return 0;
> +}
> +#endif
Note, not all glibcs have qsort_r and _GNU_SOURCE macro being defined
doesn't imply glibc. You'd need something like _GLIBC_PREREQ macro
and require 2.8 or later.
> +
> static inline __attribute__((always_inline)) struct target_mem_desc *
> gomp_map_vars_internal (struct gomp_device_descr *devicep,
> struct goacc_asyncqueue *aq, size_t mapnum,
> @@ -968,6 +982,17 @@ gomp_map_vars_internal (struct gomp_device_descr
> *devicep,
>tgt->device_descr = devicep;
>tgt->prev = NULL;
>struct gomp_coalesce_buf cbuf, *cbufp = NULL;
> + size_t hostaddr_idx;
> +
> +#if !defined(_GNU_SOURCE) && defined(__GNUC__)
> + /* If we don't have _GNU_SOURCE (thus no qsort_r), but we are compiling
> with
> + GCC (and why wouldn't we be?), we can use this nested function for
> + regular qsort. */
> + int compare_addr (const void *a, const void *b)
> +{
> + return compare_addr_r (a, b, (void *) [hostaddr_idx]);
> +}
> +#endif
Please don't use nested functions in libgomp.
> + int *order = NULL;
> + if ((kind & typemask) == GOMP_MAP_STRUCT_UNORD)
> + {
> + order = (int *) gomp_alloca (sizeof (int) * sizes[i]);
> + for (int j = 0; j < sizes[i]; j++)
> + order[j] = j;
> +#ifdef _GNU_SOURCE
> + qsort_r (order, sizes[i], sizeof (int), _addr_r,
> +[i + 1]);
> +#elif defined(__GNUC__)
> + hostaddr_idx = i + 1;
> + qsort (order, sizes[i], sizeof (int), _addr);
> +#else
> +#error no threadsafe qsort
> +#endif
This is too ugly. If this is really needed (see above) and
you need fallback for missing qsort_t, better sort array of tuples
containing the order number and some data pointer needed for the comparison
routine.
Jakub
[committed] openmp, fortran: Fix up IFN_ASSUME call
On Thu, Oct 06, 2022 at 06:15:52PM +0200, Tobias Burnus wrote:
> Like as attached? – It did survive regtesting.
Like in other spots in trans-openmp.cc that create a TARGET_EXPR, the
slot has to be created with create_tmp_var_raw, because gfc_create_var
adds the var to BLOCK_VARS and that ICEs during expansion because
gimple_add_tmp_var_fn has:
gcc_assert (!DECL_CHAIN (tmp) && !DECL_SEEN_IN_BIND_EXPR_P (tmp));
assertion. Also, both C/C++ ensure the argument to IFN_ASSUME has
boolean_type_node, it is easier if Fortran does that too.
Bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk.
2022-10-10 Jakub Jelinek
* trans-openmp.cc (gfc_trans_omp_assume): Use create_tmp_var_raw
instead of gfc_create_var for TARGET_EXPR slot creation. Create it
with boolean_type_node and convert.
--- gcc/fortran/trans-openmp.cc.jj 2022-10-07 00:13:12.508191601 +0200
+++ gcc/fortran/trans-openmp.cc 2022-10-09 14:17:55.430364168 +0200
@@ -4588,11 +4588,14 @@ gfc_trans_omp_assume (gfc_code *code)
t = se.expr;
else
{
- tree var = gfc_create_var (TREE_TYPE (se.expr), NULL);
+ tree var = create_tmp_var_raw (boolean_type_node);
+ DECL_CONTEXT (var) = current_function_decl;
stmtblock_t block2;
gfc_init_block ();
gfc_add_block_to_block (, );
- gfc_add_modify_loc (loc, , var, se.expr);
+ gfc_add_modify_loc (loc, , var,
+ fold_convert_loc (loc, boolean_type_node,
+ se.expr));
gfc_add_block_to_block (, );
t = gfc_finish_block ();
t = build4 (TARGET_EXPR, boolean_type_node, var, t, NULL, NULL);
Jakub
Re: [Patch] openmp: Map holds clause to IFN_ASSUME for Fortran
On Thu, Oct 06, 2022 at 06:15:52PM +0200, Tobias Burnus wrote:
> On 06.10.22 14:17, Jakub Jelinek wrote:
> > On Thu, Oct 06, 2022 at 12:55:01PM +0200, Tobias Burnus wrote:
> > > I don't know whether it makes sense to handle – in the long run – the
> > > case of se.pre/se.post being nonempty – and, if so, how.
> > I think it is essential not to throw those away,
> > if se.pre or se.post, you can e.g. expand it roughly as C/C++ ({ cond; }),
> > in GENERIC it can be say a TARGET_EXPR with a boolean
> > temporary as slot, where the the initializer will be the
> > se.pre part, followed by MODIFY_EXPR which sets the slot to se.expr
> > value and followed by se.post.
>
> Like as attached? – It did survive regtesting.
LGTM, thanks.
> BTW: The assumption in assume-4.f90 does not help, but I think that's
> expected. I wonder whether it will work in both cases after your
> gimplify work.
Well, gimplify + gimple-lower but more importantly ranger work later on,
at least that's the hope...
Jakub
Re: [Patch] openmp: Map holds clause to IFN_ASSUME for Fortran
On Thu, Oct 06, 2022 at 12:55:01PM +0200, Tobias Burnus wrote:
> Same as for C/C++, albeit a tiny bit longer patch.
>
> I don't know whether it makes sense to handle – in the long run – the
> case of se.pre/se.post being nonempty – and, if so, how.
I think it is essential not to throw those away,
if se.pre or se.post, you can e.g. expand it roughly as C/C++ ({ cond; }),
in GENERIC it can be say a TARGET_EXPR with a boolean
temporary as slot, where the the initializer will be the
se.pre part, followed by MODIFY_EXPR which sets the slot to se.expr
value and followed by se.post.
I've only started playing with the middle-end changes now, here
is what I have and plan to at lower_cf time turn that into essentially
bool artificial (args...)
{
return cond;
}
and
.ASSUME (, args...);
--- gcc/gimplify.cc.jj 2022-10-06 08:56:28.344131629 +0200
+++ gcc/gimplify.cc 2022-10-06 14:04:46.647204910 +0200
@@ -3569,7 +3569,45 @@ gimplify_call_expr (tree *expr_p, gimple
fndecl, 0));
return GS_OK;
}
- /* FIXME: Otherwise expand it specially. */
+ /* Temporarily, until gimple lowering, transform
+.ASSUME (cond);
+into:
+guard = .ASSUME ();
+if (guard) goto label_true; else label_false;
+label_true:;
+{
+ guard = cond;
+}
+label_false:;
+.ASSUME (guard);
+such that gimple lowering can outline the condition into
+a separate function easily. */
+ tree guard = create_tmp_var (boolean_type_node);
+ gcall *call = gimple_build_call_internal (ifn, 0);
+ gimple_call_set_nothrow (call, TREE_NOTHROW (*expr_p));
+ gimple_set_location (call, loc);
+ gimple_call_set_lhs (call, guard);
+ gimple_seq_add_stmt (pre_p, call);
+ *expr_p = build2 (MODIFY_EXPR, void_type_node, guard,
+ CALL_EXPR_ARG (*expr_p, 0));
+ *expr_p = build3 (BIND_EXPR, void_type_node, NULL, *expr_p, NULL);
+ tree label_false = create_artificial_label (UNKNOWN_LOCATION);
+ tree label_true = create_artificial_label (UNKNOWN_LOCATION);
+ gcond *cond_stmt = gimple_build_cond (NE_EXPR, guard,
+ boolean_false_node,
+ label_true, label_false);
+ gimplify_seq_add_stmt (pre_p, cond_stmt);
+ gimplify_seq_add_stmt (pre_p, gimple_build_label (label_true));
+ push_gimplify_context ();
+ gimple_seq body = NULL;
+ gimple *g = gimplify_and_return_first (*expr_p, );
+ pop_gimplify_context (g);
+ gimplify_seq_add_seq (pre_p, body);
+ gimplify_seq_add_stmt (pre_p, gimple_build_label (label_false));
+ call = gimple_build_call_internal (ifn, 1, guard);
+ gimple_call_set_nothrow (call, TREE_NOTHROW (*expr_p));
+ gimple_set_location (call, loc);
+ gimple_seq_add_stmt (pre_p, call);
return GS_ALL_DONE;
}
--- gcc/cp/pt.cc.jj 2022-10-06 08:56:28.670127213 +0200
+++ gcc/cp/pt.cc2022-10-06 13:42:26.632351930 +0200
@@ -21182,6 +21182,8 @@ tsubst_copy_and_build (tree t,
ret = error_mark_node;
break;
}
+ if (!processing_template_decl)
+ arg = fold_build_cleanup_point_expr (TREE_TYPE (arg), arg);
ret = build_call_expr_internal_loc (EXPR_LOCATION (t),
IFN_ASSUME,
void_type_node, 1,
--- gcc/cp/cp-gimplify.cc.jj2022-10-06 08:56:28.660127349 +0200
+++ gcc/cp/cp-gimplify.cc 2022-10-06 13:41:54.286789968 +0200
@@ -3117,6 +3117,8 @@ process_stmt_assume_attribute (tree std_
arg = contextual_conv_bool (arg, tf_warning_or_error);
if (error_operand_p (arg))
continue;
+ if (!processing_template_decl)
+ arg = fold_build_cleanup_point_expr (TREE_TYPE (arg), arg);
statement = build_call_expr_internal_loc (attrs_loc, IFN_ASSUME,
void_type_node, 1, arg);
finish_expr_stmt (statement);
--- gcc/cp/parser.cc.jj 2022-10-06 10:39:31.989345921 +0200
+++ gcc/cp/parser.cc2022-10-06 13:41:28.001145938 +0200
@@ -46029,6 +46029,8 @@ cp_parser_omp_assumption_clauses (cp_par
t = contextual_conv_bool (t, tf_warning_or_error);
if (is_assume && !error_operand_p (t))
{
+ if (!processing_template_decl)
+ t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
t = build_call_expr_internal_loc (eloc, IFN_ASSUME,
void_type_node, 1, t);
Re: [Patch] Fortran: Add OpenMP's assume(s) directives
On Wed, Oct 05, 2022 at 02:29:56PM +0200, Tobias Burnus wrote:
> + gfc_error ("!OMP ASSUMES at %C must be in the specification part of a "
s/!OMP/!$OMP/
Otherwise LGTM.
Jakub
Re: [Patch] Fortran: Add OpenMP's assume(s) directives
On Tue, Oct 04, 2022 at 02:26:13PM +0200, Tobias Burnus wrote:
> Hi Jakub,
>
> On 04.10.22 12:19, Jakub Jelinek wrote:
>
> On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote:
>
>
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -2749,9 +2749,9 @@ have support for @option{-pthread}. @option{-fopenmp}
> implies
> @opindex fopenmp-simd
> @cindex OpenMP SIMD
> @cindex SIMD
> -Enable handling of OpenMP's SIMD directives with @code{#pragma omp}
> -in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives
> -are ignored.
> +Enable handling of OpenMP's SIMD directives and OPENMP's @code{assume}
> directive
>
>
> s/OPENMP/OpenMP/
>
> We actually handle more directives, @code{declare reduction},
> @code{ordered}, @code{scan}, @code{loop} and combined/composite directives
> with @code{simd} as constituent.
> ...
> And now in C++ we handle also the attribute syntax (guess we should update
> the text for that here as well as in -fopenmp entry).
>
> Updated suggestion attached – I still need to update the main patch.
>
> (I also added 'declare simd' to the list. And I updated Fortran for scan +
> loop.)
>
> OK?
Ok, thanks.
> Wouldn't this be better table driven (like c_omp_directives
> in c-family, though guess for Fortran you can just use spaces
> in the name, don't need 3 strings for the separate tokens)?
> Because I think absent/contains isn't the only spot where
> you need directive names, metadirective is another.
>
> Maybe – I think there are already way to many string repetitions. One problem
> is that metadirectives permit combined/composite constructs while 'assume(s)'
> does not. I on purpose did not parse them, but probably in light of
> Metadirectives, I should.
>
> I will take a look.
It is true that metadirective supports combined/composite constructs,
and so do we in the C++ attribute case, still we IMHO can use the C/C++
table as is.and it doesn't need to include combined/composite constructs.
The thing is that for the metadirective/C++ attribute case, all we need to
know is to discover the directive category (declarative, stand-alone,
construct, informational, ...) and for that it is enough to parse the
first directive-name in combined/composite constructs. Both metadirectives
and C++ attributes then have the name of the directive followed by clauses
so we effectively have to use the normal parsing of directives/clauses
there (except perhaps not end on end of directive but on unbalanced closing
paren). And then there is the absent/contains case, where we only
allow non-combined/composite, so there we need to try to match the directive
name from the table and make sure it is followed by , or ).
> But also, resizing each time a single entry is added to the list isn't
> good for compile time, would be nice to grow the allocation size in
> powers of 2 or so.
>
> I only expect a very small number – and did not want to keep track of yet
> another number.
>
> However, the following should work:
>
>
> if (old_n_absent = 0)
>absent = ... sizeof() * 1
> else if (popcount (old_n_absent) == 1)
>absent = ... sizeof() * (old_n_absent) * 2)
Yeah. Or for 0 allocate say 8 and
use (pow2p_hwi (old_n_absent) && old_n_absent >= 8)
in the else if.
> that allocates: 1, 2, 4, 8, 16, 32, ... without keeping track of the number.
>
>
>
> +gfc_match_omp_assumes (void)
> +{
> + locus loc = gfc_current_locus;
> + gfc_omp_clauses *c = gfc_get_omp_clauses ();
> + c->assume = gfc_current_ns->omp_assumes;
> + if (!gfc_current_ns->proc_name
> + || (gfc_current_ns->proc_name->attr.flavor != FL_MODULE
> + && !gfc_current_ns->proc_name->attr.subroutine
> + && !gfc_current_ns->proc_name->attr.function))
> +{
> + gfc_error ("!OMP ASSUMES at %C must be in the specification part of a "
> +"subprogram or module");
> + return MATCH_ERROR;
> +}
> + if (gfc_match_omp_clauses (, omp_mask (OMP_CLAUSE_ASSUMPTIONS), true,
> true,
> +false, false, false, false) != MATCH_YES)
> +{
> + gfc_current_ns->omp_assumes = NULL;
> + return MATCH_ERROR;
> +}
>
>
>
> I don't understand the point of preallocation of gfc_omp_clauses here,
> can't it be allocated inside of gfc_match_omp_clauses like everywhere else?
> Because otherwise it e.g. leaks if the first error is reported.
>
> This is supposed to handle:
> subroutine foo()
>!$omp assumes absent(target)
>!$omp assumes absent(teams)
> end
>
> I did not spot anything which states that it is invalid.
> (I might have missed it, however.) And if
Re: [Patch] Fortran: Add OpenMP's assume(s) directives
On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote:
> gcc/ChangeLog:
>
> * doc/invoke.texi (-fopenmp-simd): Document that also 'assume'
> is enabled.
>
> libgomp/ChangeLog:
>
> * libgomp.texi (OpenMP 5.1 Impl. Status): Mark 'assume' as 'Y'.
>
> gcc/fortran/ChangeLog:
>
> * dump-parse-tree.cc (show_omp_assumes): New.
> (show_omp_clauses, show_namespace): Call it.
> (show_omp_node, show_code_node): Handle OpenMP ASSUME.
> * gfortran.h (enum gfc_statement): Add ST_OMP_ASSUME,
> ST_OMP_END_ASSUME and ST_OMP_ASSUMES.
> (gfc_exec_op): Add EXEC_OMP_ASSUME.
> (gfc_omp_assumptions): New struct.
> (gfc_get_omp_assumptions): New XCNEW #define.
> (gfc_omp_clauses, gfc_namespace): Add assume member.
> (gfc_resolve_omp_assumptions): New prototype.
> * match.h (gfc_match_omp_assume, gfc_match_omp_assumes): New.
> * openmp.cc (omp_code_to_statement): Declare.
> (gfc_free_omp_clauses): Free assume member and its struct data.
> (enum omp_mask2): Add OMP_CLAUSE_ASSUMPTIONS.
> (gfc_omp_absent_contains_clause): New.
> (gfc_match_omp_clauses): Call it; optionally use passed
> omp_clauses argument.
> (gfc_match_omp_assume, gfc_match_omp_assumes): New.
> (gfc_resolve_omp_assumptions): New.
> (resolve_omp_clauses): Call it.
> (gfc_resolve_omp_directive, omp_code_to_statement): Handle
> EXEC_OMP_ASSUME.
> * parse.cc (decode_omp_directive): Parse OpenMP ASSUME(S).
> (next_statement, parse_executable, parse_omp_structured_block):
> Handle ST_OMP_ASSUME.
> (case_omp_decl): Add ST_OMP_ASSUMES.
> (gfc_ascii_statement): Handle Assumes, optional return
> string without '!$OMP '/'!$ACC ' prefix.
> (is_omp_declarative_stmt, is_omp_informational_stmt): New.
> * parse.h (gfc_ascii_statement): Add optional bool arg to prototype.
> (is_omp_declarative_stmt, is_omp_informational_stmt): New prototype.
> * resolve.cc (gfc_resolve_blocks, gfc_resolve_code): Add
> EXEC_OMP_ASSUME.
> (gfc_resolve): Resolve ASSUMES directive.
> * symbol.cc (gfc_free_namespace): Free omp_assumes member.
> * st.cc (gfc_free_statement): Handle EXEC_OMP_ASSUME.
> * trans-openmp.cc (gfc_trans_omp_directive): Likewise.
> * trans.cc (trans_code): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/gomp/assume-1.f90: New test.
> * gfortran.dg/gomp/assume-2.f90: New test.
> * gfortran.dg/gomp/assumes-1.f90: New test.
> * gfortran.dg/gomp/assumes-2.f90: New test.
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -2749,9 +2749,9 @@ have support for @option{-pthread}. @option{-fopenmp}
> implies
> @opindex fopenmp-simd
> @cindex OpenMP SIMD
> @cindex SIMD
> -Enable handling of OpenMP's SIMD directives with @code{#pragma omp}
> -in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives
> -are ignored.
> +Enable handling of OpenMP's SIMD directives and OPENMP's @code{assume}
> directive
s/OPENMP/OpenMP/
We actually handle more directives, @code{declare reduction},
@code{ordered}, @code{scan}, @code{loop} and combined/composite directives
with @code{simd} as constituent.
> +with @code{#pragma omp} in C/C++ and @code{!$omp} in Fortran. Other OpenMP
> +directives are ignored.
And now in C++ we handle also the attribute syntax (guess we should update
the text for that here as well as in -fopenmp entry).
> @@ -3531,6 +3565,14 @@ show_namespace (gfc_namespace *ns)
> }
> }
>
> + if (ns->omp_assumes)
> +{
> + show_indent ();
> + fprintf (dumpfile, "!$OMP ASSUMES");
> + show_omp_assumes (ns->omp_assumes);
> +}
> +
> +
Just one empty line?
>fputc ('\n', dumpfile);
>show_indent ();
>fputs ("code:", dumpfile);
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index 4babd77924b..29a443dcd44 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -316,7 +316,7 @@ enum gfc_statement
>ST_OMP_END_PARALLEL_MASKED_TASKLOOP_SIMD, ST_OMP_MASKED_TASKLOOP,
>ST_OMP_END_MASKED_TASKLOOP, ST_OMP_MASKED_TASKLOOP_SIMD,
>ST_OMP_END_MASKED_TASKLOOP_SIMD, ST_OMP_SCOPE, ST_OMP_END_SCOPE,
> - ST_OMP_ERROR, ST_NONE
> + ST_OMP_ERROR, ST_OMP_ASSUME, ST_OMP_END_ASSUME, ST_OMP_ASSUMES, ST_NONE
> };
>
> /* Types of interfaces that we can have. Assignment interfaces are
> @@ -1506,6 +1506,19 @@ enum gfc_omp_bind_type
>OMP_BIND_THREAD
> };
>
> +typedef struct gfc_omp_assumptions
> +{
> + int n_absent, n_contains;
> + enum gfc_statement *absent, *contains;
> + gfc_expr_list *holds;
> + locus where;
> + bool no_openmp:1, no_openmp_routines:1, no_parallelism:1;
> +}
> +gfc_omp_assumptions;
> +
> +#define gfc_get_omp_assumptions() XCNEW (gfc_omp_assumptions)
> +
> +
> typedef struct gfc_omp_clauses
> {
>gfc_omp_namelist *lists[OMP_LIST_NUM];
> @@ -1529,6 +1542,7 @@ typedef struct
Re: [Patch] Fortran: Update use_device_ptr for OpenMP 5.1 [PR105318]
On Fri, Sep 30, 2022 at 12:41:19PM +0200, Tobias Burnus wrote:
> While has_device_addr has been implemented (in GCC 12), updating
> use_device_ptr for Fortran was missed.
>
> This patch fixes it: Removing the restrictions and mapping to
> has_device_addr where applicable.
>
> For use_device_ptr something similar was done, albeit I think
> this has no semantic effect.
>
> And 'device(omp_initial_device)' printed a warning in Fortran.
> (BTW: C/C++ silently accepts any negative value.)
I think that is what the standard wants.
E.g. in 5.2 device Clause chapter, there is just
"If the device_num device-modifier is specified and target-offload-var is not
mandatory,
device-description must evaluate to a conforming device number."
restriction, which is something that can't be checked at compile time,
you don't know if target-offload-var is mandatory or not.
>if (omp_clauses->device)
> -resolve_nonnegative_int_expr (omp_clauses->device, "DEVICE");
> +{
> + resolve_scalar_int_expr (omp_clauses->device, "DEVICE");
> + /* omp_initial_device == 1, omp_invalid_device = -4 (in GCC). */
> + if (omp_clauses->device->expr_type == EXPR_CONSTANT
> + && omp_clauses->device->ts.type == BT_INTEGER
> + && mpz_cmp_si (omp_clauses->device->value.integer, -1) < 0
> + && mpz_cmp_si (omp_clauses->device->value.integer, -4) != 0)
> + gfc_warning (0,
> + "INTEGER expression of DEVICE clause at %L must be non-"
> + "negative or omp_initial_device or omp_invalid_device",
> + _clauses->device->where);
> +}
So I think we should just resolve_scalar_int_expr and be done with that.
Otherwise LGTM.
Jakub
Re: [PATCH v3 06/11] OpenMP: Pointers and member mappings
On Fri, Sep 23, 2022 at 08:29:46AM +0100, Julian Brown wrote: > On Thu, 22 Sep 2022 15:17:08 +0200 > Jakub Jelinek wrote: > > > > + bool built_sym_hash = false; > > > > So, I think usually we don't construct such hash_maps right away, > > but have just pointer to the hash map initialized to NULL (then you > > don't need to built_sym_hash next to it) and you simply new the > > hash_map when needed the first time and delete it at the end (which > > does nothing if it is NULL). > > How about this version? (Re-tested.) I'd appreciate if Tobias could have a second look, I'm getting less and less familiar with Fortran, from my POV LGTM. The patch is ok if Tobias doesn't spot anything today. Jakub
Re: [PATCH v3 08/11] OpenMP/OpenACC: Rework clause expansion and nested struct handling
On Mon, Sep 19, 2022 at 08:40:34PM +0100, Julian Brown wrote: > On Wed, 14 Sep 2022 15:24:12 +0200 > Jakub Jelinek wrote: > > > On Tue, Sep 13, 2022 at 02:03:18PM -0700, Julian Brown wrote: > > > This patch is an extension and rewrite/rethink of the following two > > > patches: > > > > > > "OpenMP/OpenACC: Add inspector class to unify mapped address > > > analysis" > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591977.html > > > > > > "OpenMP: Handle reference-typed struct members" > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591978.html > > Here's a new version with some review comments addressed, rebased and > with some adjustments to tests etc., necessary because of tweaks to > earlier patches. > > Re-tested with offloading to NVPTX. OK? Ok, thanks. Jakub
Re: [PATCH v3 06/11] OpenMP: Pointers and member mappings
On Sun, Sep 18, 2022 at 08:19:29PM +0100, Julian Brown wrote: > @@ -2609,6 +2672,9 @@ gfc_trans_omp_clauses (stmtblock_t *block, > gfc_omp_clauses *clauses, >if (clauses == NULL) > return NULL_TREE; > > + hash_map sym_rooted_nl; Isn't hash_map ctor pretty costly (allocates memory etc.)? And gfc_trans_omp_clauses is called for all OpenMP constructs, in many cases they are never going to have any map clauses or even if they do, they might not trigger this code. > + bool built_sym_hash = false; So, I think usually we don't construct such hash_maps right away, but have just pointer to the hash map initialized to NULL (then you don't need to built_sym_hash next to it) and you simply new the hash_map when needed the first time and delete it at the end (which does nothing if it is NULL). Jakub
Re: [PATCH v3 05/11] OpenMP: push attaches to end of clause list in "target" regions
On Sun, Sep 18, 2022 at 08:10:00PM +0100, Julian Brown wrote: > I don't think any ATTACH clauses can appear during > the gimplify_adjust_omp_clause_1 walk. So, how about this? LGTM, thanks. Jakub
Re: [PATCH v3 11/11] FYI/unfinished: OpenMP 5.0 "declare mapper" support for C++
On Tue, Sep 13, 2022 at 02:04:30PM -0700, Julian Brown wrote:
> This patch implements OpenMP 5.0 "declare mapper" support for C++.
> This hasn't been fully revised yet following previous review comments,
> but I am including it in this series to demonstrate the new approach to
> gimplifying map clauses after "declare mapper" instantiation.
>
> The "gimplify_scan_omp_clauses" function is still called twice: firstly
> (before scanning the offload region's body) with SUPPRESS_GIMPLIFICATION
> set to true. This sets up variables in the splay tree, etc. but does
> not gimplify anything.
>
> Then, implicit "declare mappers" are instantiated after scanning the
> region's body, then "gimplify_scan_omp_clauses" is called again, and
> does the rest of its previous tasks -- builds struct sibling lists,
> and gimplifies clauses. Then gimplify_adjust_omp_clauses is called,
> and compilation continues.
>
> Does this approach seem OK?
As I wrote in https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596444.html
I don't see a reason for this 3 passes approach and it will be a
maintainance problem.
The reason why we have 2 passes approach is that we need to populate the
splay trees based on explicit clauses before we gimplify the body, at which
point we can look up for variables seen in the body those splay trees,
either mark the explicit ones as seen or create new ones for implicit
etc. And finally we need to adjust some explicit clauses based on that
(that is what the main loop in gimplify_adjust_omp_clauses does) and
add new clauses for implicit data sharing or mapping (that is done
through splay tree traversal through gimplify_adjust_omp_clauses_1).
We also need to gimplify expressions from the clauses somewhere, but
due to the way the gimplifier works we don't care that much when
exactly it is done, it can be done before the body is gimplified
(for most clauses we do it there in gimplify_scan_omp_clauses), it can be
done after the body is gimplified, the expressions from the clauses
will be in either case gimplified into some gimple sequence that we'll
place before the construct with its body. The only reason to have
the gimplification done before and not after would be if the temporaries
from the gimplification are then needed in the splay trees for the
gimplification of the body.
I'd strongly prefer if the gimplification APIs for all the constructs
were just gimplify_scan_omp_clauses before processing the body and
gimplify_adjust_omp_clauses after doing so, not some extra APIs.
If there is a strong reason for 3 or more passes on say a subset of clauses,
either gimplify_scan_omp_clauses or gimplify_adjust_omp_clauses can do more
than one loop over the clauses, but those secondary loops ideally should be
enabled only when needed (e.g. see the gimplify_adjust_omp_clauses
has_inscan_reductions guarded loop at the end) and should only process
clauses they strictly have to.
Conceptually, there is no reason why e.g. the gimplification of the explicit
map clauses can't be done in gimplify_adjust_omp_clauses rather than in
gimplify_scan_omp_clauses. What needs to happen in gimplify_scan_omp_clauses
is just what affects the gimplification of the body. Does sorting of the
map clause affect it? I don't think so. Does declare mapper processing of
the explicit map clauses affect it? I very much hope it doesn't, but I'm
afraid I don't remember all the declare mapper restrictions and discussions.
Can declare mapper e.g. try to map an unrelated variable in addition to say
parts of the structure? If yes, then it could affect the gimplification of
the body, say
struct S { int s, t; };
extern int y;
#pragma omp declare mapper (struct S s) map (to: s.s) map (to: y)
#pragma omp target defaultmap(none:scalar) map(tofrom: x)
{
int x = s.s + y;
}
because if we do process declare mapper before the gimplification of the
body, then y would be explicitly mapped, but if we don't, it wouldn't and
it should be rejected. But in that case we'd be in big trouble with
implicit mappings using that same declare mapper. Because it would then be
significant in which order we process the vars during gimplification of the
body and whether we process declare mapper right away at that point or only
at the end etc.
We have the
"List items in map clauses on the declare mapper directive may only refer to
the declared
variable var and entities that could be referenced by a procedure defined at
the same
location."
restriction but not sure that says the above isn't valid.
So probably it needs to be discussed in omp-lang.
If the agreement is that declare mapper for explicit map clauses needs to be
done before processing the body and declare mapper for implicit map clauses
can be deferred to the end, then yes, we need to handle declare mapper
twice, but it can be done say in a secondary loop of gimplify_scan_omp_clauses
guarded on at least one of the map clauses needs declare mapper processing
or something similar that can be quickly
Re: [PATCH v3 08/11] OpenMP/OpenACC: Rework clause expansion and nested struct handling
On Tue, Sep 13, 2022 at 02:03:18PM -0700, Julian Brown wrote:
> This patch is an extension and rewrite/rethink of the following two patches:
>
> "OpenMP/OpenACC: Add inspector class to unify mapped address analysis"
> https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591977.html
>
> "OpenMP: Handle reference-typed struct members"
> https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591978.html
>
> The latter was reviewed here by Jakub:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595510.html with the
>
> with the comment,
>
> > Why isn't a reference to pointer handled that way too?
>
> and that opened a whole can of worms... generally, C++ references were
> not handled very consistently after the clause-processing code had been
> extended several times already for both OpenACC and OpenMP, and many
> cases of using C++ (and Fortran) references were broken. Even some
> cases not involving references were being mapped incorrectly.
>
> At present a single clause may be turned into several mapping nodes,
> or have its mapping type changed, in several places scattered through
> the front- and middle-end. The analysis relating to which particular
> transformations are needed for some given expression has become quite hard
> to follow. Briefly, we manipulate clause types in the following places:
>
> 1. During parsing, in c_omp_adjust_map_clauses. Depending on a set of
> rules, we may change a FIRSTPRIVATE_POINTER (etc.) mapping into
> ATTACH_DETACH, or mark the decl addressable.
>
> 2. In semantics.cc or c-typeck.cc, clauses are expanded in
> handle_omp_array_sections (called via {c_}finish_omp_clauses, or in
> finish_omp_clauses itself. The two cases are for processing array
> sections (the former), or non-array sections (the latter).
>
> 3. In gimplify.cc, we build sibling lists for struct accesses, which
> groups and sorts accesses along with their struct base, creating
> new ALLOC/RELEASE nodes for pointers.
>
> 4. In gimplify.cc:gimplify_adjust_omp_clauses, mapping nodes may be
> adjusted or created.
>
> This patch doesn't completely disrupt this scheme, though clause
> types are no longer adjusted in c_omp_adjust_map_clauses (step 1).
> Clause expansion in step 2 (for C and C++) now uses a single, unified
> mechanism, parts of which are also reused for analysis in step 3.
>
> Rather than the kind-of "ad-hoc" pattern matching on addresses used to
> expand clauses used at present, a new method for analysing addresses is
> introduced. This does a recursive-descent tree walk on expression nodes,
> and emits a vector of tokens describing each "part" of the address.
> This tokenized address can then be translated directly into mapping nodes,
> with the assurance that no part of the expression has been inadvertently
> skipped or misinterpreted. In this way, all the variations of ways
> pointers, arrays, references and component accesses can be teased apart
> into easily-understood cases - and we know we've "parsed" the whole
> address before we start analysis, so the right code paths can easily
> be selected.
>
> For example, a simple access "arr[idx]" might parse as:
>
> base-decl access-indexed-array
>
> or "mystruct->foo[x]" with a pointer "foo" component might parse as:
>
> base-decl access-pointer component-selector access-pointer
>
> A key observation is that support for "array" bases, e.g. accesses
> whose root nodes are not structures, but describe scalars or arrays,
> and also *one-level deep* structure accesses, have first-class support
> in gimplify and beyond. Expressions that use deeper struct accesses
> or e.g. multiple indirections were more problematic: some cases worked,
> but lots of cases didn't. This patch reimplements the support for those
> in gimplify.cc, again using the new "address tokenization" support.
>
> An expression like "mystruct->foo->bar[0:10]" used in a mapping node will
> translate the right-hand access directly in the front-end. The base for
> the access will be "mystruct->foo". This is handled recursively -- there
> may be several accesses of "mystruct"'s members on the same directive,
> so the sibling-list building machinery can be used again. (This was
> already being done for OpenACC, but the new implementation differs
> somewhat in details, and is more robust.)
>
> For OpenMP, in the case where the base pointer itself,
> i.e. "mystruct->foo" here, is NOT mapped on the same directive, we
> create a "fragile" mapping. This turns the "foo" component access
> into a zero-length allocation (which is a new feature for the runtime,
> so support has been added there too).
>
> A couple of changes have been made to how mapping clauses are turned
> into mapping nodes:
>
> The first change is based on the observation that it is probably never
> correct to use GOMP_MAP_ALWAYS_POINTER for component accesses (e.g. for
> references), because if the containing struct is already mapped on
Re: [PATCH v3 07/11] OpenMP/OpenACC: Reindent TO/FROM/_CACHE_ stanza in {c_}finish_omp_clause
On Tue, Sep 13, 2022 at 02:03:17PM -0700, Julian Brown wrote: > This patch trivially adds braces and reindents the > OMP_CLAUSE_TO/OMP_CLAUSE_FROM/OMP_CLAUSE__CACHE_ stanza in > c_finish_omp_clause and finish_omp_clause, in preparation for the > following patch (to clarify the diff a little). > > 2022-09-13 Julian Brown > > gcc/c/ > * c-typeck.cc (c_finish_omp_clauses): Add braces and reindent > OMP_CLAUSE_TO/OMP_CLAUSE_FROM/OMP_CLAUSE__CACHE_ stanza. > > gcc/cp/ > * semantics.cc (finish_omp_clause): Add braces and reindent > OMP_CLAUSE_TO/OMP_CLAUSE_FROM/OMP_CLAUSE__CACHE_ stanza. Not very happy about this because it ruins git blame, some vars can be declared separately from initializing them and be thus usable in switches. But I see you use there classes with ctors... So ok. Jakub
Re: [PATCH v3 06/11] OpenMP: Pointers and member mappings
On Tue, Sep 13, 2022 at 02:03:16PM -0700, Julian Brown wrote:
> @@ -3440,6 +3437,50 @@ gfc_trans_omp_clauses (stmtblock_t *block,
> gfc_omp_clauses *clauses,
> {
> if (pointer || (openacc && allocatable))
> {
> + gfc_omp_namelist *n2
> + = openacc ? NULL : clauses->lists[OMP_LIST_MAP];
> +
> + /* If the last reference is a pointer to a derived
> + type ("foo%dt_ptr"), check if any subcomponents
> + of the same derived type member are being mapped
> + elsewhere in the clause list ("foo%dt_ptr%x",
> + etc.). If we have such subcomponent mappings,
> + we only create an ALLOC node for the pointer
> + itself, and inhibit mapping the whole derived
> + type. */
> +
> + for (; n2 != NULL; n2 = n2->next)
> + {
> + if (n == n2 || !n2->expr)
> + continue;
> +
> + int dep
> + = gfc_dep_resolver (n->expr->ref, n2->expr->ref,
> + NULL, true);
> + if (dep == 0)
> + continue;
Isn't this and the other loop quadratic compile time in number of clauses?
Could it be done linearly through some perhaps lazily built hash table or
something similar?
Jakub
Re: [PATCH v3 05/11] OpenMP: push attaches to end of clause list in "target" regions
On Tue, Sep 13, 2022 at 02:03:15PM -0700, Julian Brown wrote:
> This patch moves GOMP_MAP_ATTACH{_ZERO_LENGTH_ARRAY_SECTION} nodes to
> the end of the clause list, for offload regions. This ensures that when
> we do the attach operation, both the "attachment point" and the target
> region have both already been mapped on the target. This avoids a
> pathological case that can otherwise happen with struct sibling-list
> handling.
>
> 2022-09-13 Julian Brown
>
> gcc/
> * gimplify.cc (omp_segregate_mapping_groups): Update comment.
> (omp_push_attaches_to_end): New function.
> (gimplify_scan_omp_clauses): Use omp_push_attaches_to_end for offloaded
> regions.
Shouldn't this be done at the end of gimplify_adjust_omp_clauses?
I mean, can't further attach clauses appear because of declare mapper
for implicitly mapped variables?
Other than that, it is yet another walk of the whole clause list, so would
be nicer if it could be done in an existing walk over the clauses or
at least have a flag whether there are any such clauses present and do it
only in that case.
If it could be done in the main gimplify_adjust_omp_clauses loop, nice,
if it can appear also during
splay_tree_foreach (ctx->variables, gimplify_adjust_omp_clauses_1, );
that isn't the case.
Jakub
Re: [PATCH v3 04/11] OpenMP/OpenACC: mapping group list-handling improvements
On Tue, Sep 13, 2022 at 02:01:45PM -0700, Julian Brown wrote: > @@ -9443,6 +9499,41 @@ omp_containing_struct (tree expr) >return expr; > } > Missing function comment here. > +static bool > +omp_mapped_by_containing_struct (hash_map + omp_mapping_group *> *grpmap, > + tree decl, > + omp_mapping_group **mapped_by_group) Otherwise LGTM. Jakub
Re: [PATCH v3 03/11] OpenMP/OpenACC struct sibling list gimplification extension and rework
On Tue, Sep 13, 2022 at 02:01:44PM -0700, Julian Brown wrote: > static tree > -insert_struct_comp_map (enum tree_code code, tree c, tree struct_node, > - tree prev_node, tree *scp) > +build_struct_comp_nodes (enum tree_code code, tree grp_start, tree grp_end, > + tree *extra_node) I'd rename even this function, say to build_omp_struct_comp_nodes. So that it is clear it is OpenMP/OpenACC specific. Otherwise LGTM. Jakub
Re: [PATCH v3 02/11] Remove omp_target_reorder_clauses
On Tue, Sep 13, 2022 at 02:01:43PM -0700, Julian Brown wrote: > This patch has been split out from the previous one to avoid a > confusingly-interleaved diff. The two patches will be committed squashed > together. > > 2022-09-13 Julian Brown > > gcc/ > * gimplify.c (omp_target_reorder_clauses): Delete. Ok. Jakub
Re: [PATCH v3 01/11] OpenMP 5.0: Clause ordering for OpenMP 5.0 (topological sorting by base pointer)
On Tue, Sep 13, 2022 at 02:01:42PM -0700, Julian Brown wrote:
> --- a/gcc/omp-low.cc
> +++ b/gcc/omp-low.cc
> @@ -1599,8 +1599,11 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> {
> /* If this is an offloaded region, an attach operation should
>only exist when the pointer variable is mapped in a prior
> - clause. */
> - if (is_gimple_omp_offloaded (ctx->stmt))
> + clause.
> + If we had an error, we may not have attempted to sort clauses
> + properly, so avoid the test. */
> + if (is_gimple_omp_offloaded (ctx->stmt)
> + && !seen_error ())
I'll repeat that it would be better if we just leave out the whole
GOMP_TARGET if there were errors regarding it and the clauses aren't sorted
properly.
But I'm ok if it is handled incrementally and this spot reverted.
Otherwise LGTM.
Jakub
Re: [Patch] OpenMP: Document ompx warnings + add Fortran omx warning [PR106670]
On Mon, Aug 29, 2022 at 11:24:52AM +0200, Tobias Burnus wrote:
> PR fortran/106670
>
> gcc/fortran/ChangeLog:
>
> * scanner.cc (skip_fixed_omp_sentinel): Add -Wsurprising warning
> for 'omx' sentinels with -fopenmp.
> * invoke.texi (-Wsurprising): Document additional warning case.
>
> libgomp/ChangeLog:
>
> * libgomp.texi (OpenMP 5.2): Add comment to ompx/omx entry.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/gomp/ompx-1.c: New test.
> * c-c++-common/gomp/ompx-2.c: New test.
> * g++.dg/gomp/ompx-attrs-1.C: New test.
> * gfortran.dg/gomp/ompx-1.f90: New test.
> * gfortran.dg/gomp/omx-1.f: New test.
> * gfortran.dg/gomp/omx-2.f: New test.
> --- a/libgomp/libgomp.texi
> +++ b/libgomp/libgomp.texi
> @@ -359,7 +359,13 @@ to address of matching mapped list item per 5.1, Sect.
> 2.21.7.2 @tab N @tab
> @item @code{omp_in_explicit_task} routine and @emph{implicit-task-var} ICV
>@tab N @tab
> @item @code{omp}/@code{ompx}/@code{omx} sentinels and
> @code{omp_}/@code{ompx_}
> - namespaces @tab N/A @tab
> + namespaces @tab N/A
> + @tab warning for @code{omp/ompx} sentinels@footnote{@code{omp/ompx}
> + sentinels as C/C++ pragma and C++ attributes are warned for with
> + @code{-Wunknown-pragmas} (implied by @code{-Wall}) and
> @code{-Wattributes}
> + (by default), respectively; for Fortran free-source code, the there is
> a
s/by default/enabled by default/
s/the there/there/
> + warning by default and for fixed-source code with @code{-Wsurprising}
s/by default/enabled by default/
> + (enabled by @code{-Wall})}
> @item Clauses on @code{end} directive can be on directive @tab N @tab
> @item Deprecation of no-argument @code{destroy} clause on @code{depobj}
>@tab N @tab
Otherwise LGTM.
Jakub
Re: [Patch] OpenMP/Fortran: Permit end-clause on directive
On Thu, Sep 08, 2022 at 05:21:08PM +0200, Jakub Jelinek via Gcc-patches wrote: > Otherwise LGTM. Oh, and what Harald wrote, it might be better to split the nowait-4.f90 test into 2, one for all the valid nowait cases and one for the invalid ones, such that the first one can be compile tested all the way through assembly. Jakub
Re: [Patch] OpenMP/Fortran: Permit end-clause on directive
On Fri, Aug 26, 2022 at 08:21:26PM +0200, Tobias Burnus wrote:
> I did run into some issues related to this; those turned out to be
> unrelated, but I end ended up implementing this feature.
>
> Side remark: 'omp parallel workshare' seems to actually permit 'nowait'
> now, but I guess that's an unintended change due to the
> syntax-representation change. Hence, it is now tracked as Spec Issue
> 3338 and I do not permit it.
>
> OK for mainline?
>
> 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
> OpenMP/Fortran: Permit end-clause on directive
>
> gcc/fortran/ChangeLog:
>
> * openmp.cc (OMP_DO_CLAUSES, OMP_SCOPE_CLAUSES,
> OMP_SECTIONS_CLAUSES, OMP_SINGLE_CLAUSES): Add 'nowait'.
This doesn't describe what the patch actually does, Add 'nowait'.
is only true for the first 3, for OMP_SINGLE_CLAUSES IMHO you
want a separate
(OMP_SINGLE_CLAUSES): Add 'nowait' and 'copyprivate'.
entry.
> @@ -3855,7 +3857,7 @@ cleanup:
> | OMP_CLAUSE_ORDER | OMP_CLAUSE_ALLOCATE)
> #define OMP_SINGLE_CLAUSES \
>(omp_mask (OMP_CLAUSE_PRIVATE) | OMP_CLAUSE_FIRSTPRIVATE \
> - | OMP_CLAUSE_ALLOCATE)
> + | OMP_CLAUSE_ALLOCATE | OMP_CLAUSE_NOWAIT | OMP_CLAUSE_COPYPRIVATE)
> #define OMP_ORDERED_CLAUSES \
>(omp_mask (OMP_CLAUSE_THREADS) | OMP_CLAUSE_SIMD)
> #define OMP_DECLARE_TARGET_CLAUSES \
> @@ -5909,13 +5915,11 @@ gfc_match_omp_teams_distribute_simd (void)
> match
> gfc_match_omp_workshare (void)
> {
> - if (gfc_match_omp_eos () != MATCH_YES)
> -{
> - gfc_error ("Unexpected junk after $OMP WORKSHARE statement at %C");
> - return MATCH_ERROR;
> -}
> + gfc_omp_clauses *c;
> + if (gfc_match_omp_clauses (, omp_mask (OMP_CLAUSE_NOWAIT)) != MATCH_YES)
> +return MATCH_ERROR;
>new_st.op = EXEC_OMP_WORKSHARE;
> - new_st.ext.omp_clauses = gfc_get_omp_clauses ();
> + new_st.ext.omp_clauses = c;
>return MATCH_YES;
> }
I think it would be better to introduce OMP_WORKSHARE_CLAUSES and use
it in both gfc_match_omp_workshare and just use
return match_omp (EXEC_OMP_WORKSHARE, OMP_WORKSHARE_CLAUSES);
?
> @@ -6954,6 +6952,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses
> *omp_clauses,
> }
> break;
> case OMP_LIST_COPYPRIVATE:
> + if (omp_clauses->nowait)
> + gfc_error ("NOWAIT clause must not be be used with COPYPRIVATE "
s/be be/be/
> + "clause at %L", >where);
> for (; n != NULL; n = n->next)
> {
> if (n->sym->as && n->sym->as->type == AS_ASSUMED_SIZE)
> @@ -5284,7 +5285,13 @@ parse_omp_do (gfc_statement omp_st)
>if (st == omp_end_st)
> {
>if (new_st.op == EXEC_OMP_END_NOWAIT)
> - cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool;
> + {
> + if (cp->ext.omp_clauses->nowait && new_st.ext.omp_bool)
> + gfc_error_now ("Duplicated NOWAIT clause on %s and %s at %C",
> +gfc_ascii_statement (omp_st),
> +gfc_ascii_statement (omp_end_st));
> + cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool;
> + }
>else
> gcc_assert (new_st.op == EXEC_NOP);
>gfc_clear_new_st ();
Not sure if the standard is clear enough that unique clauses can't be
repeated on both directive and corresponding end directive. But let's
assume that is the case.
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/copyprivate-2.f90
> @@ -0,0 +1,69 @@
> + FUNCTION t()
> +INTEGER :: a, b, t
> +a = 0
> +t = b
> +b = 0
> +!$OMP PARALLEL REDUCTION(+:b)
> + !$OMP SINGLE COPYPRIVATE (b) NOWAIT ! { dg-error "NOWAIT clause must
> not be be used with COPYPRIVATE clause" }
Here too (several times).
> +!$OMP ATOMIC WRITE
> +b = 6
> + !$OMP END SINGLE
> +!$OMP END PARALLEL
> +t = t + b
> + END FUNCTION
> +
> + FUNCTION t2()
> +INTEGER :: a, b, t2
> +a = 0
> +t2 = b
> +b = 0
> +!$OMP PARALLEL REDUCTION(+:b)
> + !$OMP SINGLE NOWAIT COPYPRIVATE (b) ! { dg-error "NOWAIT clause must
> not be be used with COPYPRIVATE clause" }
> +!$OMP ATOMIC WRITE
> +b = 6
> + !$OMP END SINGLE
> +!$OMP END PARALLEL
> +t2 = t2 + b
> + END FUNCTION
> +
> + FUNCTION t3()
> +INTEGER :: a, b, t3
> +a = 0
> +t3 = b
> +b = 0
> +!$OMP PARALLEL REDUCTION(+:b)
> + !$OMP SINGLE COPYPRIVATE (b) ! { dg-error "NOWAIT clause must not be
> be used with COPYPRIVATE clause" }
> +!$OMP ATOMIC WRITE
> +b = 6
> + !$OMP END SINGLE NOWAIT
> +!$OMP END PARALLEL
> +t3 = t3 + b
> + END FUNCTION
> +
> + FUNCTION t4()
> +INTEGER :: a, b, t4
> +a = 0
> +t4 = b
> +b = 0
> +!$OMP PARALLEL
Re: Floating-point comparisons in the middle-end
On Thu, Sep 01, 2022 at 11:04:03AM +0200, FX wrote: > Hi Jakub, > > >> 2. All the functions are available as GCC type-generic built-ins (yeah!), > >> except there is no __builtin_ iseqsig > >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77928). Is there a > >> fundamental problem with creating one, and could someone help there? > > > > IMHO until that one is implemented you can just use > > tx = x, ty = y, tx>=ty && tx<=ty > > (in GENERIC just SAVE_EXPR >= SAVE_EXPR && SAVE_EXPR <= > > SAVE_EXPR > > If it’s just that (optimization aside), I probably can create a C built-in. > It would need to be: > > 1. defined in builtins.def > 2. lowered in builtins.cc > 3. type-checked in c-family/c-common.cc > 4. documented in doc/extend.texi > 5. tested in fp-test.cc > 6. covered in the testsuite > > Is that right? Dunno if we really need a builtin for this, especially if it is lowered to that x >= y && x <= y early, will defer to Joseph. Because if it is for better code generation only, IMNSHO we want to optimize even when users write it that way by hand and so want to pattern recognize that during instruction selection before expansion (isel pass) or during expansion if target can do that. E.g. x86 with AVX can do that: where the 4 booleans are A>B, A= y && x <= y can be handled using vcmpeq_ossd or similar instructions. Jakub
Re: Floating-point comparisons in the middle-end
On Thu, Sep 01, 2022 at 10:04:58AM +0200, FX wrote: > Fortran 2018 introduced intrinsic functions for all the IEEE-754 comparison > operations, compareQuiet* and compareSignaling* I want to introduce those > into the Fortran front-end, and make them emit the right code. But cannot > find the correspondance between IEEE-754 nomenclature and GCC internal > representation. > > I understand that the middle-end representation was mostly created with C in > mind, so I assume that the correspondance is that used by the C standard. > That helps me to some extent, as I can find draft documents that seem to list > the following table (page 8 of > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1615.pdf): > > compareQuietEqual == > compareQuietNotEqual != > compareSignalingEqual iseqsig > compareSignalingGreater > > compareSignalingGreaterEqual >= > compareSignalingLess < > compareSignalingLessEqual <= > compareSignalingNotEqual !iseqsig > compareSignalingNotGreater !(x>y) > compareSignalingLessUnordered !(x=>y) > compareSignalingNotLess !(x compareSignalingGreaterUnorder !(x<=y) > compareQuietGreater isgreater > compareQuietGreaterEqual isgreaterequal > compareQuietLess isless > compareQuietLessEqual islessequal > compareQuietUnordered isunordered > compareQuietNotGreater !isgreater > compareQuietLessUnordered !isgreaterequal > compareQuietNotLess !isless > compareQuietGreaterUnordered !islessequal > compareQuietOrdered !isunordered > > > I have two questions: > > 1. Is this list normative, and was it modified later (I have only found a > 2012 draft)? > > 2. All the functions are available as GCC type-generic built-ins (yeah!), > except there is no __builtin_ iseqsig > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77928). Is there a > fundamental problem with creating one, and could someone help there? IMHO until that one is implemented you can just use tx = x, ty = y, tx>=ty && tx<=ty (in GENERIC just SAVE_EXPR >= SAVE_EXPR && SAVE_EXPR <= SAVE_EXPR PowerPC backend is still broken, not just for that but for most other cases above, it doesn't violate just Fortran requirements, but C too. Jakub
[PATCH] fortran: Drop -static-lib{gfortran,quadmath} from f951 [PR46539]
Hi!
As discussed earlier, all other -static-lib* options are Driver only,
these 2 are Driver in common.opt and Fortran in lang.opt.
The spec files never pass the -static-lib* options down to any compiler
(f951 etc.), so the 2 errors below are reported only when one
runs ./f951 -static-libgfortran by hand.
The following patch just removes f951 support of these options, the
gfortran driver behavior remains as before. For other -static-lib*
option (and even these because it is never passed to f951) we never
error if we can't support those options, and e.g. Darwin is actually
able to handle those options through other means.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2022-08-20 Jakub Jelinek
PR fortran/46539
* lang.opt (static-libgfortran, static-libquadmath): Change Fortran
to Driver.
* options.cc (gfc_handle_option): Don't handle OPT_static_libgfortran
nor OPT_static_libquadmath here.
--- gcc/fortran/lang.opt.jj 2022-08-17 17:00:28.558530108 +0200
+++ gcc/fortran/lang.opt2022-08-19 18:09:23.505859992 +0200
@@ -860,11 +860,11 @@ Fortran Joined Separate
; Documented in common.opt
static-libgfortran
-Fortran
+Driver
Statically link the GNU Fortran helper library (libgfortran).
static-libquadmath
-Fortran
+Driver
Statically link the GCC Quad-Precision Math Library (libquadmath).
std=f2003
--- gcc/fortran/options.cc.jj 2022-08-17 17:00:28.559530096 +0200
+++ gcc/fortran/options.cc 2022-08-19 18:05:32.153797148 +0200
@@ -685,20 +685,6 @@ gfc_handle_option (size_t scode, const c
gfc_option.source_form = FORM_FREE;
break;
-case OPT_static_libgfortran:
-#ifndef HAVE_LD_STATIC_DYNAMIC
- gfc_fatal_error ("%<-static-libgfortran%> is not supported in this "
- "configuration");
-#endif
- break;
-
-case OPT_static_libquadmath:
-#ifndef HAVE_LD_STATIC_DYNAMIC
- gfc_fatal_error ("%<-static-libquadmath%> is not supported in this "
- "configuration");
-#endif
- break;
-
case OPT_fintrinsic_modules_path:
case OPT_fintrinsic_modules_path_:
Jakub
Re: [PATCH] fortran, v2: Add -static-libquadmath support [PR46539]
On Thu, Aug 18, 2022 at 11:35:06AM +0100, Iain Sandoe wrote:
> > --- gcc/fortran/options.cc.jj 2022-01-18 11:58:59.568982256 +0100
> > +++ gcc/fortran/options.cc 2022-08-16 14:56:22.807525218 +0200
> > @@ -692,6 +692,13 @@ gfc_handle_option (size_t scode, const c
> > #endif
> > break;
> >
> > +case OPT_static_libquadmath:
> > +#ifndef HAVE_LD_STATIC_DYNAMIC
> > + gfc_fatal_error ("%<-static-libquadmath%> is not supported in this "
> > + "configuration");
> > +#endif
>
> I think that this will disable the option on Darwin (where the linker does not
> support Bstatic/dynamic) - the point of the specs outfile substitution is to
> work
> for such platforms. So long as the option is not stripped out by the driver,
> the
> specs substitution should work (there is a bug in the g++ driver where this is
> not happening properly for -static-libstdc++ - but the gdc driver has it
> right).
It does the same thing as OPT_static_libgfortran and that option
presumably isn't disabled on Darwin.
My guess is that this is just dead code and could be removed for both
options, I think gfc_handle_option is only in f951 program, and the
-static-lib* options are driver only, not passed to the compiler.
Note, for other options like -static-libstdc++, we also don't reject them
if HAVE_LD_STATIC_DYNAMIC isn't defined.
Jakub
Re: [Patch] Fortran: OpenMP fix declare simd inside modules and absent linear step [PR106566]
On Tue, Aug 16, 2022 at 04:45:07PM +0200, Tobias Burnus wrote:
> Fixed subject line: "absent linear" should be "absent linear step" in the
> subject line;
> i.e. with "step" added: "Fortran: OpenMP fix declare simd inside modules and
> absent linear step [PR106566]"
>
> I have also decided to move the 'step = 1' to openmp.cc, which also set it
> before with
> the old pre-OpenMP 5.2 syntax.
>
> I also added a pre-OpenMP-5.2-syntax example.
>
> * * *
>
> For GCC 12 (and GCC 11), only the '%s' fix and the third, now added example
> apply;
> for the 5.1 syntax, 'step' was already set.
>
> OK? And thoughts regarding the backports (none? Only 12? Or 11+12?)?
>
> 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
> Fortran: OpenMP fix declare simd inside modules and absent linear step
> [PR106566]
>
> gcc/fortran/ChangeLog:
>
> PR fortran/106566
> * openmp.cc (gfc_match_omp_clauses): Fix setting linear-step value
> to 1 when not specified.
> (gfc_match_omp_declare_simd): Accept module procedures.
>
> gcc/testsuite/ChangeLog:
>
> PR fortran/106566
> * gfortran.dg/gomp/declare-simd-4.f90: New test.
> * gfortran.dg/gomp/declare-simd-5.f90: New test.
> * gfortran.dg/gomp/declare-simd-6.f90: New test.
>
> gcc/fortran/openmp.cc | 10 +++--
> gcc/testsuite/gfortran.dg/gomp/declare-simd-4.f90 | 42 +++
> gcc/testsuite/gfortran.dg/gomp/declare-simd-5.f90 | 49
> +++
> gcc/testsuite/gfortran.dg/gomp/declare-simd-6.f90 | 42 +++
> 4 files changed, 140 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
> index a7eb6c3e8f4..594907714ff 100644
> --- a/gcc/fortran/openmp.cc
> +++ b/gcc/fortran/openmp.cc
> @@ -2480,7 +2480,7 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const
> omp_mask mask,
> goto error;
> }
> }
> - else
> + if (step == NULL)
> {
> step = gfc_get_constant_expr (BT_INTEGER,
> gfc_default_integer_kind,
Ah, didn't know that gfc_match ("%e ) ", ) will free and clear
step if it successfully matched it first and then doesn't match ) after it.
So ok.
> @@ -4213,9 +4213,13 @@ gfc_match_omp_declare_simd (void)
>gfc_omp_declare_simd *ods;
>bool needs_space = false;
>
> - switch (gfc_match (" ( %s ) ", _name))
> + switch (gfc_match (" ( "))
> {
> -case MATCH_YES: break;
> +case MATCH_YES:
> + if (gfc_match_symbol (_name, /* host assoc = */ true) != MATCH_YES
> + || gfc_match (" ) ") != MATCH_YES)
> + return MATCH_ERROR;
> + break;
> case MATCH_NO: proc_name = NULL; needs_space = true; break;
> case MATCH_ERROR: return MATCH_ERROR;
> }
LGTM.
> diff --git a/gcc/testsuite/gfortran.dg/gomp/declare-simd-4.f90
> b/gcc/testsuite/gfortran.dg/gomp/declare-simd-4.f90
> new file mode 100644
> index 000..44132525963
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/declare-simd-4.f90
> @@ -0,0 +1,42 @@
> +! { dg-do compile }
> +! { dg-additional-options "-fdump-tree-gimple" }
> +!
> +! PR fortran/106566
> +!
> +! { dg-final { scan-tree-dump-times "__attribute__\\(\\(omp declare simd
> \\(linear\\(0:ref,step\\(4\\)\\) simdlen\\(8\\)\\)\\)\\)" 2 "gimple" } }
> +! { dg-final { scan-tree-dump-times "__attribute__\\(\\(omp declare simd
> \\(linear\\(0:ref,step\\(8\\)\\) simdlen\\(8\\)\\)\\)\\)" 2 "gimple" } }
> +
> +subroutine add_one2(p)
> + implicit none
> + !$omp declare simd(add_one2) linear(p: ref) simdlen(8)
> + integer :: p
Wonder if it wouldn't be better to use integer(kind=4) explicitly
when you try to match the size of that multiplied by 1 or 2 in
dg-final, as say with -fdefault-integer-8 this will fail miserably
otherwise. Ditto in other spots in this as well as other tests.
Ok with/without that change.
As for backports, I'd wait some time with just trunk and then
backport wherever you are willing to test it.
Jakub
[PATCH] fortran, v2: Add -static-libquadmath support [PR46539]
On Wed, Aug 17, 2022 at 10:28:29AM +0200, Mikael Morin wrote:
> Tobias approved it already, but I spotted what looks like typos.
> See below.
Thanks for catching that.
> > --- gcc/config/darwin.h.jj 2022-08-16 14:51:14.529544492 +0200
> > +++ gcc/config/darwin.h 2022-08-16 14:53:54.402460097 +0200
> > @@ -443,6 +443,7 @@ extern GTY(()) int darwin_ms_struct;
> >%:replace-outfile(-lobjc libobjc-gnu.a%s); \
> > :%:replace-outfile(-lobjc -lobjc-gnu )}}\
> > %{static|static-libgcc|static-libgfortran:%:replace-outfile(-lgfortran
> > libgfortran.a%s)}\
> > + %{static|static-libgcc|static-libgfortran:%:replace-outfile(-lquadmath
> > libquadmath.a%s)}\
>
> s/static-libgfortran/static-libquadmath/ I guess? Otherwise I don’t
> understand the corresponding ChangeLog description.
Yeah. I just copied this part from the 2014 patch and didn't spot that.
> > +elif grep "conftest1.$ac_objext.* -aarchive_shared -lgfortran
> > -adefault .*conftest2.$ac_objext" \
> > + conftest.cmd >/dev/null 2>&1; then
> > + LQUADMATH="%{static-libquadmath:-aarchive_shared} -lquadmath
> > %{static-libquadmath:-adefault}"
> > +elif grep "conftest1.$ac_objext.* ligfortran.a .*conftest2.$ac_objext"
> > \
>
> s/ligfortran.a/libgfortran.a/
Indeed. Also removed the space before libgfortran.a because I'm not sure if
it won't have because of the %s full path in front of that.
This is Darwin only stuff.
I must say I don't know if even this elif ... LQUADMATH part is needed,
maybe replace-outfile will rename even the -lquadmath from the spec file.
Here is an updated version (but nothing relevant to Linux changed, so there
is no point for me to bootstrap/regtest it again):
2022-08-17 Francois-Xavier Coudert
Jakub Jelinek
PR fortran/46539
gcc/
* common.opt (static-libquadmath): New option.
* gcc.c (driver_handle_option): Always accept -static-libquadmath.
* config/darwin.h (LINK_SPEC): Handle -static-libquadmath.
gcc/fortran/
* lang.opt (static-libquadmath): New option.
* invoke.texi (-static-libquadmath): Document it.
* options.c (gfc_handle_option): Error out if -static-libquadmath
is passed but we do not support it.
libgfortran/
* acinclude.m4 (LIBQUADSPEC): From $FC -static-libgfortran -###
output determine -Bstatic/-Bdynamic, -bstatic/-bdynamic,
-aarchive_shared/-adefault linker support or Darwin remapping
of -lgfortran to libgfortran.a%s and use that around or instead
of -lquadmath in LIBQUADSPEC.
* configure: Regenerated.
--- gcc/common.opt.jj 2022-06-27 11:18:02.050066582 +0200
+++ gcc/common.opt 2022-08-16 14:51:04.611673800 +0200
@@ -3601,6 +3601,10 @@ static-libphobos
Driver
; Documented for D, but always accepted by driver.
+static-libquadmath
+Driver
+; Documented for Fortran, but always accepted by driver.
+
static-libstdc++
Driver
--- gcc/gcc.cc.jj 2022-08-11 09:57:24.765334380 +0200
+++ gcc/gcc.cc 2022-08-16 14:57:54.708327024 +0200
@@ -4585,12 +4585,14 @@ driver_handle_option (struct gcc_options
case OPT_static_libgcc:
case OPT_shared_libgcc:
case OPT_static_libgfortran:
+case OPT_static_libquadmath:
case OPT_static_libphobos:
case OPT_static_libstdc__:
/* These are always valid, since gcc.cc itself understands the
first two, gfortranspec.cc understands -static-libgfortran,
-d-spec.cc understands -static-libphobos, and g++spec.cc
-understands -static-libstdc++ */
+d-spec.cc understands -static-libphobos, g++spec.cc
+understands -static-libstdc++ and libgfortran.spec handles
+-static-libquadmath. */
validated = true;
break;
--- gcc/config/darwin.h.jj 2022-08-16 14:51:14.529544492 +0200
+++ gcc/config/darwin.h 2022-08-16 14:53:54.402460097 +0200
@@ -443,6 +443,7 @@ extern GTY(()) int darwin_ms_struct;
%:replace-outfile(-lobjc libobjc-gnu.a%s); \
:%:replace-outfile(-lobjc -lobjc-gnu )}}\
%{static|static-libgcc|static-libgfortran:%:replace-outfile(-lgfortran
libgfortran.a%s)}\
+ %{static|static-libgcc|static-libquadmath:%:replace-outfile(-lquadmath
libquadmath.a%s)}\
%{static|static-libgcc|static-libphobos:%:replace-outfile(-lgphobos
libgphobos.a%s)}\
%{static|static-libgcc|static-libstdc++|static-libgfortran:%:replace-outfile(-lgomp
libgomp.a%s)}\
%{static|static-libgcc|static-libstdc++:%:replace-outfile(-lstdc++
libstdc++.a%s)}\
--- gcc/fortran/lang.opt.jj 2022-02-04 14:36:55.050604670 +0100
+++ gcc/fortran/lang.opt2022-08-16 14:52:52.459267705 +0200
@@ -863,6 +863,10 @@ static-libgfortran
Fortran
Statically link the GNU Fortran helper library (libgfortra
[PATCH] fortran: Add -static-libquadmath support [PR46539]
Hi!
The following patch is a revival of the
https://gcc.gnu.org/legacy-ml/gcc-patches/2014-10/msg00771.html
patch. While trunk configured against recent glibc and with linker
--as-needed support doesn't really need to link against -lquadmath
anymore, there are still other targets where libquadmath is still in
use.
As has been discussed, making -static-libgfortran imply statically
linking both libgfortran and libquadmath is undesirable because of
the significant licensing differences between the 2 libraries.
Compared to the 2014 patch, this one doesn't handle -lquadmath
addition in the driver, which to me looks incorrect, libgfortran
configure determines where in libgfortran.spec -lquadmath should
be present if at all and with what it should be wrapper, but
analyzes gfortran -### -static-libgfortran stderr and based on
that figures out what gcc/configure.ac determined.
So far slightly tested on x86_64-linux (and will bootstrap/regtest
it there tonight), but I unfortunately don't have a way to test it
e.g. on Darwin.
Thoughts on this?
2022-08-16 Francois-Xavier Coudert
Jakub Jelinek
PR fortran/46539
gcc/
* common.opt (static-libquadmath): New option.
* gcc.c (driver_handle_option): Always accept -static-libquadmath.
* config/darwin.h (LINK_SPEC): Handle -static-libquadmath.
gcc/fortran/
* lang.opt (static-libquadmath): New option.
* invoke.texi (-static-libquadmath): Document it.
* options.c (gfc_handle_option): Error out if -static-libquadmath
is passed but we do not support it.
libgfortran/
* acinclude.m4 (LIBQUADSPEC): From $FC -static-libgfortran -###
output determine -Bstatic/-Bdynamic, -bstatic/-bdynamic,
-aarchive_shared/-adefault linker support or Darwin remapping
of -lgfortran to libgfortran.a%s and use that around or instead
of -lquadmath in LIBQUADSPEC.
* configure: Regenerated.
--- gcc/common.opt.jj 2022-06-27 11:18:02.050066582 +0200
+++ gcc/common.opt 2022-08-16 14:51:04.611673800 +0200
@@ -3601,6 +3601,10 @@ static-libphobos
Driver
; Documented for D, but always accepted by driver.
+static-libquadmath
+Driver
+; Documented for Fortran, but always accepted by driver.
+
static-libstdc++
Driver
--- gcc/gcc.cc.jj 2022-08-11 09:57:24.765334380 +0200
+++ gcc/gcc.cc 2022-08-16 14:57:54.708327024 +0200
@@ -4585,12 +4585,14 @@ driver_handle_option (struct gcc_options
case OPT_static_libgcc:
case OPT_shared_libgcc:
case OPT_static_libgfortran:
+case OPT_static_libquadmath:
case OPT_static_libphobos:
case OPT_static_libstdc__:
/* These are always valid, since gcc.cc itself understands the
first two, gfortranspec.cc understands -static-libgfortran,
-d-spec.cc understands -static-libphobos, and g++spec.cc
-understands -static-libstdc++ */
+d-spec.cc understands -static-libphobos, g++spec.cc
+understands -static-libstdc++ and libgfortran.spec handles
+-static-libquadmath. */
validated = true;
break;
--- gcc/config/darwin.h.jj 2022-08-16 14:51:14.529544492 +0200
+++ gcc/config/darwin.h 2022-08-16 14:53:54.402460097 +0200
@@ -443,6 +443,7 @@ extern GTY(()) int darwin_ms_struct;
%:replace-outfile(-lobjc libobjc-gnu.a%s); \
:%:replace-outfile(-lobjc -lobjc-gnu )}}\
%{static|static-libgcc|static-libgfortran:%:replace-outfile(-lgfortran
libgfortran.a%s)}\
+ %{static|static-libgcc|static-libgfortran:%:replace-outfile(-lquadmath
libquadmath.a%s)}\
%{static|static-libgcc|static-libphobos:%:replace-outfile(-lgphobos
libgphobos.a%s)}\
%{static|static-libgcc|static-libstdc++|static-libgfortran:%:replace-outfile(-lgomp
libgomp.a%s)}\
%{static|static-libgcc|static-libstdc++:%:replace-outfile(-lstdc++
libstdc++.a%s)}\
--- gcc/fortran/lang.opt.jj 2022-02-04 14:36:55.050604670 +0100
+++ gcc/fortran/lang.opt2022-08-16 14:52:52.459267705 +0200
@@ -863,6 +863,10 @@ static-libgfortran
Fortran
Statically link the GNU Fortran helper library (libgfortran).
+static-libquadmath
+Fortran
+Statically link the GCC Quad-Precision Math Library (libquadmath).
+
std=f2003
Fortran
Conform to the ISO Fortran 2003 standard.
--- gcc/fortran/options.cc.jj 2022-01-18 11:58:59.568982256 +0100
+++ gcc/fortran/options.cc 2022-08-16 14:56:22.807525218 +0200
@@ -692,6 +692,13 @@ gfc_handle_option (size_t scode, const c
#endif
break;
+case OPT_static_libquadmath:
+#ifndef HAVE_LD_STATIC_DYNAMIC
+ gfc_fatal_error ("%<-static-libquadmath%> is not supported in this "
+ "configuration");
+#endif
+ break;
+
case OPT_fintrinsic_modules_path:
case OPT_fintrinsic_modules_path_:
--- gcc/fortran/invoke.texi.jj 2022-05-09 09:09:20.312473272 +0200
+++ gcc/fortran/invoke.texi 2022-08-16 16:12:47.638203577 +0200
@@ -17
Re: [PATCH] fortran: Expand ieee_arithmetic module's ieee_value inline [PR106579]
On Mon, Aug 15, 2022 at 10:00:02PM +0200, FX wrote:
> I have two questions, on this and the ieee_class patch:
>
>
> > + tree type = TREE_TYPE (arg);
> > + gcc_assert (TREE_CODE (type) == RECORD_TYPE);
> > + tree field = NULL_TREE;
> > + for (tree f = TYPE_FIELDS (type); f != NULL_TREE; f = DECL_CHAIN (f))
> > +if (TREE_CODE (f) == FIELD_DECL)
> > + {
> > + gcc_assert (field == NULL_TREE);
> > + field = f;
> > + }
> > + gcc_assert (field);
>
> Why looping over fields? The class type is a simple type with only one member
> (and it should be an integer, we can assert that).
I wanted to make sure it has exactly one field.
The ieee_arithmetic.F90 module in libgfortran surely does that, but I've
been worrying about some user overriding that module with something
different. At least in the C/C++ FEs we had in the past tons of bugs filed
for when some builtin made some assumptions about some headers and data
types in those and then somebody running a testcase that violated that.
Even failed gcc_assertion isn't the best answer to that, ideally one would
verify that upfront and then either error, sorry or ignore the call (leave
it as is). In that last case, it might be better to do the check on the
gfortran FE types instead of trees (verify the return type or second
argument type is ieee_class_type derived type with a single integral
(hidden) field).
> > + case IEEE_POSITIVE_ZERO:
> > + /* Make this also the default: label. */
> > + label = gfc_build_label_decl (NULL_TREE);
> > + tmp = build_case_label (NULL_TREE, NULL_TREE, label);
> > + gfc_add_expr_to_block (, tmp);
> > + real_from_integer (, TYPE_MODE (type), 0, SIGNED);
> > + break;
>
> Do we need a default label? It’s not like this is a more likely case than
> anything else…
The libgfortran version had default: label:
switch (type) \
{ \
case IEEE_SIGNALING_NAN: \
return __builtin_nans ## SUFFIX (""); \
case IEEE_QUIET_NAN: \
return __builtin_nan ## SUFFIX (""); \
case IEEE_NEGATIVE_INF: \
return - __builtin_inf ## SUFFIX (); \
case IEEE_NEGATIVE_NORMAL: \
return -42; \
case IEEE_NEGATIVE_DENORMAL: \
return -(GFC_REAL_ ## TYPE ## _TINY) / 2; \
case IEEE_NEGATIVE_ZERO: \
return -(GFC_REAL_ ## TYPE) 0; \
case IEEE_POSITIVE_ZERO: \
return 0; \
case IEEE_POSITIVE_DENORMAL: \
return (GFC_REAL_ ## TYPE ## _TINY) / 2; \
case IEEE_POSITIVE_NORMAL: \
return 42; \
case IEEE_POSITIVE_INF: \
return __builtin_inf ## SUFFIX (); \
default: \
return 0; \
} \
and I've tried to traslate that into what it generates.
There is at least the IEEE_OTHER_VALUE (aka 0) value
that isn't covered in the switch, but it is just an integer
under the hood, so it could have any other value.
Jakub
