Re: [PATCH, Fortran] Skip gfortran.dg/PR100914.f90 on targets that don't provide quadmath.h
On Mon, Sep 6, 2021 at 7:21 AM Sandra Loosemore wrote: > On 9/5/21 7:29 PM, H.J. Lu wrote: > > On Sun, Sep 5, 2021 at 11:02 AM Sandra Loosemore > > wrote: > >> > >> On 9/5/21 7:31 AM, H.J. Lu wrote: > >>> On Sat, Sep 4, 2021 at 7:31 PM Sandra Loosemore < > san...@codesourcery.com> wrote: > > The testcase gfortran.dg/PR100914.f90 that I recently checked in > (originally written by José Rui Faustino de Sousa) depends on the > header file to obtain a typedef for __complex128. It > appears not to be possible to define an equivalent type in a portable > way in the testcase itself (see > https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Floating-Types.html) so > this patch skips the test entirely on targets where quadmath.h is not > available. > > The target-supports.exp change was cut-and-pasted from similar code in > that file, but I haven't figured out how to test this change in a > build > that doesn't provide quadmath.h (e.g., my aarch64-linux-gnu toolchain > build attempt croaked with an unrelated compilation error in glibc). > Perhaps someone who previously encountered the FAILs on this testcase > can confirm that it's skipped with this change? > >>> > >>> Since libqaudmath provides , I prefer either of 2 patches > >>> enclosed here. > >> > >> Of these two, the first one looks better to me, and seems to work OK in > >> my x86 build. But, I'm not sure it is the right thing for ARM/Aarch64 > >> targets, which apparently have _Float128 support without the __float128 > >> type or libquadmath. It's pretty clear quadmath.h depends on having > >> __float128. > > > > The only used by GCC is the one in libquadmath. I > > will check in my first patch tomorrow if there are no objections. > > > >> See Christophe's mail here: > >> > >> https://gcc.gnu.org/pipermail/fortran/2021-September/056467.html > >> > >> As I said in my last mail, I ran into some problems getting an aarch64 > >> toolchain built so I haven't been able to do any testing or experiments > >> on that target myself yet. :-( > > I finally got an aarch64-linux-gnu toolchain built and confirmed that it > is still broken there: it indeed does not define __float128, and > including quadmath.h results in a gazillion errors like > > /path/to/quadmath.h:47:8: error: unknown type name '__float128' > > I also observed that _Float128 is the same type as long double on this > target. > > Unless the aarch64 maintainers think it is a bug that __float128 is not > supported, I think the right solution here is the one I was thinking of > previously, to fix the Fortran front end to tie the C_FLOAT128 kind to > _Float128 rather than __float128, and fix the runtime support and test > cases accordingly. Then there should be no need to depend on quadmath.h > at all. C_FLOAT128 is a GNU extension and _Float128 is supported on a > superset of targets that __float128 is, so there should be no issue with > backward compatibility. > > I'm not really in a position to make a comment, but any patch aiming at skipping pr100914.f90 only will have no effect on the other errors I reported, which are related to __float128 vs _Float128. Christophe > -Sandra >
Re: [PATCH, Fortran] Skip gfortran.dg/PR100914.f90 on targets that don't provide quadmath.h
Hi Sandra, hi all,
On 06.09.21 07:20, Sandra Loosemore wrote:
On 9/5/21 7:29 PM, H.J. Lu wrote:
On Sun, Sep 5, 2021 at 11:02 AM Sandra Loosemore
wrote:
On 9/5/21 7:31 AM, H.J. Lu wrote:
On Sat, Sep 4, 2021 at 7:31 PM Sandra Loosemore
wrote:
The testcase gfortran.dg/PR100914.f90 that I recently checked in
(originally written by José Rui Faustino de Sousa) depends on the
header file to obtain a typedef for __complex128.
https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Floating-Types.html) so
this patch skips the test entirely on targets where quadmath.h is not
available.
Can't we just use the following in the testcase?
typedef _Complex _Float128 __complex128;
That avoids the including quadmath.h – and my impression is
that the only reason for including quadmath.h is this single
function. Or does this run into issues with the proper mode?
At least when I tried it, it did compile on x86-64. On the
other hand, there is the following remark in the header file:
/* Define the complex type corresponding to __float128
("_Complex __float128" is not allowed) */
#if (!defined(_ARCH_PPC)) || defined(__LONG_DOUBLE_IEEE128__)
typedef _Complex float __attribute__((mode(TC))) __complex128;
#else
typedef _Complex float __attribute__((mode(KC))) __complex128;
#endif
The '#else' was added with
https://gcc.gnu.org/g:0c949f0a1ce9cfa8c48e62628493140d60e65ea7
for https://gcc.gnu.org/PR81848
The comment about _Complex __float128 was in the original patch.
See Christophe's mail here:
https://gcc.gnu.org/pipermail/fortran/2021-September/056467.html
As I said in my last mail, I ran into some problems getting an aarch64
toolchain built so I haven't been able to do any testing or experiments
on that target myself yet. :-(
I finally got an aarch64-linux-gnu toolchain built and confirmed that
it is still broken there: it indeed does not define __float128, and
including quadmath.h results in a gazillion errors like
/path/to/quadmath.h:47:8: error: unknown type name '__float128'
I also observed that _Float128 is the same type as long double on this
target.
Which means that it is pointless to provide libquadmath on this target
as as the libc's long double math functions are sufficient. – Or not, I
recall that that on PowerPC libquadmath has a effective higher precision
than libc's long double. But in terms of checking type =
Unless the aarch64 maintainers think it is a bug that __float128 is
not supported, I think the right solution here is the one I was
thinking of previously, to fix the Fortran front end to tie the
C_FLOAT128 kind to _Float128 rather than __float128, and fix the
runtime support and test cases accordingly.
Whorks for me – I assume that includes updating the documentation. As on
many systems __float128 and _Float128 is available, I don't think it has
to be mentioned in the release notes (but it can).
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
[Patch] Fortran: Fix Bind(C) Array-Descriptor Conversion (Move to Front-End Code)
Hi all,
gfortran's internal array descriptor (xgfc descriptor) and
the descriptor used with BIND(C) (CFI descriptor, ISO_Fortran_binding.h
of TS29113 / Fortran 2018) are different. Thus, when calling a BIND(C)
procedure the gfc descriptor has to be converted to cfi – and when a
BIND(C) procedure is implemented in Fortran, the argument has to be
converted back from CFI to gfc.
The current implementation handles part in the FE and part in libgfortran,
but there were several issues, e.g. PR101635 failed due to alias issues,
debugging wasn't working well, uninitialized memory was used in some cases
etc.
This patch now moves descriptor conversion handling to the FE – which also
can make use of compile-time knowledge, useful both for diagnostic and to
optimize the code.
Additionally:
- Some cases where TS29113 mandates that the array descriptor should be
used now use the array descriptor, in particular character scalars with
'len=*' and allocatable/pointer scalars.
- While debugging the alias issue, I simplified 'select rank'. While some
special case is needed for assumed-shape arrays, those cannot appear when
the argument has the pointer or allocatable attribute. That's not only a
missed optimization, pointer/allocatable arrays can also be NULL - such
that accessing desc->dim.ubound[rank-1] can be uninitialized memory ...
OK? Comments? Suggestions?
* * *
For some more dumps, see the discussion about the alias issue at:
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578364.html
("[RFH] ME optimizes variable assignment away / Fortran bind(C) descriptor
conversion")
plus the original emails:
- https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578271.html
- and (correct dump)
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578274.html
Debugging - not ideal but not too bad either. For
subroutine f(x) bind(C)
integer :: x(:)
with an uninitialized size-4 array as argument:
m::f (_x=...) at foo4.f90:3
3 subroutine f(x) bind(C)
(gdb) p x
Cannot access memory at address 0x38
(gdb) p _x
$6 = ( base_addr = 0x7fffe2c0, elem_len = 4, version = 1, rank = 1 '\001',
attribute = 2 '\002', type = 1025, dim = (( lower_bound = 0, extent = 5, sm = 4
)) )
(gdb) s
5 x(1) = 5
(gdb) p x
$7 = (0, 0, 0, -670762413, 0)
Tobias
PS: This patch fixes but not necessarily fully the following PRs:
PR fortran/102086 - [F2008][TS29113] Accepts invalid scalar TYPE(*) as actual
argument to assumed-rank
PR fortran/92189 - Fortran-written bind(C) function with allocatable argument
does not update C descriptor on exit
PR fortran/92621 - Problems with memory handling with allocatable intent(out)
arrays with bind(c)
PR fortran/101308 - Bind(C): gfortran does not create C descriptors for scalar
pointer/allocatable arguments
PR fortran/101635 - FAIL: gfortran.dg/PR93963.f90 – alias-handling issue with
BIND(C)'s _gfortran_cfi_desc_to_gfc_desc
PR fortran/92482 - BIND(C) with array-descriptor mishandled for type character
and possibly some more.
PPS: I should add some additional testcases – I try to do this as Part 2 of
this patch.
PPPS: Once the patch is in, some audit needs to be done which parts of those
PRs remain
as follow-up work. I think some still existing issues are covered by José's
pending
patches + for those which are now fixed, the testcase might still be added.
-
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: Fix Bind(C) Array-Descriptor Conversion
gfortran uses internally a different array descriptor ("gfc") as
Fortran 2018 alias TS291113 defines for C interoperability via
ISO_Fortran_binding.h ("CFI"). Hence, when calling a C function
from Fortran, it has to be converted in the callee - and if a
BIND(C) procedure is written in Fortran, the CFI argument has
to be converted to gfc in order work with the rest of the FE
code and the library calls.
Before this patch, part was handled in the FE generated code and
other parts in libgfortran. With this patch, all code is generated
and CFI is defined as proper type - visible in the debugger and to
the middle end - avoiding both alias issues and missed optimization
issues.
This patch also fixes issues like: intent(out) deallocation in
the bind(C) callee, using the CFI descriptor also for allocatable
and pointer scalars and for len=* character strings.
For 'select rank', it also optimizes the code + avoid accessing
uninitialized memory if the dummy argument is allocatable/a pointer.
It additionally rejects passing a descriptorless type(*) to an
assumed-rank dummy argument. [F2018:C711]
PR fortran/102086
PR fortran/92189
PR fortran/92621
PR fortran/101308
PR fortran/101635
PR fortran/92482
gcc/fortran/ChangeLog:
* decl.c (gfc_verify_c_interop_param): Remove 'sorry' for
scalar allocatable/po
[Patch] C, C++, Fortran, OpenMP: Add support for 'flush seq_cst' construct
Hi,
this patch adds support for the 'seq_cst' memory order clause on the
'flush' directive which was introduced in OpenMP 5.1 (p.275ff of the
OpenMP 5.1 Specification):
"If neither memory-order-clause nor a list appears on the flush
construct then the behavior is as if memory-order-clause is seq_cst.
A flush construct with the seq_cst clause, executed on a given thread,
operates as if all data storage blocks that are accessible to the thread
are flushed by a strong flush operation.
...
An implementation may implement a flush construct with a list by
ignoring the list and treating it the same as a flush construct with the
seq_cst clause."
I am not completely sure about the correct memory model specification:
"MEMMODEL_SYNC_SEQ_CST" vs. "MEMMODEL_SEQ_CST".
As "MEMMODEL_SYNC_SEQ_CST" is already used for flush without a clause
(that should behave in the same way than using seq_cst), see
expand_builtin_sync_synchronize in gcc/builtins.c, and regarding the
discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697 I found
it appropriate to use "MEMMODEL_SYNC_SEQ_CST" in order to guarantee a
strong flush.
I tested on x86_64-linux with nvptx offloading with no regressions.
Marcel
-
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
C, C++, Fortran, OpenMP: Add support for 'flush seq_cst' construct.
This patch adds support for the 'seq_cst' memory order clause on the 'flush'
directive which was introduced in OpenMP 5.1.
gcc/c-family/ChangeLog:
* c-omp.c (c_finish_omp_flush): Handle MEMMODEL_SEQ_CST.
gcc/c/ChangeLog:
* c-parser.c (c_parser_omp_flush): Parse 'seq_cst' clause on 'flush'
directive.
gcc/cp/ChangeLog:
* parser.c (cp_parser_omp_flush): Parse 'seq_cst' clause on 'flush'
directive.
* semantics.c (finish_omp_flush): Handle MEMMODEL_SEQ_CST.
gcc/fortran/ChangeLog:
* openmp.c (gfc_match_omp_flush): Parse 'seq_cst' clause on 'flush'
directive.
* trans-openmp.c (gfc_trans_omp_flush): Handle OMP_MEMORDER_SEQ_CST.
gcc/testsuite/ChangeLog:
* c-c++-common/gomp/flush-1.c: Add test case for 'seq_cst'.
* c-c++-common/gomp/flush-2.c: Add test case for 'seq_cst'.
* g++.dg/gomp/attrs-1.C: Adapt test to handle all flush clauses.
* gfortran.dg/gomp/flush-1.f90: Add test case for 'seq_cst'.
* gfortran.dg/gomp/flush-2.f90: Add test case for 'seq_cst'.
diff --git a/gcc/c-family/c-omp.c b/gcc/c-family/c-omp.c
index 18de7e4..4b95fc1 100644
--- a/gcc/c-family/c-omp.c
+++ b/gcc/c-family/c-omp.c
@@ -606,7 +606,7 @@ c_finish_omp_flush (location_t loc, int mo)
{
tree x;
- if (mo == MEMMODEL_LAST)
+ if (mo == MEMMODEL_LAST || mo == MEMMODEL_SEQ_CST)
{
x = builtin_decl_explicit (BUILT_IN_SYNC_SYNCHRONIZE);
x = build_call_expr_loc (loc, x, 0);
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 3b1d10f..4d074ec 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -18339,7 +18339,9 @@ c_parser_omp_flush (c_parser *parser)
const char *p
= IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
- if (!strcmp (p, "acq_rel"))
+ if (!strcmp (p, "seq_cst"))
+ mo = MEMMODEL_SEQ_CST;
+ else if (!strcmp (p, "acq_rel"))
mo = MEMMODEL_ACQ_REL;
else if (!strcmp (p, "release"))
mo = MEMMODEL_RELEASE;
@@ -18347,7 +18349,8 @@ c_parser_omp_flush (c_parser *parser)
mo = MEMMODEL_ACQUIRE;
else
error_at (c_parser_peek_token (parser)->location,
- "expected %, % or %");
+ "expected %, %, % or "
+ "%");
c_parser_consume_token (parser);
}
if (c_parser_next_token_is (parser, CPP_OPEN_PAREN))
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index ea71f9c..f9c2c8a 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -40742,7 +40742,9 @@ cp_parser_omp_flush (cp_parser *parser, cp_token
*pragma_tok)
{
tree id = cp_lexer_peek_token (parser->lexer)->u.value;
const char *p = IDENTIFIER_POINTER (id);
- if (!strcmp (p, "acq_rel"))
+ if (!strcmp (p, "seq_cst"))
+ mo = MEMMODEL_SEQ_CST;
+ else if (!strcmp (p, "acq_rel"))
mo = MEMMODEL_ACQ_REL;
else if (!strcmp (p, "release"))
mo = MEMMODEL_RELEASE;
@@ -40750,7 +40752,8 @@ cp_parser_omp_flush (cp_parser *parser, cp_token
*pragma_tok)
mo = MEMMODEL_ACQUIRE;
else
error_at (cp_lexer_peek_token (parser->lexer)->location,
- "expected %, % or %");
+ "expected %, %, % or "
+ "%");
cp_lexer_consume_token (parser->lexer);
}
if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_PAREN))
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index f4b042f..8b78e89
Re: [PATCH, Fortran] Skip gfortran.dg/PR100914.f90 on targets that don't provide quadmath.h
On Sun, 5 Sep 2021, Sandra Loosemore wrote: > Unless the aarch64 maintainers think it is a bug that __float128 is not > supported, I think the right solution here is the one I was thinking of > previously, to fix the Fortran front end to tie the C_FLOAT128 kind to > _Float128 rather than __float128, and fix the runtime support and test cases > accordingly. Then there should be no need to depend on quadmath.h at all. > C_FLOAT128 is a GNU extension and _Float128 is supported on a superset of > targets that __float128 is, so there should be no issue with backward > compatibility. gfc_build_intrinsic_lib_fndecls (at least) knows about the "q" function naming in libquadmath, and I think will result in gfortran generating calls to *q functions in some cases. I think there are at least three cases involved: (a) long double has the IEEE binary128 format. In that case, the *l functions from libm can be used, with no need for libquadmath to be involved at all. (It's still necessary to allow for the long double libm functions possibly having a different assembler name, for the powerpc64le case, but if the front end goes via built-in functions then it may not need further code to handle that specifically; see rs6000_mangle_decl_assembler_name.) (b) long double does not have the IEEE binary128 format, but glibc includes support for _Float128 functions (*f128) in libm for this target. Whether that support is included depends on the architecture and glibc version (glibc 2.26 or later needed; supported for x86_64/x86/ia64/powerpc64le). If the glibc support is present (could be tested using GCC_GLIBC_VERSION_GTE_IFELSE in configure to work properly when bootstrapping a cross compiler), it would make sense for the Fortran front end to use *f128 functions directly and so not depend on libquadmath. But I don't think the Fortran front end actually has any support for using *f128 functions at present. It's possible some non-glibc libraries also support *f128 or will do so in future (those are the standard names in TS 18661-3 / C23). However, you can't do configure tests of compiling/linking with target libraries when configuring front ends, only grep headers or use information about the configured target triplet or configure options (which is more or less what GCC_GLIBC_VERSION_GTE_IFELSE does), so I don't think there's anything that could readily be done to allow using *f128 from non-glibc libm automatically if such libm gains support for those functions in future. (c) __float128 is supported, different format from long double, but glibc does not have _Float128 functions (too old) or a non-glibc libm is in use. This is the only case where libquadmath is actually relevant. (libquadmath code is based on glibc, with some support (update-quadmath.py) for updating parts of that code from glibc sources automatically (most of the parts based on glibc libm, but not the string conversion parts). In practice, it's likely that changes to update-quadmath.py would be needed as part of such an update.) -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, Fortran] Revert to non-multilib-specific ISO_Fortran_binding.h
On 19.08.21 04:57, Sandra Loosemore wrote: This is a follow-up to commit fef67987cf502fe322e92ddce22eea7ac46b4d75: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=fef67987cf502fe322e92ddce22eea7ac46b4d75 I realized last week that having multilib-specific versions of ISO_Fortran_binding.h (generated by running the compiler to ask what kinds it supports) was still broken outside of the test support; the directory where it's being installed isn't on GCC's normal search path. It seemed to me that it was better to try to find some other solution for this problem than to venture down what appears to be a rat hole. I've come up with this patch to return to a single ISO_Fortran_binding.h file that uses preprocessor magic to identify the Fortran kind corresponding to the standard C long double type and the GCC extension types __float128 and int128_t. I haven't attempted to undo the follow-up patches that fixed in-tree testing; the static .h file is still copied to the build directory, and it can still be referenced with <> syntax during testing. Any complaints about either the overall strategy here, or the logic to infer the C type -> kind mapping? Or OK to commit? OK. [I think you need to fix the merge differences due to your https://gcc.gnu.org/g:93b6b2f614eb692d1d8126ec6cb946984a9d01d7 commit (regarding CFI_type_cfunptr) when applying the patch.] I believe that, at some point, we still want to have a multi-lib include directory, e.g. $(libsubdir)/include-multi/, for those include files which have to be split per multi lib (+ update GCC's include search paths). An example would be libgomp's omp.h (→ https://gcc.gnu.org/PR60670). However, that's a separate issue. If that's available, we can then still decide whether the new solution of __... feature tests is sufficient or whether it should be then changed back to multiple include files. In any case, this patch unbreaks https://sourceware.org/PR28183 which is surely a good thing :-) Thanks and sorry for the slow review. Tobias PS: One reason for the slow review was that I initially had planned to include bits of ISO_Fortran_binding.h in the compiler FE for constructing the CFI descriptor. However, the code started to be that convoluted and additional #defines were needed. Thus, I have now simply copied the definitions to gfortran.h + some hard-coded values in trans-type.c, where the CFI type is constructed. – Thus, you patch does not interfere with my CFI patch → https://gcc.gnu.org/pipermail/gcc-patches/2021-September/578904.html - 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
*PING* [PATCH] PR fortran/101327 - ICE in find_array_element, at fortran/expr.c:1355
PING. > Gesendet: Montag, 30. August 2021 um 23:40 Uhr > Von: "Harald Anlauf" > An: "fortran" , "gcc-patches" > Betreff: [PATCH] PR fortran/101327 - ICE in find_array_element, at > fortran/expr.c:1355 > > There was an issue when trying to use an element from an array constructor > which was a broken in a way probably only Gerhard could conceive. > We hit an assert that can be replaced by more robust code. > > Patch is basically Steve's. > > Regtested on x86_64-pc-linux-gnu. OK for mainline? > > Thanks, > Harald > > > Fortran - improve error recovery determining array element from constructor > > gcc/fortran/ChangeLog: > > PR fortran/101327 > * expr.c (find_array_element): When bounds cannot be determined as > constant, return error instead of aborting. > > gcc/testsuite/ChangeLog: > > PR fortran/101327 > * gfortran.dg/pr101327.f90: New test. > >
Re: [PATCH] PR fortran/101327 - ICE in find_array_element, at fortran/expr.c:1355
On 30.08.21 23:40, Harald Anlauf via Fortran wrote: There was an issue when trying to use an element from an array constructor which was a broken in a way probably only Gerhard could conceive. We hit an assert that can be replaced by more robust code. Patch is basically Steve's. Regtested on x86_64-pc-linux-gnu. OK for mainline? LGTM. Thanks to both of you – and sorry for the belated review. Tobias Fortran - improve error recovery determining array element from constructor gcc/fortran/ChangeLog: PR fortran/101327 * expr.c (find_array_element): When bounds cannot be determined as constant, return error instead of aborting. gcc/testsuite/ChangeLog: PR fortran/101327 * gfortran.dg/pr101327.f90: New test. - 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
