On Linux/x86, revision 163781 gave FAIL: gfortran.dg/pr25923.f90 -O (test for warnings, line 13) FAIL: gfortran.dg/pr25923.f90 -O (test for excess errors) Revision 163775 is OK. It may be caused by revision 163776: http://gcc.gnu.org/ml/gcc-cvs/2010-09/msg00067.html
That's a -m32 (fails) vs. -m64 (works) issue, cf. http://gcc.gnu.org/ml/fortran/2010-09/msg00062.html
I think I'll try doing it the IIUC documented preferred way for deferred bugs; to xfail the test-case for ilp32 targets with a reference to this PR. And yes, seen by the cris-elf autotester too.
(In reply to comment #2) > xfail the test-case for ilp32 targets with a reference to this PR. <http://gcc.gnu.org/ml/fortran/2010-09/msg00179.html>
This isn't specific to Fortran, struct S { int a; }; struct S foo (struct S *arg) { struct S s; if (bar ()) baz ("not valid"); else s = *arg; return s; } has the exactly same difference. I believe this is caused by ESRA or gimplification. In *.ealias we have: <bb 5>: [pr25923.f90 : 22:0] D.1570 = res; return D.1570; and in *.esra this is: <bb 5>: # res$yr_9 = PHI <res$yr_8(D)(3), [pr25923.f90 : 20:0] res$yr_3(4)> [pr25923.f90 : 22:0] SR.2_10 = res$yr_9; D.1570.yr = SR.2_10; return D.1570; Note no locus on D.1570.yr = SR.2_10; assignment. The question is if ESRA just forgots to set the locus, or if it sets it from the following stmt (GIMPLE_RETURN). If the former, then it would just be a SRA bug, if the latter, then the question is why don't we set location on GIMPLE_RETURN. Note that on the C testcase there apparently is locus on GIMPLE_RETURN between gimplification and gimple lowering (where the locus on it is lost). On the Fortran testcase in *.gimple the locus is strange: [pr25923.f90 : 22:0] D.1570 = res; [pr25923.f90 : 13:0] return D.1570; Line 22 is the END FUNCTION line, line 13 is the FUNCTION line. For C it is obvious where it wants to report the return, for Fortran, given that it doesn't have any kind of RETURN statement, I guess either of the locations is fine, but we should be consistent.
Subject: Bug 45505 Author: hp Date: Tue Sep 7 13:23:24 2010 New Revision: 163949 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163949 Log: PR fortran/45505 * gfortran.dg/pr25923.f90: XFAIL warning on wrong line for ilp32. Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gfortran.dg/pr25923.f90
Jakub, I have not understood whether you think the warning emitted when compiling the c code from comment #4 has the correct line number or not. I see it attributed to the line with the return statement which I think is correct. Nevertheless, SRA currently does not set locations of gimple statements it generates and is quite confused when it comes to tree locations. I was already looking into the latter problem and added setting gimple statement locations appropriately because it fitted in well. The patch (http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01175.html) has just been approved and I am going to commit it in a few moments but it does not change the line the Fortran warning is reported at. It is probably because the gimple return statement, which is where I get the location for the newly generated load, has a location corresponding to the end of the function.
You now set the location, but I believe the wrong one. esra changes are: foo (struct S * arg) { + int SR.1; + int s$a; struct S s; struct S D.2694; int D.2690; @@ -72,10 +37,12 @@ foo (struct S * arg) goto <bb 5>; <bb 4>: - [pr25923.c : 13:7] s = [pr25923.c : 13] *arg_2(D); + [pr25923.c : 13:7] s$a_9 = [pr25923.c : 13] MEM[(struct S *)arg_2(D)]; <bb 5>: - [pr25923.c : 14:3] D.2694 = s; + # s$a_8 = PHI <s$a_7(D)(3), [pr25923.c : 13:7] s$a_9(4)> + [pr25923.c : 14:3] SR.1_10 = s$a_8; + MEM[(struct S *)&D.2694] = SR.1_10; return D.2694; } The added MEM = SR.1_10 uses location_t of the stmt after it, as sra_modify_expr emits statements before gsi. I'm arguing that in this case the MEM[(struct S *)&D.2694] = SR.1_10; stmt is not related to return D.2694, but to D.2694 = s that has been removed and thus I believe it should inherit its locus as well.
(In reply to comment #7) > > The added MEM = SR.1_10 uses location_t of the stmt after it, as > sra_modify_expr > emits statements before gsi. I'm arguing that in this case the MEM[(struct S > *)&D.2694] = SR.1_10; stmt is not related to return D.2694, but to D.2694 = s > that has been removed and thus I believe it should inherit its locus as well. > Unfortunately no, the statement SR.1_10 = s$a_8; is produced when replacing the original assignment, MEM[(struct S *)&D.2694] = SR.1_10; is created when processing the return statement, it is updating the original agregate D.2694 with data in its replacements (in this case there is only one but there can be more) before it is used as a whole. I can't see how it could be otherwise. It would be difficult also to set the location of the MEM_REF statement according to the definition of its RHS because when the statement is built the RHS is not yet in SSA form. Thinking about it more, the RHS in that statement, SR.1_10 has its TREE_NO_WARNING set and so that line should not be a problem at this stage. So I guess s$a_8 is propagated there at some point later. Perhaps the fwprop could change the location when it substitutes an operand in situations like this? I know it would be a bit ugly but doing the same in SRA would require some aggregate data flow analysis which would be quite hard.
I have just examined the Fortran testcase more thoroughly and to my surprise I realized SRA did not create any new statements on i686. It merely changed statements res = *arg_1(D); into res$yr = MEM[(struct bar &)arg_1(D)]; and <retval> = res; into MEM[(struct bar *)&<retval>] = res$yr; both by simply changing the LHS and RHS. Needless to say, the locations of the statements remain unchanged. Locations for the new memory references have been taken from the statements.
gimple_has_location returns false for the return statement on both i686 and x86_64. When I hacked SRA to set the location of return statement to the location of the preceding statement ("<retval> = res;" or "D.1523 = res;"), the reported lines in the warnings were consistent on both i686 and x86_64 (22 in both cases, of course). So making sure the two statements both have the location we want to give in the warning is a way of dealing with this issue. To me it even seems they do belong to the same source construct and should have the same location.
The reason why the return stmt, at least after lowering, doesn't have a location, is because after lowering there is just one return instead of possibly multiple returns from before lowering. So the location_t of the individual returns is preserved on the gimple assignments to the RESULT_DECL. What I just find strange is why is the return stmt involved in the SRA optimization (except as unrelated stmt following the deleted stmt). There was an assignment to RESULT_DECL before that, it had the intended locus of the return from the source, and I'd say that the replacements are connected to that statements if the RESULT_DECL can't be scalarized.
I have been seeing this XPASS on x86_64-apple-darwin10 at -m32... Executing on host: /sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/gfortran/../../gfortran -B/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/gfortran/../../ /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20101007/gcc/testsuite/gfortran.dg/pr25923.f90 -O -O -Wuninitialized -S -m32 -o pr25923.s (timeout = 300) /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20101007/gcc/testsuite/gfortran.dg/pr25923.f90: In function 'baz':^M /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20101007/gcc/testsuite/gfortran.dg/pr25923.f90:13:0: warning: 'res.yr' may be used uninitialized in this function [-Wuninitialized]^M output is: /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20101007/gcc/testsuite/gfortran.dg/pr25923.f90: In function 'baz':^M /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20101007/gcc/testsuite/gfortran.dg/pr25923.f90:13:0: warning: 'res.yr' may be used uninitialized in this function [-Wuninitialized]^M XPASS: gfortran.dg/pr25923.f90 -O PR45505 (test for warnings, line 13) XPASS: gfortran.dg/pr25923.f90 -O PR45505 (test for bogus messages, line 22) PASS: gfortran.dg/pr25923.f90 -O (test for excess errors) Is that expected?
I don't think this is a P1, the warning is reported, just with differing locus.
Interestingly, on x86_64-apple-darwin10, when using a --enable-build-with-cxx bootstrap, these tests change to... XPASS: gfortran.dg/pr25923.f90 -O PR45505 (test for warnings, line 13) XPASS: gfortran.dg/pr25923.f90 -O PR45505 (test for bogus messages, line 22) I don't see the same behavior with a --enable-build-with-cxx bootstrap on x86_64 Fedora10 however.
I've played around with this a bit more and came to the conclusion that we could refine SRA heuristics some more to not scalarize this if we added two more attributes to struct access, one meaning "read as a scalar" and another for "written as a scalar." (I'm quite confident this would work, I have a different patch that works too but it uses a rather ad-hoc approach). However, I'm not sure whether we should be adding more attributes when we have already quite a few just in order to be able to make slightly better judgments about single-field structures like this one. (Maybe we really could have a location for return instead?). In either case, it is nothing for stage4. BTW, is this even a 4.6 regression?
The test "XPASSes" with -m32 on *86*-apple-darwin* (see http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02532.html http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02281.html ). The following patch cleans it. --- ../_clean/gcc/testsuite/gfortran.dg/pr25923.f90 2010-09-07 15:25:44.000000000 +0200 +++ gcc/testsuite/gfortran.dg/pr25923.f90 2011-01-09 14:13:01.000000000 +0100 @@ -10,7 +10,7 @@ implicit none contains - function baz(arg) result(res) ! { dg-warning "res.yr' may be" "PR45505" { xfail ilp32 } } + function baz(arg) result(res) ! { dg-warning "res.yr' may be" "PR45505" { xfail { ilp32 && { ! *86*-apple-darwin* } } } } type(bar), intent(in) :: arg type(bar) :: res logical, external:: some_func @@ -19,7 +19,7 @@ contains else res = arg end if - end function baz ! { dg-bogus "res.yr' may be" "PR45505" { xfail ilp32 } } + end function baz ! { dg-bogus "res.yr' may be" "PR45505" { xfail { ilp32 && { ! *86*-apple-darwin* } } } } end module foo Note also that this is also true for some hppa*-*-* and i386-unknown-freebsd9.0 (see http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02550.html http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02368.html http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02365.html ), while the test fails on alpha-dec-osf5.1b and on powerpc64-unknown-linux-gnu with -m64 (see http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02158.html http://gcc.gnu.org/ml/gcc-testresults/2011-01/msg02626.html ). I can adjust the above patch for these cases (and put *86* in a more canonical form), but I am not able to test it for these platforms.
Thanks, Dominique. I think it'll be great if you can add i386-*-freebsd*. I'll be happy to test a patch, but I think in this specific case it's just fine if you commit and we'll have my nightly tester pick it up.
The test case XPASSes on spu-*-* as well.
I have posted a proposed fix to the mailing list: http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00538.html
Author: jamborm Date: Wed Feb 9 11:48:09 2011 New Revision: 169964 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=169964 Log: 2011-02-09 Martin Jambor <mjambor@suse.cz> PR middle-end/45505 * tree-sra.c (struct access): New flags grp_scalar_read and grp_scalar_write. Changed description of assignment read and write flags. (dump_access): Dump new flags, reorder all of them. (sort_and_splice_var_accesses): Set the new flag accordingly, use them to detect multiple scalar reads. (analyze_access_subtree): Use the new scalar read write flags instead of the old flags. Adjusted comments. * testsuite/gfortran.dg/pr25923.f90: Remove xfails. Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gfortran.dg/pr25923.f90 trunk/gcc/tree-sra.c
Fixed.