Re: [PATCH V2] powerpc: properly check for feenableexcept() on FreeBSD
Hi Piotr, Thanks for doing this, some comments are inlined. on 2022/5/11 07:32, Piotr Kubaj via Gcc-patches wrote: > Is there anything more required? > > On 22-05-03 12:33:43, Piotr Kubaj wrote: >> Here are gmake check-gfortran results requested by FX. >> >> Before patching: >> === gfortran Summary === >> >> # of expected passes65106 >> # of unexpected failures6 >> # of expected failures 262 >> # of unsupported tests 367 >> >> After patching: >> === gfortran Summary === >> >> # of expected passes65384 >> # of unexpected failures6 >> # of expected failures 262 >> # of unsupported tests 373 >> Nice! >> In both cases, unexpected failures are: >> FAIL: gfortran.dg/pr98076.f90 -O0 execution test >> FAIL: gfortran.dg/pr98076.f90 -O1 execution test >> FAIL: gfortran.dg/pr98076.f90 -O2 execution test >> FAIL: gfortran.dg/pr98076.f90 -O3 -fomit-frame-pointer -funroll-loops >> -fpeel-loops -ftracer -finline-functions execution test >> FAIL: gfortran.dg/pr98076.f90 -O3 -g execution test >> FAIL: gfortran.dg/pr98076.f90 -Os execution test >> >> But this seems unrelated to my patch.>> >> On 22-05-03 12:21:12, pku...@freebsd.org wrote: >>> From: Piotr Kubaj >>> >>> FreeBSD/powerpc* has feenableexcept() defined in fenv.h header. >>> >>> Signed-off-by: Piotr Kubaj >>> --- >>> libgfortran/configure| 41 +++- >>> libgfortran/configure.ac | 17 - >>> 2 files changed, 56 insertions(+), 2 deletions(-) >>> ... snip >>> # At least for glibc, clock_gettime is in librt. But don't >>> # pull that in if it still doesn't give us the function we want. This >>> diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac >>> index 97cc490cb5e..2dd6d345b22 100644 >>> --- a/libgfortran/configure.ac >>> +++ b/libgfortran/configure.ac >>> @@ -602,8 +602,23 @@ fi >>> # Check whether we have a __float128 type, depends on >>> enable_libquadmath_support >>> LIBGFOR_CHECK_FLOAT128 >>> >>> +case x$target in >>> + xpowerpc*-freebsd*) >>> +AC_CACHE_CHECK([for fenv.h and feenableexcept], have_feenableexcept, [ >>> + AC_COMPILE_IFELSE([AC_LANG_PROGRAM( >>> +[[ #include ]], >>> +[[ feenableexcept(FE_DIVBYZERO | FE_INVALID); ]])], >>> +[ have_feenableexcept="yes" ], >>> +[ have_feenableexcept="no" ])]) >>> +if test "x$have_feenableexcept" = "xyes"; then >>> + AC_DEFINE(HAVE_FEENABLEEXCEPT,1,[fenv.h includes feenableexcept]) >>> +fi; >>> +;; >>> + *) As your explanation in [1], IMHO it's not a good idea to take powerpc*-freebsd* specially here, for your mentioned triples that may also suffer this same issue, we will have to update this hunk for them in future. How about: do the glibc check first as before, if it fails then do one further general check (for all) with one compilation on fenv.h as what your patch proposes. Besides, IMHO it should use AC_LINK_IFELSE, since one fenv.h which doesn't implement feenableexcept could also pass the check. And there is one AC_CHECK_HEADERS_ONCE checking for fenv.h existence, not sure if it's worth to guarding the further check with its result. [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593248.html BR, Kewen
Re: [PATCH V2] powerpc: properly check for feenableexcept() on FreeBSD
on 2022/5/13 04:16, Segher Boessenkool wrote: > Hi Piotr, > > On Tue, May 03, 2022 at 12:21:12PM +0200, pku...@freebsd.org wrote: >> FreeBSD/powerpc* has feenableexcept() defined in fenv.h header. > > Declared, not defined. These are required to be real functions (on all > platforms that have these functions), not macros or inlines or whatever. > Piotr's reply "FreeBSD doesn't have this function in libm, it's implemented in /usr/include/fenv.h." from [1] made me feel like it's a definition instead of declaration. So I thought the check should use AC_LINK_IFELSE instead, since one fenv.h which doesn't have the definition can still pass the proposed AC_COMPILE_IFELSE check. I just did a further search, the powerpc fenv.h [2] does include the definition of feenableexcept. By comparison, the x86 fenv.h [3] doesn't. But I'm not sure if it's the same as what Piotr's environments have. Hope it's similar. :-) [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593193.html [2] https://github.com/freebsd/freebsd-src/blob/main/lib/msun/powerpc/fenv.h [3] https://github.com/freebsd/freebsd-src/blob/main/lib/msun/x86/fenv.h BR, Kewen
Re: [PATCH, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi Haochen,
on 2023/3/1 15:09, HAO CHEN GUI wrote:
> Hi,
> The patch escalates the failure when Hollerith constant to real conversion
> fails in native_interpret_expr. It finally reports an "Unclassifiable
> statement" error.
>
> The patch of pr95450 added a verification for decoding/encoding checking
> in native_interpret_expr. native_interpret_expr may fail on real type
> conversion and returns a NULL tree then. But upper layer calls don't handle
> the failure so that an ICE is reported when the verification fails.
>
> IBM long double is an example. It doesn't have a unique memory presentation
> for some real values. So it may not pass the verification. The new test
> case shows the problem.
>
> Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
>
> Thanks
> Gui Haochen
>
> ChangeLog
> 2023-03-01 Haochen Gui
>
> gcc/
> PR target/103628
> * fortran/target-memory.cc (gfc_interpret_float): Return FAIL when
> native_interpret_expr gets a NULL tree.
> * fortran/arith.cc (gfc_hollerith2real): Return NULL when
> gfc_interpret_float fails.
>
> gcc/testsuite/
> PR target/103628
> * gfortran.dg/pr103628.f90: New.
>
>
> patch.diff
> diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
> index c0d12cfad9d..d3d38c7eb6a 100644
> --- a/gcc/fortran/arith.cc
> +++ b/gcc/fortran/arith.cc
> @@ -2752,10 +2752,12 @@ gfc_hollerith2real (gfc_expr *src, int kind)
>result = gfc_get_constant_expr (BT_REAL, kind, &src->where);
>
>hollerith2representation (result, src);
> - gfc_interpret_float (kind, (unsigned char *) result->representation.string,
> -result->representation.length, result->value.real);
> -
> - return result;
> + if (gfc_interpret_float (kind,
> +(unsigned char *) result->representation.string,
> +result->representation.length, result->value.real))
> +return result;
> + else
> +return NULL;
> }
>
> /* Convert character to real. The constant will be padded or truncated. */
> diff --git a/gcc/fortran/target-memory.cc b/gcc/fortran/target-memory.cc
> index 7ce7d736629..04afc357e3c 100644
> --- a/gcc/fortran/target-memory.cc
> +++ b/gcc/fortran/target-memory.cc
> @@ -417,10 +417,13 @@ gfc_interpret_float (int kind, unsigned char *buffer,
> size_t buffer_size,
> {
>gfc_set_model_kind (kind);
>mpfr_init (real);
> - gfc_conv_tree_to_mpfr (real,
> - native_interpret_expr (gfc_get_real_type (kind),
> - buffer, buffer_size));
>
> + tree source = native_interpret_expr (gfc_get_real_type (kind), buffer,
> + buffer_size);
> + if (!source)
> +return 0;
> +
> + gfc_conv_tree_to_mpfr (real, source);
>return size_float (kind);
> }
>
> diff --git a/gcc/testsuite/gfortran.dg/pr103628.f90
> b/gcc/testsuite/gfortran.dg/pr103628.f90
> new file mode 100644
> index 000..e49aefc18fd
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/pr103628.f90
> @@ -0,0 +1,14 @@
> +! { dg-do compile { target powerpc*-*-* } }
> +! { dg-options "-O2 -mabi=ibmlongdouble" }
Since this test case is powerpc only, I think it can be moved to
gcc/testsuite/gcc.target/powerpc/ppc-fortran.
BR,
Kewen
Re: [PATCHv2, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi Haochen, on 2023/3/3 20:54, Tobias Burnus wrote: > Hi Haochen, > > On 03.03.23 10:56, HAO CHEN GUI via Gcc-patches wrote: >> Sure, I will merge it into the patch and do the regression test. > Thanks :-) >> Additionally, Kewen suggested: Since this test case is powerpc only, I think it can be moved to gcc/testsuite/gcc.target/powerpc/ppc-fortran/ppc-fortran.expgcc.target/powerpc/ppc-fortran. >>> Which sounds reasonable. >> Test cases under gcc.target are tested by check-gcc-c. It greps "warning" >> and "error" (C style, lower case) from the output while check-gcc-fortran >> greps "Warning" and "Error" (upper case). As the test case needs to check >> the "Warning" and "Error" messages. I have to put it in gfortran.dg >> directory. What's your opinion? > > Thanks for digging and giving a reason. +1! I just posted one patch[1] to make ppc-fortran.exp support the need of your patch here, I verified it can work for this revision, could you double check with your updated revision? [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613442.html > > Looks as if at some point, adapting > gcc/testsuite/gcc.target/powerpc/ppc-fortran/ppc-fortran.exp to handle > this as well could make sense. > > But placing it - as you did - under gcc/testsuite/gfortran.dg is fine > and surely the simpler solution. Thus, leave it as it. Yeah, either way works for me. Thanks again! BR, Kewen
