Re: [PATCH] libgfortran: Fix -Wincompatible-pointer-types errors
On 05/12/2023 10:33, Jakub Jelinek wrote:
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;
That doesn't look right for a 64-bit processor. Presumably 4 means 4
bytes, but long will generally be 8 on such targets.
R.
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(&vlist) = 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 : &noiostat;
+ child_iostat = ((dtp->common.flags & IOPARM_HAS_IOSTAT)
+ ? dtp->common.iostat : &noiostat);
/* 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 : &noiostat;
+ child_iostat = ((dtp->common.flags & IOPARM_HAS_IOSTAT)
+ ? dtp->common.iostat : &noiostat);
/* Set iomsg, intent(inout). */
if (dtp->common.flags & IOPARM_HAS_IOMSG)
Re: [PATCH] libgfortran: Fix -Wincompatible-pointer-types errors
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. R. Jakub
Re: [PATCH] libgfortran: Fix -Wincompatible-pointer-types errors
On 05/12/2023 10:59, Jakub Jelinek wrote: 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 Ah, I understand now. I've successfully built arm and aarch64 cross toolchains with this patch (newlib). So LGTM, thanks. R.
Re: [PATCH v7] libgfortran: Replace mutex with rwlock
On 15/12/2023 11:31, Lipeng Zhu wrote: On 2023/12/14 23:50, Richard Earnshaw (lists) wrote: On 09/12/2023 15:39, 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. It looks like this has broken builds on arm-none-eabi when using newlib: In file included from /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran /runtime/error.c:27: /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran/io/io.h: In function ‘dec_waiting_unlocked’: /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran/io/io.h:1023:3: error : implicit declaration of function ‘WRLOCK’ [-Wimplicit-function-declaration] 1023 | WRLOCK (&unit_rwlock); | ^~ /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran/io/io.h:1025:3: error : implicit declaration of function ‘RWUNLOCK’ [-Wimplicit-function-declaration] 1025 | RWUNLOCK (&unit_rwlock); | ^~~~ R. Hi Richard, The root cause is that the macro WRLOCK and RWUNLOCK are not defined in io.h. The reason of x86 platform not failed is that HAVE_ATOMIC_FETCH_ADD is defined then caused above macros were never been used. Code logic show as below: #ifdef HAVE_ATOMIC_FETCH_ADD (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED); #else WRLOCK (&unit_rwlock); u->waiting--; RWUNLOCK (&unit_rwlock); #endif I just draft a patch try to fix this bug, because I didn't have arm platform, would you help to validate if it was fixed on arm platform? diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h index 15daa0995b1..c7f0f7d7d9e 100644 --- a/libgfortran/io/io.h +++ b/libgfortran/io/io.h @@ -1020,9 +1020,15 @@ dec_waiting_unlocked (gfc_unit *u) #ifdef HAVE_ATOMIC_FETCH_ADD (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED); #else - WRLOCK (&unit_rwlock); +#ifdef __GTHREAD_RWLOCK_INIT + __gthread_rwlock_wrlock (&unit_rwlock); + u->waiting--; + __gthread_rwlock_unlock (&unit_rwlock); +#else + __gthread_mutex_lock (&unit_rwlock); u->waiting--; - RWUNLOCK (&unit_rwlock); + __gthread_mutex_unlock (&unit_rwlock); +#endif #endif } Lipeng Zhu Hi Lipeng, Thanks for the quick reply. I can confirm that with the above change the bootstrap failure is fixed. However, this shouldn't be considered a formal review; libgfortran is not really my area. I'll be away now until January 2nd. Richard.
Re: [PATCH v2] libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD
On 05/01/2024 01:43, 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 Has this been committed yet? R. --- libgfortran/io/io.h | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h index 15daa0995b1..c7f0f7d7d9e 100644 --- a/libgfortran/io/io.h +++ b/libgfortran/io/io.h @@ -1020,9 +1020,15 @@ dec_waiting_unlocked (gfc_unit *u) #ifdef HAVE_ATOMIC_FETCH_ADD (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED); #else - WRLOCK (&unit_rwlock); +#ifdef __GTHREAD_RWLOCK_INIT + __gthread_rwlock_wrlock (&unit_rwlock); + u->waiting--; + __gthread_rwlock_unlock (&unit_rwlock); +#else + __gthread_mutex_lock (&unit_rwlock); u->waiting--; - RWUNLOCK (&unit_rwlock); + __gthread_mutex_unlock (&unit_rwlock); +#endif #endif }
Re: [PATCH v7] libgfortran: Replace mutex with rwlock
On 09/12/2023 15:39, 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. > It looks like this has broken builds on arm-none-eabi when using newlib: In file included from /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran /runtime/error.c:27: /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran/io/io.h: In function ‘dec_waiting_unlocked’: /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran/io/io.h:1023:3: error : implicit declaration of function ‘WRLOCK’ [-Wimplicit-function-declaration] 1023 | WRLOCK (&unit_rwlock); | ^~ /work/rearnsha/gnusrc/nightly/gcc-cross/master/libgfortran/io/io.h:1025:3: error : implicit declaration of function ‘RWUNLOCK’ [-Wimplicit-function-declaration] 1025 | RWUNLOCK (&unit_rwlock); | ^~~~ R. > --- > v1 -> v2: > Limit the pthread_rwlock usage in libgcc only when __cplusplus isn't defined. > > v2 -> v3: > Rebase the patch with trunk branch. > > v3 -> v4: > Update the comments. > > v4 -> v5: > Fix typos and code formatter. > > v5 -> v6: > Add unit tests. > > v6 -> v7: > Update ChangeLog and code formatter. > > Reviewed-by: Hongjiu Lu > Reviewed-by: Bernhard Reutner-Fischer > Reviewed-by: Thomas Koenig > Reviewed-by: Jakub Jelinek > Signed-off-by: Lipeng Zhu > --- > libgcc/gthr-posix.h | 60 +++ > libgfortran/io/async.c| 4 + > libgfortran/io/async.h| 151 ++ > libgfortran/io/io.h | 15 +- > libgfortran/io/transfer.c | 8 +- > libgfortran/io/unit.c | 117 +- > libgfortran/io/unix.c | 16 +- > .../testsuite/libgomp.fortran/rwlock_1.f90| 33 > .../testsuite/libgomp.fortran/rwlock_2.f90| 22 +++ > .../testsuite/libgomp.fortran/rwlock_3.f90| 18 +++ > 10 files changed, 386 insertions(+), 58 deletions(-) > create mode 100644 libgomp/testsuite/libgomp.fortran/rwlock_1.f90 > create mode 100644 libgomp/testsuite/libgomp.fortran/rwlock_2.f90 > create mode 100644 libgomp/testsuite/libgomp.fortran/rwlock_3.f90 > > diff --git a/libgcc/
Re: [patch, Fortran] IEEE support for aarch64-apple-darwin
On 06/12/2021 16:32, FX via Gcc-patches wrote: Hi everyone, Since support for target aarch64-apple-darwin has been submitted for review, it’s time to submit the Fortran part, i.e. enabling IEEE support on that target. The patch has been in use now for several months, in a developer branch shipped by some distros on macOS (including Homebrew). It was authored more than a year ago, but I figured it wasn’t relevant to submit until the target was actually close to be in trunk: https://github.com/iains/gcc-darwin-arm64/commit/b107973550d3d9a9ce9acc751adbbe2171d13736 Bootstrapped and tested on aarch64-apple-darwin20 (macOS Big Sur) and aarch64-apple-darwin21 (macOS Monterey). OK to merge? Can someone point me to the right way of formatting ChangeLogs and commit entries, nowadays? Thanks, FX This isn't a full review, but I do have a question: is this really specific to Darwin? or is it really generic aarch64 code? If the former, then the file name is not right and it should reflect the darwin-specific nature of the contents. If the latter, then I wonder why most other fortran targets don't seem to have specific implementations of this. R.
Re: [patch, Fortran] IEEE support for aarch64-apple-darwin
On 08/12/2021 15:47, FX via Gcc-patches wrote: Hi Richard, This isn't a full review, but I do have a question: is this really specific to Darwin? or is it really generic aarch64 code? If the former, then the file name is not right and it should reflect the darwin-specific nature of the contents. If the latter, then I wonder why most other fortran targets don't seem to have specific implementations of this. The code is not specific to Darwin, but right now I chose to only enable on Darwin because: - All glibc targets are covered already by using glibc function calls - I don’t know if there are other aarch64 targets that exist, support Fortran and IEEE, but don’t have glibc I’d suggest other non-glibc aarch64 targets could be added to the support and enable this code, but I don’t want to do it unless it’s been tested there. IEEE support is optional in Fortran, so I suggest we keep it “opt-in”: targets where it’s known to work enable it, but it’s off by default on other targets. I hope this explains the rationale. FX OK, that makes sense. I wonder if a comment to that effect is warranted somewhere. One tricky thing about aarch64 IEEE support is that trapping exceptions (raising a software exception, as opposed to just setting an exceptional condition bit in the FPSR) is implementation defined. The only way to tell if your implementation can support trapping an exception is to try to set the trapping enable bit in the FPCR. If the bit is read back and is non-zero then that exception supports trapping; if it reads back as zero, then trapping that exception is not supported. More details can be found in the armv8 Arm ARM. HTH, R.
