Re: Fix some more decl types in the Fortran frontend

Re: Fix some more decl types in the Fortran frontend

[FX]

Changing the fntype[2] looks wrong to me, as it is also used for powi(double, int) , where the argument order matches the current version:

>   gfc_define_builtin ("__builtin_powi", mfunc_double[2],
>                       BUILT_IN_POWI, "powi", ATTR_CONST_NOTHROW_LEAF_LIST);

(I don’t see any other use of this, but I might be missing something.)

It looks like fntype[5] is actually what you need, and it’s already constructed! However, there is even more mistery here, because it is currently used for __builtin_scalbn, which doesn’t seem right: http://pubs.opengroup.org/onlinepubs/009695399/functions/scalbln.html

So I suspect looking a bit more in depth is required! Also, testcases that excercise this fndecl matching (which you would see fail on ptx) would be a great addition to the testsuite, once you commit (for powi & scalbn, which do not look covered right now, otherwise you would have seen regressions).

Cheers,
FX

Re: Fix some more decl types in the Fortran frontend

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

On 09/11/2014 12:37 PM, FX wrote:
> Changing the fntype[2] looks wrong to me, as it is also used for
> powi(double, int) , where the argument order matches the current
> version:

Ah, sorry. I only looked at mathbuiltins.def and didn't spot the other use.

> It looks like fntype[5] is actually what you need, and itÂ’s already
> constructed! However, there is even more mistery here, because it is
> currently used for __builtin_scalbn, which doesnÂ’t seem right:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/scalbln.html
>
>  So I suspect looking a bit more in depth is required! Also,
> testcases that excercise this fndecl matching (which you would see
> fail on ptx) would be a great addition to the testsuite, once you
> commit (for powi & scalbn, which do not look covered right now,
> otherwise you would have seen regressions).

So it looks like the following patch would be the right thing? I'm 
afraid I failed to construct a compileable Fortran testcase for scalbn.


Bernd


