Re: [PATCH RESEND 1/1] p1689r5: initial support
On Tue, Oct 04, 2022 at 21:12:03 +0200, Harald Anlauf wrote:
> Am 04.10.22 um 17:12 schrieb Ben Boeckel:
> > This patch implements support for [P1689R5][] to communicate to a build
> > system the C++20 module dependencies to build systems so that they may
> > build `.gcm` files in the proper order.
>
> Is there a reason that you are touching so many frontends?
Just those that needed the update for `cpp_finish`. It does align with
those that will (eventually) need this support anyways (AFAIK).
> > diff --git a/gcc/fortran/cpp.cc b/gcc/fortran/cpp.cc
> > index 364bd0d2a85..0b9df9c02cd 100644
> > --- a/gcc/fortran/cpp.cc
> > +++ b/gcc/fortran/cpp.cc
> > @@ -712,7 +712,7 @@ gfc_cpp_done (void)
> > FILE *f = fopen (gfc_cpp_option.deps_filename, "w");
> > if (f)
> > {
> > - cpp_finish (cpp_in, f);
> > + cpp_finish (cpp_in, f, NULL);
> > fclose (f);
> > }
> > else
> > @@ -721,7 +721,7 @@ gfc_cpp_done (void)
> > xstrerror (errno));
> > }
> > else
> > - cpp_finish (cpp_in, stdout);
> > + cpp_finish (cpp_in, stdout, NULL);
> > }
> >
> > cpp_undef_all (cpp_in);
>
> Couldn't you simply default the third argument of cpp_finish() to NULL?
I could do that. Wasn't sure how much that would be acceptable given
that it is a "silent" change, but it would reduce the number of files
touched here.
Thanks,
--Ben
Re: [PATCH RESEND 1/1] p1689r5: initial support
On Mon, Oct 10, 2022 at 17:04:09 -0400, Jason Merrill wrote:
> On 10/4/22 11:12, Ben Boeckel wrote:
> > This patch implements support for [P1689R5][] to communicate to a build
> > system the C++20 module dependencies to build systems so that they may
> > build `.gcm` files in the proper order.
>
> Thanks!
>
> > Support is communicated through the following three new flags:
> >
> > - `-fdeps-format=` specifies the format for the output. Currently named
> >`p1689r5`.
> >
> > - `-fdeps-file=` specifies the path to the file to write the format to.
>
> Do you expect users to want to emit Makefile (-MM) and P1689
> dependencies from the same compilation?
Yes, the build system wants to know what files affect scanning as well
(e.g., `#include ` is still possible and if it changes, this
operation should be performed again. The `-M` flags do this quite nicely
already :) .
> > - `-fdep-output=` specifies the `.o` that will be written for the TU
> >that is scanned. This is required so that the build system can
> >correlate the dependency output with the actual compilation that will
> >occur.
>
> The dependency machinery already needs to be able to figure out the name
> of the output file, can't we reuse that instead of specifying it on the
> command line?
This is how it determines the output of the compilation. Because it is
piggy-backing on the `-E` flag, the `-o` flag specifies the output of
the preprocessed source (purely a side-effect right now).
> > diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> > index 2db1e9cbdfb..90787230a9e 100644
> > --- a/libcpp/include/cpplib.h
> > +++ b/libcpp/include/cpplib.h
> > @@ -298,6 +298,9 @@ typedef CPPCHAR_SIGNED_T cppchar_signed_t;
> > /* Style of header dependencies to generate. */
> > enum cpp_deps_style { DEPS_NONE = 0, DEPS_USER, DEPS_SYSTEM };
> >
> > +/* Format of header dependencies to generate. */
> > +enum cpp_deps_format { DEPS_FMT_NONE = 0, DEPS_FMT_P1689R5 };
>
> Why not add this to cpp_deps_style?
It is orthogonal. The `-M` flags and `-fdeps-*` flags are similar, but
`-fdeps-*` dependencies are fundamentally different than `-M`
dependencies. The comment does need updated though.
`-M` is about discovered dependencies: those that you find out while
doing work. `-fdep-*` is about ordering dependencies: extracting
information from file content in order to even order future work around.
It stems from the loss of embarassing parallelism when building C++20
code that uses `import`. It's isomorphic to the Fortran module
compilation ordering problem.
> > @@ -387,7 +410,7 @@ make_write_vec (const mkdeps::vec &vec,
> > FILE *fp,
> > .PHONY targets for all the dependencies too. */
> >
> > static void
> > -make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> > +make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax, int
> > extra)
>
> Instead of adding the "extra" parameter...
Hmm. Not sure why I had named this so poorly. Maybe this comes from my
initial stab at this functionality in 2019 (the format has been hammered
out in ISO C++'s SG15 since then).
> > {
> > const mkdeps *d = pfile->deps;
> >
> > @@ -398,7 +421,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned
> > int colmax)
> > if (d->deps.size ())
> > {
> > column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
> > - if (CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> > + if (extra && CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> > column = make_write_name (d->cmi_name, fp, column, colmax);
> > fputs (":", fp);
> > column++;
> > @@ -412,7 +435,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned
> > int colmax)
> > if (!CPP_OPTION (pfile, deps.modules))
> > return;
>
> ...how about checking CPP_OPTION for p1689r5 mode here?
I'll take a look at this.
> >
> > - if (d->modules.size ())
> > + if (extra && d->modules.size ())
> > {
> > column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
> > if (d->cmi_name)
> > @@ -423,7 +446,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned
> > int colmax)
> > fputs ("\n", fp);
> > }
> >
> > - if (d->module_name)
> > + if (extra && d->module_name)
> > {
> > if (d->cmi_name)
> > {
> > @@ -455,7 +478,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned
> > int colmax)
> > }
> > }
> >
> > - if (d->modules.size ())
> > + if (extra && d->modules.size ())
> > {
> > column = fprintf (fp, "CXX_IMPORTS +=");
> > make_write_vec (d->modules, fp, column, colmax, 0, ".c++m");
> > @@ -468,9 +491,203 @@ make_write (const cpp_reader *pfile, FILE *fp,
> > unsigned int colmax)
> > /* Really we should be opening fp here. */
> >
> > void
> > -deps_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> > +deps_write (const struct cpp_reader *pfi
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 @@ cleanup
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 2/5] [gfortran] Translate allocate directive (OpenMP 5.0).
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
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
on the deallocation TU and only slow down -fopenmp compiled code)?
The problem with storing is that gfortran inserts the malloc/realloc/free calls
directly, i.e. without library preloading, intercepting those libcalls, I do
not see how it can work at all.
I also do not know how to handle the pinned-memory case above correctly, either.
One partial support would be requiring that the code using allocatables cannot
do any reallocation/deallocation by only permitting calls to procedures which
do not permit allocatables. (Such that no reallocation can happen.) – And print
a 'sorry' for the rest.
Other implementations seem to have a Fortran library call for (re)allocations,
which permits to swap the allocator from the generic one to the
omp_default_mem_alloc.
* * *
In terms of the array descriptor, we have inside 'struct dtype_type' the
'signed short attribute', which currently only h
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
> on the deallocation TU and only slow down -fopenmp compiled code)?
>
> The problem with storing is that gfortran inserts the malloc/realloc/free
> calls directly, i.e. without library preloading, intercepting those libcalls,
> I do not see how it can work at all.
Well, it can use a weak symbol, if not linked against libgo
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 11.10.22 16:15, Jakub Jelinek wrote: 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? Here, I was only talking about the most common usage case, with the assumption that the user code does not cause any reallocation. I later talked about accepting only code which cannot cause reallocation (compile-time check of the code contained in the scope). Thus, a 'call foo(a)' would be fine, but not for ... Anyway, even this simple case has the problem that one can have subroutine foo (var) integer, allocatable:: var(:) a 'foo' that has an 'allocatable' attribute for the dummy argument. I think in the common case, it has not – such that most code can run w/o running into this issue. However, for code like type t real, allocatable :: x(:), y(:), z(:) end type t type(t) :: var !$omp allocators(my_alloc) allocate(var%x(N), var%y(N), var%z(N)) call bar(var%x) call foo(var) it is more difficult: 'bar' works (if its dummy argument is not 'allocatable') but for 'foo', the (re|de)allocation cannot be ruled out. Thus, we always have to 'sorry' for such a code – and I fear it could be somewhat common. 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.? As mentioned, we could use the 'dtype.attribute' field which is currently not really used – and if, only 2 of the 16 bits are used. But you are right that for scalar allocatables, we do not use array descriptors (except with BIND(C)). Hmm. For allocatable components, the same applied: If arrays, then there is an array descriptor – for scalars, there isn't. (And storing the length of a scalar character string with deferred length uses an aux variable + has lots of bugs.) 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. I think we could do tricks with scalar allocatable variable – but it will be more complicated with scalar allocatable components. Hmm. 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. The problem of making pseudo-USM work is that it has to be applied to all (stack,heap) memory – which implies that all code using malloc/free needs to be either call the GOMP version or the GLIBC version, but shall not mix one or the other. – Thus, calling some library or any other file that was not compiled with -f... will have issues with malloc/free. Another issue is that variables not allocated via GOMP_* will not be accessible on the device in that case. Tobias - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
[PATCH] Fortran: check types of source expressions before conversion [PR107215]
Dear all,
this PR is an obvious followup to PR107000, where invalid
types appeared in array constructors and lead to an ICE
either in a conversion or reduction of a unary or binary
expression.
The present PR shows that several other conversions need to
be protected by a check of the type of the source expression.
Regtested on x86_64-pc-linux-gnu. OK for mainline?
Thanks,
Harald
From 87dae7eb9d4cc76060d258ba99bc53f62c7130f2 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Tue, 11 Oct 2022 20:37:42 +0200
Subject: [PATCH] Fortran: check types of source expressions before conversion
[PR107215]
gcc/fortran/ChangeLog:
PR fortran/107215
* arith.cc (gfc_int2int): Check validity of type of source expr.
(gfc_int2real): Likewise.
(gfc_int2complex): Likewise.
(gfc_real2int): Likewise.
(gfc_real2real): Likewise.
(gfc_complex2int): Likewise.
(gfc_complex2real): Likewise.
(gfc_complex2complex): Likewise.
(gfc_log2log): Likewise.
(gfc_log2int): Likewise.
(gfc_int2log): Likewise.
gcc/testsuite/ChangeLog:
PR fortran/107215
* gfortran.dg/pr107215.f90: New test.
---
gcc/fortran/arith.cc | 33 ++
gcc/testsuite/gfortran.dg/pr107215.f90 | 17 +
2 files changed, 50 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/pr107215.f90
diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
index 086b1f856b1..9e079e42995 100644
--- a/gcc/fortran/arith.cc
+++ b/gcc/fortran/arith.cc
@@ -2040,6 +2040,9 @@ gfc_int2int (gfc_expr *src, int kind)
gfc_expr *result;
arith rc;
+ if (src->ts.type != BT_INTEGER)
+return NULL;
+
result = gfc_get_constant_expr (BT_INTEGER, kind, &src->where);
mpz_set (result->value.integer, src->value.integer);
@@ -2085,6 +2088,9 @@ gfc_int2real (gfc_expr *src, int kind)
gfc_expr *result;
arith rc;
+ if (src->ts.type != BT_INTEGER)
+return NULL;
+
result = gfc_get_constant_expr (BT_REAL, kind, &src->where);
mpfr_set_z (result->value.real, src->value.integer, GFC_RND_MODE);
@@ -2116,6 +2122,9 @@ gfc_int2complex (gfc_expr *src, int kind)
gfc_expr *result;
arith rc;
+ if (src->ts.type != BT_INTEGER)
+return NULL;
+
result = gfc_get_constant_expr (BT_COMPLEX, kind, &src->where);
mpc_set_z (result->value.complex, src->value.integer, GFC_MPC_RND_MODE);
@@ -2150,6 +2159,9 @@ gfc_real2int (gfc_expr *src, int kind)
arith rc;
bool did_warn = false;
+ if (src->ts.type != BT_REAL)
+return NULL;
+
result = gfc_get_constant_expr (BT_INTEGER, kind, &src->where);
gfc_mpfr_to_mpz (result->value.integer, src->value.real, &src->where);
@@ -2196,6 +2208,9 @@ gfc_real2real (gfc_expr *src, int kind)
arith rc;
bool did_warn = false;
+ if (src->ts.type != BT_REAL)
+return NULL;
+
result = gfc_get_constant_expr (BT_REAL, kind, &src->where);
mpfr_set (result->value.real, src->value.real, GFC_RND_MODE);
@@ -2310,6 +2325,9 @@ gfc_complex2int (gfc_expr *src, int kind)
arith rc;
bool did_warn = false;
+ if (src->ts.type != BT_COMPLEX)
+return NULL;
+
result = gfc_get_constant_expr (BT_INTEGER, kind, &src->where);
gfc_mpfr_to_mpz (result->value.integer, mpc_realref (src->value.complex),
@@ -2372,6 +2390,9 @@ gfc_complex2real (gfc_expr *src, int kind)
arith rc;
bool did_warn = false;
+ if (src->ts.type != BT_COMPLEX)
+return NULL;
+
result = gfc_get_constant_expr (BT_REAL, kind, &src->where);
mpc_real (result->value.real, src->value.complex, GFC_RND_MODE);
@@ -2439,6 +2460,9 @@ gfc_complex2complex (gfc_expr *src, int kind)
arith rc;
bool did_warn = false;
+ if (src->ts.type != BT_COMPLEX)
+return NULL;
+
result = gfc_get_constant_expr (BT_COMPLEX, kind, &src->where);
mpc_set (result->value.complex, src->value.complex, GFC_MPC_RND_MODE);
@@ -2504,6 +2528,9 @@ gfc_log2log (gfc_expr *src, int kind)
{
gfc_expr *result;
+ if (src->ts.type != BT_LOGICAL)
+return NULL;
+
result = gfc_get_constant_expr (BT_LOGICAL, kind, &src->where);
result->value.logical = src->value.logical;
@@ -2518,6 +2545,9 @@ gfc_log2int (gfc_expr *src, int kind)
{
gfc_expr *result;
+ if (src->ts.type != BT_LOGICAL)
+return NULL;
+
result = gfc_get_constant_expr (BT_INTEGER, kind, &src->where);
mpz_set_si (result->value.integer, src->value.logical);
@@ -2532,6 +2562,9 @@ gfc_int2log (gfc_expr *src, int kind)
{
gfc_expr *result;
+ if (src->ts.type != BT_INTEGER)
+return NULL;
+
result = gfc_get_constant_expr (BT_LOGICAL, kind, &src->where);
result->value.logical = (mpz_cmp_si (src->value.integer, 0) != 0);
diff --git a/gcc/testsuite/gfortran.dg/pr107215.f90 b/gcc/testsuite/gfortran.dg/pr107215.f90
new file mode 100644
index 000..2c2a0ca7502
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr107215.f90
@@ -0,0 +1,17 @@
+! { dg-do compile }
+! PR fortran/107215 - ICE in gfc_real2real and gfc_complex2complex
+! Contributed by G.Steinmetz
+
+program p
+ double precision, parameter :: z = 1.0
Re: [PATCH] Fortran: check types of source expressions before conversion [PR107215]
Le 11/10/2022 à 20:47, Harald Anlauf via Fortran a écrit : Dear all, this PR is an obvious followup to PR107000, where invalid types appeared in array constructors and lead to an ICE either in a conversion or reduction of a unary or binary expression. The present PR shows that several other conversions need to be protected by a check of the type of the source expression. Regtested on x86_64-pc-linux-gnu. OK for mainline? OK, thanks.
[PATCH] Fortran: check types of operands of arithmetic binary operations [PR107217]
Dear all,
we need to check that the operands of arithmetic binary operations
are consistent and of numeric type.
The PR reported an issue for multiplication ("*"), but we better
extend this to the other binary operations.
I chose the following solution:
- consistent types for +,-,*,/, keeping an internal error if any
unhandled type shows up,
- numeric types for **
Regtested on x86_64-pc-linux-gnu. OK for mainline?
Thanks,
Harald
From a95f251504bcb8ba28b7db1d2b7990631c761e9c Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Tue, 11 Oct 2022 22:08:48 +0200
Subject: [PATCH] Fortran: check types of operands of arithmetic binary
operations [PR107217]
gcc/fortran/ChangeLog:
PR fortran/107217
* arith.cc (gfc_arith_plus): Compare consistency of types of operands.
(gfc_arith_minus): Likewise.
(gfc_arith_times): Likewise.
(gfc_arith_divide): Likewise.
(arith_power): Check that both operands are of numeric type.
gcc/testsuite/ChangeLog:
PR fortran/107217
* gfortran.dg/pr107217.f90: New test.
---
gcc/fortran/arith.cc | 15 +++
gcc/testsuite/gfortran.dg/pr107217.f90 | 18 ++
2 files changed, 33 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/pr107217.f90
diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
index 9e079e42995..14ba931e37f 100644
--- a/gcc/fortran/arith.cc
+++ b/gcc/fortran/arith.cc
@@ -624,6 +624,9 @@ gfc_arith_plus (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp)
gfc_expr *result;
arith rc;
+ if (op1->ts.type != op2->ts.type)
+return ARITH_INVALID_TYPE;
+
result = gfc_get_constant_expr (op1->ts.type, op1->ts.kind, &op1->where);
switch (op1->ts.type)
@@ -658,6 +661,9 @@ gfc_arith_minus (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp)
gfc_expr *result;
arith rc;
+ if (op1->ts.type != op2->ts.type)
+return ARITH_INVALID_TYPE;
+
result = gfc_get_constant_expr (op1->ts.type, op1->ts.kind, &op1->where);
switch (op1->ts.type)
@@ -692,6 +698,9 @@ gfc_arith_times (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp)
gfc_expr *result;
arith rc;
+ if (op1->ts.type != op2->ts.type)
+return ARITH_INVALID_TYPE;
+
result = gfc_get_constant_expr (op1->ts.type, op1->ts.kind, &op1->where);
switch (op1->ts.type)
@@ -727,6 +736,9 @@ gfc_arith_divide (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp)
gfc_expr *result;
arith rc;
+ if (op1->ts.type != op2->ts.type)
+return ARITH_INVALID_TYPE;
+
rc = ARITH_OK;
result = gfc_get_constant_expr (op1->ts.type, op1->ts.kind, &op1->where);
@@ -815,6 +827,9 @@ arith_power (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp)
gfc_expr *result;
arith rc;
+ if (!gfc_numeric_ts (&op1->ts) || !gfc_numeric_ts (&op2->ts))
+return ARITH_INVALID_TYPE;
+
rc = ARITH_OK;
result = gfc_get_constant_expr (op1->ts.type, op1->ts.kind, &op1->where);
diff --git a/gcc/testsuite/gfortran.dg/pr107217.f90 b/gcc/testsuite/gfortran.dg/pr107217.f90
new file mode 100644
index 000..9c8492e64f0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr107217.f90
@@ -0,0 +1,18 @@
+! { dg-do compile }
+! PR fortran/107217 - ICE in gfc_arith_times
+! Contributed by G.Steinmetz
+
+program p
+ print *, [real :: (['1'])] * 2 ! { dg-error "Cannot convert" }
+ print *, 2 * [real :: (['1'])] ! { dg-error "Cannot convert" }
+ print *, [real :: (['1'])] + 2 ! { dg-error "Cannot convert" }
+ print *, [real :: (['1'])] - 2 ! { dg-error "Cannot convert" }
+ print *, [real :: (['1'])] / 2 ! { dg-error "Cannot convert" }
+ print *, 1 / [real :: (['1'])] ! { dg-error "Cannot convert" }
+ print *, [real :: (['1'])] ** 2 ! { dg-error "Cannot convert" }
+ print *, 2 ** [real :: (['1'])] ! { dg-error "Cannot convert" }
+ print *, 2.0 ** [real :: (.true.)] ! { dg-error "Cannot convert" }
+ print *, [real :: (.true.)] ** 2.0 ! { dg-error "Cannot convert" }
+ print *, [complex :: (['1'])] ** (1.0,2.0) ! { dg-error "Cannot convert" }
+ print *, (1.0,2.0) ** [complex :: (['1'])] ! { dg-error "Cannot convert" }
+end
--
2.35.3
