Hi Yuao,
first comments, I still need to look again at some changes, especially
at simplify.cc.
Yuao Ma wrote:
This patch introduces constant input support for trigonometric functions,
including those involving half-revolutions. Both valid and invalid
inputs have
been thoroughly tested, as have mpfr versions greater than or equal to
4.2 and
less than 4.2.
Inspired by Steve's previous work, this patch also fixes subtle bugs
revealed
by newly added test cases.
Can you add the PR number to the commit log ("PR fortran/113152")?
→ https://gcc.gnu.org/PR113152
[Side remark: That's also where Steve attached his draft version of the
patch.]
* * *
Can you also update the documentation – as you already did for ATAND?
(Namely, the one that is shown, e.g., at
https://gcc.gnu.org/onlinedocs/gfortran/Intrinsic-Procedures.html – but
you already know the gcc/fortran/intrinsics.texi file.)
* * *
BTW: Your patch currently give an internal compiler error ("ICE") if one
tries to use it with non-constant expressions.
As now the builtins are available, how about updating mathbuiltins.def
already?
That's a trivial change and will convert the ugly ICE into either a
link-time error or working code, depending whether libc supports it.
[Adding run-time testcases should then be done in lockstep with the
fallback implementation in libgfortran.]
* * *
If this patch is merged, I plan to work on middle-end optimization
support for
previously added GCC built-ins and libgfortran intrinsics.
Thanks.
* * *
FYI for the fallback implementation in libgfortran:
Note that GCC uses a very specific version of the automake / autoconf,
see https://gcc.gnu.org/install/prerequisites.html (reason: the
generated files shouldn't change because someone used a different tool
version).
One way to regenerate them is to configure + build GCC with
--enable-maintainer-mode, which will then call the autotools (ensure the
right version is in the $PATH), but there are also other ways to run them.
For the checks, have a look at GCC_CHECK_MATH_FUNC in configure.ac +
c99_protos.h + intrinsics/c99_functions.c
for the existing fall-back functions.
You also have to update "gfortran.map"; there is the existing
GFORTRAN_C99_8 but we need a new section.
[For real(16), the changes likely need to go into libquadmath instead,
albeit I am not quite sure how to disentangle atan2f128 supported by a
bit older GLIBC from atan2pif128 that is only supported in the newest
GLIBC.]
* * *
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -717,8 +717,15 @@ enum gfc_isym_id
GFC_ISYM_YN,
GFC_ISYM_YN2,
+ GFC_ISYM_ACOSPI,
+ GFC_ISYM_ASINPI,
+ GFC_ISYM_ATANPI,
+ GFC_ISYM_ATAN2PI,
+ GFC_ISYM_COSPI,
+ GFC_ISYM_SINPI,
+ GFC_ISYM_TANPI,
/* Add this at the end, so maybe the module format
remains compatible. */
GFC_ISYM_SU_KIND,
GFC_ISYM_UINT,
};
As the comment states, it seems to make sense to put the new symbols at
the end. GCC generates for "module m" a module file (.mod) which is a
compress lisp-like file. That one also stores the intrinsic procedure's
ID – and it makes sense to be upward compatible by appending the new
ones at the end.
* * *
--- a/gcc/fortran/intrinsic.cc
+++ b/gcc/fortran/intrinsic.cc
@@ -3452,6 +3452,5 @@add_functions (void)
- add_sym_1 ("acosd", GFC_ISYM_ACOSD, CLASS_ELEMENTAL, ACTUAL_YES,
- BT_REAL, dr, GFC_STD_F2023,
- gfc_check_fn_r, gfc_simplify_acosd, gfc_resolve_trigd,
- x, BT_REAL, dr, REQUIRED);
+ add_sym_1 ("acosd", GFC_ISYM_ACOSD, CLASS_ELEMENTAL, ACTUAL_YES,
BT_REAL, dr,
+ GFC_STD_F2023, gfc_check_fn_r, gfc_simplify_acosd,
+ gfc_resolve_trig, x, BT_REAL, dr, REQUIRED);
I don't understand this change. First, I don't see any reason why this
should get modified. And by modifying it, a simple "git blame" will also
point to the "wrong" commit.
But also otherwise: it does not save the on the number of lines and it
also breaks the current pattern:
First line – name plus classification.
Second line – return value, Fortran standard
Third line – functions pointers
Fourth line – arguments
I have to admit that we haven't been super consistent and the first two
lines are often also in one, but mostly (but not always) the function
pointer are on a line of their own.
Thus, I think we can break this pattern - but, in particular, I wouldn't
do so for the existing code, unless there is a good reason for doing so.
* * *
Testcases: I think it makes sense to check – either in this patch or in
the follow up – that -std=f2018 gives a diagnostic that the intrinsic is
unsupported while -std=f2023 supports it.
* * *
Tobias