[-- Attachment #2: bessel2.diff --]
[-- Type: text/x-patch, Size: 2089 bytes --]

commit 5f170b2710aaa5e098d74c71fcd206ef209f0b60
Author: Bernd Schmidt <bernds@codesourcery.com>
Date:   Wed Sep 10 18:02:53 2014 +0200

    Fix type mismatches in intrinsic functions.
    
    	* f95-lang.c (gfc_init_builtin_functions): Use type index 2 for
    	scalbn, scalbnl and scalbnf.
    	* mathbuiltins.def (JN, YN): Use type index 5.

diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
index da3a0d0..e485201 100644
--- a/gcc/fortran/f95-lang.c
+++ b/gcc/fortran/f95-lang.c
@@ -784,11 +784,11 @@ gfc_init_builtin_functions (void)
   gfc_define_builtin ("__builtin_fabsf", mfunc_float[0], 
 		      BUILT_IN_FABSF, "fabsf", ATTR_CONST_NOTHROW_LEAF_LIST);
  
-  gfc_define_builtin ("__builtin_scalbnl", mfunc_longdouble[5], 
+  gfc_define_builtin ("__builtin_scalbnl", mfunc_longdouble[2], 
 		      BUILT_IN_SCALBNL, "scalbnl", ATTR_CONST_NOTHROW_LEAF_LIST);
-  gfc_define_builtin ("__builtin_scalbn", mfunc_double[5], 
+  gfc_define_builtin ("__builtin_scalbn", mfunc_double[2], 
 		      BUILT_IN_SCALBN, "scalbn", ATTR_CONST_NOTHROW_LEAF_LIST);
-  gfc_define_builtin ("__builtin_scalbnf", mfunc_float[5], 
+  gfc_define_builtin ("__builtin_scalbnf", mfunc_float[2], 
 		      BUILT_IN_SCALBNF, "scalbnf", ATTR_CONST_NOTHROW_LEAF_LIST);
  
   gfc_define_builtin ("__builtin_fmodl", mfunc_longdouble[1], 
diff --git a/gcc/fortran/mathbuiltins.def b/gcc/fortran/mathbuiltins.def
index d5bf60d..d06a90b 100644
--- a/gcc/fortran/mathbuiltins.def
+++ b/gcc/fortran/mathbuiltins.def
@@ -42,10 +42,10 @@ DEFINE_MATH_BUILTIN_C (TAN,   "tan",    0)
 DEFINE_MATH_BUILTIN_C (TANH,  "tanh",   0)
 DEFINE_MATH_BUILTIN   (J0,    "j0",     0)
 DEFINE_MATH_BUILTIN   (J1,    "j1",     0)
-DEFINE_MATH_BUILTIN   (JN,    "jn",     2)
+DEFINE_MATH_BUILTIN   (JN,    "jn",     5)
 DEFINE_MATH_BUILTIN   (Y0,    "y0",     0)
 DEFINE_MATH_BUILTIN   (Y1,    "y1",     0)
-DEFINE_MATH_BUILTIN   (YN,    "yn",     2)
+DEFINE_MATH_BUILTIN   (YN,    "yn",     5)
 DEFINE_MATH_BUILTIN   (ERF,   "erf",    0)
 DEFINE_MATH_BUILTIN   (ERFC,  "erfc",   0)
 DEFINE_MATH_BUILTIN   (TGAMMA,"tgamma", 0)

Re: Fix some more decl types in the Fortran frontend

[FX]

> So it looks like the following patch would be the right thing?

I would think so.

FX

Re: Fix some more decl types in the Fortran frontend

[Tobias Burnus]

On 11.09.2014 18:30, Bernd Schmidt wrote:
> So it looks like the following patch would be the right thing? I'm 
> afraid I failed to construct a compileable Fortran testcase for scalbn.

On 11.09.2014 18:32, FX wrote:
> I would think so.

Looks also good to me. Thanks for the patch, Bernd. And thanks for the 
review, FX. Do you want to undo your Fortran-maintainer → 
mere-contributor status, given that you are now again a bit more 
involved in the GCC development?

Tobias


>      Fix type mismatches in intrinsic functions.
>      
>      	* f95-lang.c (gfc_init_builtin_functions): Use type index 2 for
>      	scalbn, scalbnl and scalbnf.
>      	* mathbuiltins.def (JN, YN): Use type index 5.

Re: Fix some more decl types in the Fortran frontend

[FX]

> And thanks for the review, FX. Do you want to undo your Fortran-maintainer → mere-contributor status, given that you are now again a bit more involved in the GCC development?

Yeah, why not. I promise I'll be careful and only review things in my comfort zone (which isn't so large).

I'll wait for a few days before changing the MAINTAINERS file in case someone thinks it's a bad idea.

FX

Re: Fix some more decl types in the Fortran frontend

[FX]

So, after a six-year break (was it so long?), I’m back among the maintainers.
Committed as rev. 215237

FX




2014-09-13  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	* MAINTAINERS: Move myself to reviewers (Fortran).



Index: MAINTAINERS
===================================================================
--- MAINTAINERS	(revision 215236)
+++ MAINTAINERS	(working copy)
@@ -268,6 +268,7 @@ dataflow 		Kenneth Zadeck		zadeck@natura
 driver			Joseph Myers		joseph@codesourcery.com
 Fortran			Janne Blomqvist		jb@gcc.gnu.org
 Fortran			Tobias Burnus		burnus@net-b.de
+Fortran			François-Xavier Coudert	fxcoudert@gcc.gnu.org
 Fortran			Jerry DeLisle		jvdelisle@gcc.gnu.org
 Fortran			Erik Edelmann		erik.edelmann@iki.fi
 Fortran			Daniel Franke		franke.daniel@gmail.com
@@ -356,7 +357,6 @@ William Cohen					wcohen@redhat.com
 Josh Conner					jconner@apple.com
 R. Kelley Cook					kcook@gcc.gnu.org
 Christian Cornelssen				ccorn@cs.tu-berlin.de
-François-Xavier Coudert				fxcoudert@gcc.gnu.org
 Cary Coutant					ccoutant@google.com
 Lawrence Crowl					crowl@google.com
 Ian Dall					ian@beware.dropbear.id.au

Fix some more decl types in the Fortran frontend

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

This shows up as failures of gfortran.dg/bessel_[67].f90 on ptx. 
According to the man page, the correct declarations are

double jn(int n, double x);
double yn(int n, double x);

and our calls match this, but the argument types are switched in the 
decls built by the Fortran frontend. On ptx, such errors are diagnosed 
by the assembler. Fixed by the following patch, bootstrapped and tested 
on x86_64-linux. Ok?

I'm also seeing some new Fortran testsuite failures after the recent 
merge to gomp-4_0-branch, due to a similar issue. That one has been 
harder to figure out, one of the affected tests is 
gfortran.dg/array_assignment_5.f90.  Here, we call 
_gfortran_spread_char_scalar, which is declared in the library as a 
function taking 6 args, and we pass 6 args to it. However, the decl for 
it only has 5 arguments. It's unclear to me where the problem is in the 
construction of the argument list for the decl. I'm also unsure why this 
has shown up only very recently.


Bernd

[-- Attachment #2: gfc-bessel.diff --]
[-- Type: text/x-patch, Size: 1091 bytes --]

commit 87c261fd190c9dea09a793b1295e7cff9bb6044e
Author: Bernd Schmidt <bernds@codesourcery.com>
Date:   Wed Sep 10 18:02:53 2014 +0200

    Fix type mismatches in intrinsic functions.
    
    	* f95-lang.c (build_builtin_fntypes): Switch order of args for type
    	with index 2.

diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
index da3a0d0..27cfc87 100644
--- a/gcc/fortran/f95-lang.c
+++ b/gcc/fortran/f95-lang.c
@@ -608,9 +608,9 @@ build_builtin_fntypes (tree *fntype, tree type)
   fntype[0] = build_function_type_list (type, type, NULL_TREE);
   /* type (*) (type, type) */
   fntype[1] = build_function_type_list (type, type, type, NULL_TREE);
-  /* type (*) (type, int) */
-  fntype[2] = build_function_type_list (type,
-                                        type, integer_type_node, NULL_TREE);
+  /* type (*) (int, type) */
+  fntype[2] = build_function_type_list (type, integer_type_node, type,
+                                        NULL_TREE);
   /* type (*) (void) */
   fntype[3] = build_function_type_list (type, NULL_TREE);
   /* type (*) (type, &int) */