| Message ID | yddegxzk05e.fsf@CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
Dear Rainer, > Unfortunately, while the patch works fine on Solaris/x86, it broke > Solaris/SPARC bootstrap for trivial reasons: contrary to the ChangeLog, > configure and config.h.in weren't regenerated, thus FPSETSTICKY > wasn't defined. I apologize. Thanks for checking in the fix. > FAIL: gfortran.dg/ieee/ieee_6.f90 -Os execution test > > The test aborts at l.47, but unfortunately I cannot print mode in gdb 7.7. That’s weird, especially if that one fails but ieee_rounding_1.f90 works. Let me know if I can do anything to help debug this. > The following patch corrects this, at the same time fixing this warning: > > /fpu-target.h:451:3: warning: implicit declaration of function 'assert' [-Wimplicit-function-declaration] > assert (sizeof(fpu_state_t) <= GFC_FPE_STATE_BUFFER_SIZE); Actually, it makes a lot of sense to change these into static assertions: this way, any target-specific issues with FP-state buffer size will show up at libgfortran-building-time, rather than be swept under the rug. Since libgfortran is compiled with GCC, which supports _Static_assert since 4.6, I propose the attached patch. Built and tested on x86_64-linux, OK to commit? FX
On Sun, Jul 6, 2014 at 11:13 PM, FX <fxcoudert@gmail.com> wrote: > Actually, it makes a lot of sense to change these into static assertions: this way, any target-specific issues with FP-state buffer size will show up at libgfortran-building-time, rather than be swept under the rug. > > Since libgfortran is compiled with GCC, which supports _Static_assert since 4.6, I propose the attached patch. Furthermore, on 2014-05-12 I committed a patch changing libgfortran to be built with -std=gnu11 instead of -std=gnu99, so that we can make use of C11 functionality; see https://gcc.gnu.org/ml/fortran/2014-04/msg00101.html . > Built and tested on x86_64-linux, OK to commit? Ok, thanks.
> Furthermore, on 2014-05-12 I committed a patch changing libgfortran to > be built with -std=gnu11 instead of -std=gnu99, so that we can make > use of C11 functionality; see > https://gcc.gnu.org/ml/fortran/2014-04/msg00101.html . Committed as rev. 212323, thanks for the review. I now propose the attached patch, which performs a small cleaning up: - Use the new _Noreturn language feature (supported in GCC since 2011) instead of the old attribute. This makes prototypes shorter and more generic. - Move the complex-related REALPART, IMAGPART and COMPLEX_ASSIGN macros from libgfortran.h to c99_intrinsics.c, which is the only place they’re ever used. Built and tested on x86_64-linux, OK to commit? FX PS: I didn’t touch libcaf, as I assume this might be compiled with a different compiler. Am I right? PS2: A third issue I’ve though about is: should we get rid of the following __GNUC__ test? libgfortran is not used as a standalone Fortran runtime library, and I think it is (and never will) be built by something else than a stage3 compiler. > #ifndef __GNUC__ > #define __attribute__(x) > #define likely(x) (x) > #define unlikely(x) (x) > #else > #define likely(x) __builtin_expect(!!(x), 1) > #define unlikely(x) __builtin_expect(!!(x), 0) > #endif
FX <fxcoudert@gmail.com> writes: >> Unfortunately, while the patch works fine on Solaris/x86, it broke >> Solaris/SPARC bootstrap for trivial reasons: contrary to the ChangeLog, >> configure and config.h.in weren't regenerated, thus FPSETSTICKY >> wasn't defined. > > I apologize. Thanks for checking in the fix. No worries: if only all bootstrap failures were that easy to fix ;-) >> FAIL: gfortran.dg/ieee/ieee_6.f90 -Os execution test >> >> The test aborts at l.47, but unfortunately I cannot print mode in gdb 7.7. > > That’s weird, especially if that one fails but ieee_rounding_1.f90 > works. Let me know if I can do anything to help debug this. I see now what's going on: mode is 1 on that line, while ieee_to_zero is 3. Looking at Solaris <ieeefp.h> explains what happens: #if defined(__sparc) /* * NOTE: the values given are chosen to match those used by the * RD (Round Direction) field of the FSR (Floating Point State Register). */ typedef enum fp_rnd { FP_RN = 0, /* round to nearest representable number, tie -> even */ FP_RZ = 1, /* round toward zero (truncate) */ FP_RP = 2, /* round toward plus infinity */ FP_RM = 3 /* round toward minus infinity */ } fp_rnd; #endif while the i386/amd64 values are the usual ones. Unfortunately, gcc/fortran/libgfortran.h hardcodes the more common values for GFC_FPE_*, and libgfortran/Makefile.am extracts them from there into fpu-target.inc. I'm unsure what's the best way to handle this. Rainer
Hi Rainer, > while the i386/amd64 values are the usual ones. Unfortunately, > gcc/fortran/libgfortran.h hardcodes the more common values for > GFC_FPE_*, and libgfortran/Makefile.am extracts them from there into > fpu-target.inc. I'm unsure what's the best way to handle this. No, we don’t hardcode any values (unless I misunderstand what you are saying). Look at libgfortran/config/fpu-sysv.h get_fpu_rounding_mode() and set_fpu_rounding_mode(): we have two switches, to translate between the GFC_FPE_* values and the FP_R* values. So this should work, really. The only thing I can see is that libgfortran/config/fpu-sysv.h assumes that FP_RM and others are macros, checking them with "#ifdef FP_RM”. Is that the reason? If so, we might just want to use them unconditionally… unless it creates a mess on some other SysV target! FX
On Mon, Jul 7, 2014 at 11:29 AM, FX <fxcoudert@gmail.com> wrote: >> Furthermore, on 2014-05-12 I committed a patch changing libgfortran to >> be built with -std=gnu11 instead of -std=gnu99, so that we can make >> use of C11 functionality; see >> https://gcc.gnu.org/ml/fortran/2014-04/msg00101.html . > > Committed as rev. 212323, thanks for the review. > > I now propose the attached patch, which performs a small cleaning up: > - Use the new _Noreturn language feature (supported in GCC since 2011) instead of the old attribute. This makes prototypes shorter and more generic. > - Move the complex-related REALPART, IMAGPART and COMPLEX_ASSIGN macros from libgfortran.h to c99_intrinsics.c, which is the only place they’re ever used. > > > Built and tested on x86_64-linux, OK to commit? Ok. > PS: I didn’t touch libcaf, as I assume this might be compiled with a different compiler. Am I right? My understanding is that libcaf is delivered in source form, and the end user is expected to compile it against the correct MPI library on the target. So yes, one could be a bit more conservative here than in libgfortran proper (although one could expect the user to have access to the gcc version corresponding to gfortran..). But Tobias certainly knows better. > PS2: A third issue I’ve though about is: should we get rid of the following __GNUC__ test? libgfortran is not used as a standalone Fortran runtime library, and I think it is (and never will) be built by something else than a stage3 compiler. > >> #ifndef __GNUC__ >> #define __attribute__(x) >> #define likely(x) (x) >> #define unlikely(x) (x) >> #else >> #define likely(x) __builtin_expect(!!(x), 1) >> #define unlikely(x) __builtin_expect(!!(x), 0) >> #endif What about --disable-bootstrap? Does it just skip stage1 and stage2, and stage3 is used to compile libgfortran, or is the host compiler used to build libgfortran as well? If the former, yes I guess we can remove #ifnderf __GNUC__ stuff?
> What about --disable-bootstrap? Does it just skip stage1 and stage2, > and stage3 is used to compile libgfortran, or is the host compiler > used to build libgfortran as well? If the former, yes I guess we can > remove #ifnderf __GNUC__ stuff? Even with --disable-bootstrap, libgfortran is compiled with the freshly-built compiler, not the host compiler. FX
On Mon, Jul 7, 2014 at 12:16 PM, FX <fxcoudert@gmail.com> wrote: >> What about --disable-bootstrap? Does it just skip stage1 and stage2, >> and stage3 is used to compile libgfortran, or is the host compiler >> used to build libgfortran as well? If the former, yes I guess we can >> remove #ifnderf __GNUC__ stuff? > > Even with --disable-bootstrap, libgfortran is compiled with the freshly-built compiler, not the host compiler. > > FX Right, that's what I (vaguelly) remembered. Please consider a patch removing the ifndef __GNUC__ stuff from libgfortran.h pre-approved.
> Right, that's what I (vaguelly) remembered. Please consider a patch > removing the ifndef __GNUC__ stuff from libgfortran.h pre-approved. The only occurrence (outside of libcaf) is in libgfortran.h. Committed attached patch as rev. 212328, after building on x86_64-linux. FX
diff --git a/libgfortran/config/fpu-sysv.h b/libgfortran/config/fpu-sysv.h --- a/libgfortran/config/fpu-sysv.h +++ b/libgfortran/config/fpu-sysv.h @@ -25,6 +25,8 @@ see the files COPYING3 and COPYING.RUNTI /* FPU-related code for SysV platforms with fpsetmask(). */ +#include <assert.h> + /* BSD and Solaris systems have slightly different types and functions naming. We deal with these here, to simplify the code below. */