Re: Someone has caused regressions in gfortran (c_char_tests_red.f03, now PR33330)
Dominique Dhumieres wrote: In comment #7 of PR0, Richard Guenther asked the following question I cannot answer: Btw, is it mandated by the fortran standard to pass a scalar as array reference? Does anyone knows the answer? or should it be asked on comp.lang.fortran? Here, it looks as if you mean passing a character string of length 1 as a variable length string. Certainly, this should be no problem. Pardon me if I misunderstood. In either case, there should be plenty of references in c.l.f archives. In general, passing a scalar where an array reference is required is non-standard and a serious portability issue. Use of module or interface syntax should cause any problems to be diagnosed. For CHARACTER type, there is a distinction between an array (of character strings, possibly of length 1) and a scalar character string.
Re: Someone has caused regressions in gfortran (c_char_tests_red.f03, now PR33330)
On 9/7/07, Tim Prince [EMAIL PROTECTED] wrote: Dominique Dhumieres wrote: In comment #7 of PR0, Richard Guenther asked the following question I cannot answer: Btw, is it mandated by the fortran standard to pass a scalar as array reference? Does anyone knows the answer? or should it be asked on comp.lang.fortran? Here, it looks as if you mean passing a character string of length 1 as a variable length string. Certainly, this should be no problem. Pardon me if I misunderstood. In either case, there should be plenty of references in c.l.f archives. In general, passing a scalar where an array reference is required is non-standard and a serious portability issue. Use of module or interface syntax should cause any problems to be diagnosed. For CHARACTER type, there is a distinction between an array (of character strings, possibly of length 1) and a scalar character string. What I was after in this particular context is that the miscompilation would have not occured if the frontend passed the character as char* and did the de-reference as plain indirect reference instead of an array reference. Richard.
Re: Someone has caused regressions in gfortran (c_char_tests_red.f03, now PR33330)
Salut Dominique, moin Richard, hello all, (Answering Richard's question from PR0.) Dominique Dhumieres wrote: Btw, is it mandated by the fortran standard to pass a scalar as array reference? Does anyone knows the answer? or should it be asked on comp.lang.fortran? The standard mandates that (when the dummy argument has no VALUE attribute) variables are passed as reference; I'm pretty sure that the rest is implementation dependent. I think the question arose form: character :: my_char which is a scalar character variable which takes exactly one character. However, the following is also a scalar variable: character(len=10) :: my_char which contains 10 characters. It has the same storage size as an array with ten elements with one character each: character(len=1), dimension(10) :: my_char As a scalar character variable with len 1 needs an array reference, it is quite natural to pass also a scalar variable with length 1 as array reference. I think the standard also allows to pass it as non-array reference, which happens (for obvious reasons) if one uses C Bindings. (In this case the standard mandates len=1; for strings one has thus to use an array.) Hmm, thinking it over, I think on can say that the standard mandates that an array reference is passed. Let's assume two functions, expecting, respectively, character(len=1) :: arg character(len=*) :: arg (len=* allows for any lengths; the length is passed as an additional argument in both cases.) In the main program I do now: call mySubroutine( 'a' ) in order to decide whether one needs to pass an array reference or not, one needs to know whether len=1 or len=*. This is only possible if one knows the explicit interface (= function definition); but Fortran allows also implicit interfaces (i.e. assume mySubroutine exists, it returns VOID and takes 0 to oo arguments of a certain but unknown type). Tobias
Re: Someone has caused regressions in gfortran (c_char_tests_red.f03, now PR33330)
Does anyone knows the answer? or should it be asked on comp.lang.fortran? It's very specific to the problem at hand, so I doubt c.l.f could give us much input on that. As I understand, in this case, it actually is the right thing to do. FX
Re: Someone has caused regressions in gfortran (c_char_tests_red.f03, now PR33330)
In comment #7 of PR0, Richard Guenther asked the following question I cannot answer: Btw, is it mandated by the fortran standard to pass a scalar as array reference? Does anyone knows the answer? or should it be asked on comp.lang.fortran? TIA Dominique
Re: Someone has caused regressions in gfortran (c_char_tests_red.f03, now PR33330)
This is now PR0 handled by Richard Guenther. Dominique
Re: Someone has caused regressions in gfortran
Sadly, the testsuite regressions don't seems to be fixed. I will try to figure out tomorrow why the function is still being inlined. The test case gfortran.dg/do_3.F90 pass with -fno-strict-overflow (see http://gcc.gnu.org/ml/fortran/2007-09/msg00116.html). I have posted at http://gcc.gnu.org/ml/fortran/2007-09/msg00107.html a reduced test case without inlining issues showing a similar breakage. If someone can show that before the recent failure the functions were not inlined, I think the failure would be fully explained. Otherwise it will require further investigation. As far as I can tell without -fno-strict-overflow the executable reduces to a call abort at the level of if (i /= final) call abort as if final = huge(to)+1_1 giving an overflow, the comparison is assuming to always fail. I remember a lot of traffic on the gcc mailing list a couple months ago about this kind of optimization and the reasons behind -fno-strict-overflow, but I dont have the time right now to look deeper. Dominique
Re: Someone has caused regressions in gfortran
Sadly, the testsuite regressions don't seems to be fixed. I will try to figure out tomorrow why the function is still being inlined. The test case gfortran.dg/do_3.F90 pass with -fno-strict-overflow (see http://gcc.gnu.org/ml/fortran/2007-09/msg00116.html). I have posted at http://gcc.gnu.org/ml/fortran/2007-09/msg00107.html a reduced test case without inlining issues showing a similar breakage. If someone can show that before the recent failure the functions were not inlined, I think the failure would be fully explained. Otherwise it will require further investigation. The testcase was indeed previously not inlined at all. Shall we add -fno-strict-overflow to the testcase then? Honza As far as I can tell without -fno-strict-overflow the executable reduces to a call abort at the level of if (i /= final) call abort as if final = huge(to)+1_1 giving an overflow, the comparison is assuming to always fail. I remember a lot of traffic on the gcc mailing list a couple months ago about this kind of optimization and the reasons behind -fno-strict-overflow, but I dont have the time right now to look deeper. Dominique
Re: Someone has caused regressions in gfortran
The testcase was indeed previously not inlined at all. Shall we add -fno-strict-overflow to the testcase then? This what I would do, but I am not qualified to make the call. In addition my working setup is totally broken right now (at stage1). Could you do the addition to the testcase and run the gfortran testsuite? From the result it would be easier to reach a conclusion. TIA Dominique
Re: Someone has caused regressions in gfortran (c_char_tests_red.f03)
I have done some investigation about the recent failure of
gfortran.dg/c_char_tests.f03. First the failure disappears
with -fno-inline or -fno-inline-functions:
[karma] f90/bug% gfc c_char_tests_db.f03 -O3 -fno-inline c_char_driver_db.c
[karma] f90/bug% a.out
[karma] f90/bug% gfc c_char_tests_db.f03 -O3 -fno-inline-functions
c_char_driver_db.c
[karma] f90/bug% a.out
Second, if I remove the line sub0();
in c_char_driver.c, the test succeeds, so the C driver can be reduced to:
[karma] f90/bug% cat c_char_driver_red.c
void sub0(void);
int main(int argc, char **argv)
{
char my_char = 'y';
sub0();
return 0;
}
with the same behavior. Now the stange part: if I try the following reduced
code:
! { dg-do run }
! { dg-additional-sources c_char_driver.c }
! Verify that character dummy arguments for bind(c) procedures can work both
! by-value and by-reference when called by either C or Fortran.
! PR fortran/32732
module c_char_tests
use, intrinsic :: iso_c_binding, only: c_char
implicit none
contains
subroutine param_test(my_char) bind(c)
character(c_char), value :: my_char
call sub1(my_char)
end subroutine param_test
subroutine sub0() bind(c)
call param_test('y')
end subroutine sub0
subroutine sub1(my_char_ref) bind(c)
character(c_char) :: my_char_ref
if(my_char_ref /= c_char_'y') call abort()
end subroutine sub1
end module c_char_tests
! { dg-final { cleanup-modules c_char_tests } }
I get:
[karma] f90/bug% gfc c_char_tests_red.f03 -O3 -fno-inline-functions
c_char_driver_red.c
[karma] f90/bug% a.out
Abort
[karma] f90/bug% gfc c_char_tests_red.f03 -O3 -fno-inline-functions -fno-inline
c_char_driver_red.c
[karma] f90/bug% a.out
Wierd, isn't it?
So if one wants an immediate clean test suite, add -fno-inline-functions.
Now clearly the new version of GCC inlines more than the previous one,
with two failing cases. The first one (do_3) is a very borderline one,
testing folding after integer overflows and I think the addition of
-fno-strict-overflow is enough. In my opinion, the second case requires
further investigation, but I don't think it would be a good idea to
try to prevent the new inlining (unless we discover that it open
another Pandora box).
Cheers
Dominique
Re: Someone has caused regressions in gfortran
The testcase was indeed previously not inlined at all. Shall we add -fno-strict-overflow to the testcase then? This what I would do, but I am not qualified to make the call. In addition my working setup is totally broken right now (at stage1). Could you do the addition to the testcase and run the gfortran testsuite? From the result it would be easier to reach a conclusion. Yep, it does fix the problem. Honza TIA Dominique
Re: Someone has caused regressions in gfortran
Because of the famous duplicated declaration problem This sentence is reminding me that I forgot to send the following update: As I said I was going to give it a shot over the week-end, here's an update on this: it won't make it into 4.3, because it's a big change and my current patch is triggering a very long string of ice-on-invalid-code bugs (all type mismatches in Fortran interfaces for procedures end up dying badly) as well as a few ice-on-valid-code that are currently hard to track (and might be preexisting front-end bugs exposed by the patch). I intend to work slowly on this, and hopefully will have put a complete patch together when 4.4 stage1 opens. I am not sure if inlining is not completely unsafe for fortan and we would not be forced to disable it completely (not just partly as before the patch). This would be rather sad. I think the current situation is safe: we can online local functions (functions declared and inside other functions), which are the Fortran CONTAIN'ed functions. This should be safe, while all other inlining is currently impossible. FX
Re: Someone has caused regressions in gfortran
Because of the famous duplicated declaration problem This sentence is reminding me that I forgot to send the following update: As I said I was going to give it a shot over the week-end, here's an update on this: it won't make it into 4.3, because it's a big change and my current patch is triggering a very long string of ice-on-invalid-code bugs (all type mismatches in Fortran interfaces for procedures end up dying badly) as well as a few ice-on-valid-code that are currently hard to track (and might be preexisting front-end bugs exposed by the patch). I intend to work slowly on this, and hopefully will have put a complete patch together when 4.4 stage1 opens. Huh, still I would be interested in seeing the patch. I am not sure if inlining is not completely unsafe for fortan and we would not be forced to disable it completely (not just partly as before the patch). This would be rather sad. I think the current situation is safe: we can online local functions (functions declared and inside other functions), which are the Fortran CONTAIN'ed functions. This should be safe, while all other inlining is currently impossible. Can we trick fotran to set DECL_UNINLINABLE in the non CONTAIN'ed functions? Honza FX
Re: Someone has caused regressions in gfortran
As I said I was going to give it a shot over the week-end, here's an update on this: it won't make it into 4.3, because it's a big change and my current patch is triggering a very long string of Huh, still I would be interested in seeing the patch. It's based on Michal Matz's patch at http://gcc.gnu.org/ml/gcc/2007-08/msg00236.html I'll send it tomorrow, I don't have my laptop with me today. Can we trick fotran to set DECL_UNINLINABLE in the non CONTAIN'ed functions? Yes, I think we can do that. Grep the front-end sources for FUNCTION_DECL as argument to build_decl: * the decl built in gfc_get_intrinsic_lib_fndecl (trans-intrinsic.c) can be made DECL_UNINLINABLE unconditionally * in trans-decl.c, the decls built in gfc_get_extern_function_decl and gfc_build_library_function_decl can also be made DECL_UNINLINABLE unconditionally * finally, in build_function_decl (trans-decl.c), you can do something like Index: trans-decl.c === --- trans-decl.c(revision 127902) +++ trans-decl.c(working copy) @@ -1217,6 +1217,8 @@ build_function_decl (gfc_symbol * sym) type = gfc_get_function_type (sym); fndecl = build_decl (FUNCTION_DECL, gfc_sym_identifier (sym), type); + if (!sym-attr.contained) +DECL_UNINLINABLE (fndecl) = 1; /* Perform name mangling if this is a top level or module procedure. */ if (current_function_decl == NULL_TREE) I have neither built nor regtested this, and I won't be able to do it in the next few days. If you feel like trying, please go ahead. FX
Re: Someone has caused regressions in gfortran
As I said I was going to give it a shot over the week-end, here's an update on this: it won't make it into 4.3, because it's a big change and my current patch is triggering a very long string of Huh, still I would be interested in seeing the patch. It's based on Michal Matz's patch at http://gcc.gnu.org/ml/gcc/2007-08/msg00236.html I'll send it tomorrow, I don't have my laptop with me today. Thanks, I am interested in it as I am thinking about solving this in cgraph for 4.4 - we discussed this briefly on GCCsummit - basically we can add an ABI for cgraph that will let frotnend to specify that two decls should be identified. The cgraph then can to pass over the function bodies and initializers, doing the replacements where possible adding VIEW_CONVERT_EXPRs on mismatches and possibly producing some useful diagnostics... Can we trick fotran to set DECL_UNINLINABLE in the non CONTAIN'ed functions? Yes, I think we can do that. Grep the front-end sources for FUNCTION_DECL as argument to build_decl: * the decl built in gfc_get_intrinsic_lib_fndecl (trans-intrinsic.c) can be made DECL_UNINLINABLE unconditionally * in trans-decl.c, the decls built in gfc_get_extern_function_decl and gfc_build_library_function_decl can also be made DECL_UNINLINABLE unconditionally * finally, in build_function_decl (trans-decl.c), you can do something like Thanks, I sent the patch for testing and lets see if it solves the problem. Honza Index: trans-decl.c === --- trans-decl.c(revision 127902) +++ trans-decl.c(working copy) @@ -1217,6 +1217,8 @@ build_function_decl (gfc_symbol * sym) type = gfc_get_function_type (sym); fndecl = build_decl (FUNCTION_DECL, gfc_sym_identifier (sym), type); + if (!sym-attr.contained) +DECL_UNINLINABLE (fndecl) = 1; /* Perform name mangling if this is a top level or module procedure. */ if (current_function_decl == NULL_TREE) I have neither built nor regtested this, and I won't be able to do it in the next few days. If you feel like trying, please go ahead. FX
Re: Someone has caused regressions in gfortran
Jan Hubicka wrote: Thanks, I sent the patch for testing and lets see if it solves the problem. If the testsuite passes, and you intend to commit this, please add a FIXME. Cheers, - Tobi Honza Index: trans-decl.c === --- trans-decl.c(revision 127902) +++ trans-decl.c(working copy) @@ -1217,6 +1217,8 @@ build_function_decl (gfc_symbol * sym) type = gfc_get_function_type (sym); fndecl = build_decl (FUNCTION_DECL, gfc_sym_identifier (sym), type); + if (!sym-attr.contained) +DECL_UNINLINABLE (fndecl) = 1; /* Perform name mangling if this is a top level or module procedure. */ if (current_function_decl == NULL_TREE) I have neither built nor regtested this, and I won't be able to do it in the next few days. If you feel like trying, please go ahead. FX
Re: Someone has caused regressions in gfortran
Jan Hubicka wrote: Thanks, I sent the patch for testing and lets see if it solves the problem. If the testsuite passes, and you intend to commit this, please add a FIXME. Sadly, the testsuite regressions don't seems to be fixed. I will try to figure out tomorrow why the function is still being inlined. Honza Cheers, - Tobi Honza Index: trans-decl.c === --- trans-decl.c(revision 127902) +++ trans-decl.c(working copy) @@ -1217,6 +1217,8 @@ build_function_decl (gfc_symbol * sym) type = gfc_get_function_type (sym); fndecl = build_decl (FUNCTION_DECL, gfc_sym_identifier (sym), type); + if (!sym-attr.contained) +DECL_UNINLINABLE (fndecl) = 1; /* Perform name mangling if this is a top level or module procedure. */ if (current_function_decl == NULL_TREE) I have neither built nor regtested this, and I won't be able to do it in the next few days. If you feel like trying, please go ahead. FX
Re: Someone has caused regressions in gfortran
Someone has committed a patch that is causing both gfortran.dg/do_3.F90 and gfortran.dg/c_char_tests.f03 to fail at -O3 on amd64-*-freebsd. A quick inspection of fortran/ChangeLog doesn't yield a pointer to any particular commit. This may be caused by some middle-end change, but I won't have time to narrow down the revision until later tonight. Actually I just looked into the testcases - they are caused by the inliner change (and the problem didn't appear at the original testing run). The problem is that fortran never set DECL_INLINE on anything, so we ended up not doing any automatic inlining with exception of inlining functions called once and at -Os. Inliner now makes it's own decision on auto inlining and thus we uncover the latent bug. Because of the famous duplicated declaration problem, I am not sure if inlining is not completely unsafe for fortan and we would not be forced to disable it completely (not just partly as before the patch). This would be rather sad. My fortran-fu is however not at level to figure out what precisely is going wrong in those two testcases. Honza -- Steve
Re: Someone has caused regressions in gfortran
On Tue, Sep 04, 2007 at 07:48:08PM -0700, Steve Kargl wrote: My fortran-fu is however not at level to figure out what precisely is going wrong in those two testcases. I'll try to reduce the do_3.F90 code to a minimum testcase. Unfortunately, my middle/back-end knowledge is probably much worse than your Fortran-fu. Honza, I've reduce do_3.F90 to the following code: program test integer :: count integer(kind=1) :: i1 integer(kind=1), parameter :: h = huge(h) count = 0 do i1 = -huge(i1)-1_1, huge(i1), 1_1 count = count + 1 end do if (test_i1(-h-1_1, h, 1_1, h+1_1) /= int(h) * 2 + 2) call abort contains function test_i1 (from, too, step, final) result(res) integer(kind=1), intent(in) :: from, too, step, final integer(kind=1) :: i integer :: res res = 0 do i = from, too, step res = res + 1 end do if (i /= final) call abort end function test_i1 end program test A couple comments: 1) AFAIK, gfortran will only inline CONTAIN'd function (see the contains statement above). There are essentially static functions private to the test program. 2) If I comment out either IF statement, then the test will not abort. In particular, in the function test_i1, i == final to the 'call abort' is never executed, but apparently inlining doesn't like the IF statement. 3) integer(kind=1) is equivalent to int8_t. I'll see if I can translate the Fortran into a failing C program. -- Steve
Re: Someone has caused regressions in gfortran
On Tue, Sep 04, 2007 at 08:37:15PM -0700, Steve Kargl wrote: On Tue, Sep 04, 2007 at 07:48:08PM -0700, Steve Kargl wrote: My fortran-fu is however not at level to figure out what precisely is going wrong in those two testcases. I'll try to reduce the do_3.F90 code to a minimum testcase. Unfortunately, my middle/back-end knowledge is probably much worse than your Fortran-fu. Honza, I've reduce do_3.F90 to the following code: program test integer :: count You can delete the above declaration. integer(kind=1) :: i1 integer(kind=1), parameter :: h = huge(h) count = 0 do i1 = -huge(i1)-1_1, huge(i1), 1_1 count = count + 1 end do You can also delete the above 4 lines. if (test_i1(-h-1_1, h, 1_1, h+1_1) /= int(h) * 2 + 2) call abort contains function test_i1 (from, too, step, final) result(res) integer(kind=1), intent(in) :: from, too, step, final integer(kind=1) :: i integer :: res res = 0 do i = from, too, step res = res + 1 end do if (i /= final) call abort end function test_i1 end program test A couple comments: 1) AFAIK, gfortran will only inline CONTAIN'd function (see the contains statement above). There are essentially static functions private to the test program. 2) If I comment out either IF statement, then the test will not abort. In particular, in the function test_i1, i == final to the 'call abort' is never executed, but apparently inlining doesn't like the IF statement. 3) integer(kind=1) is equivalent to int8_t. I'll see if I can translate the Fortran into a failing C program. -- Steve -- Steve
