[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 Paul Thomas changed: What|Removed |Added Status|NEW |RESOLVED Resolution||FIXED --- Comment #14 from Paul Thomas 2011-05-26 21:18:23 UTC --- Fixed on trunk and 4.6 Paul
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #13 from Paul Thomas 2011-05-26 20:49:11 UTC --- Author: pault Date: Thu May 26 20:49:07 2011 New Revision: 174308 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174308 Log: 2011-05-26 Paul Thomas Thomas Koenig PR fortran/48955 * trans-expr.c (gfc_trans_assignment_1): GFC_REVERSE_NOT_SET changed to GFC_ENABLE_REVERSE. * trans-array.c (gfc_init_loopinfo): GFC_CANNOT_REVERSE changed to GFC_INHIBIT_REVERSE. * gfortran.h : Enum gfc_reverse is now GFC_ENABLE_REVERSE, GFC_FORWARD_SET, GFC_REVERSE_SET and GFC_INHIBIT_REVERSE. * dependency.c (gfc_dep_resolver): Change names for elements of gfc_reverse as necessary. Change the logic so that forward dependences are remembered as well as backward ones. When both have appeared, force a temporary. 2011-05-26 Thomas Koenig PR fortran/48955 * gfortran.dg/dependency_40.f90 : New test. Added: branches/gcc-4_6-branch/gcc/testsuite/gfortran.dg/dependency_40.f90 Modified: branches/gcc-4_6-branch/gcc/fortran/ChangeLog branches/gcc-4_6-branch/gcc/fortran/dependency.c branches/gcc-4_6-branch/gcc/fortran/gfortran.h branches/gcc-4_6-branch/gcc/fortran/trans-array.c branches/gcc-4_6-branch/gcc/fortran/trans-expr.c branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #12 from Paul Thomas 2011-05-26 18:19:40 UTC --- Author: pault Date: Thu May 26 18:19:36 2011 New Revision: 174302 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174302 Log: 2011-05-26 Paul Thomas Thomas Koenig PR fortran/48955 * trans-expr.c (gfc_trans_assignment_1): GFC_REVERSE_NOT_SET changed to GFC_ENABLE_REVERSE. * trans-array.c (gfc_init_loopinfo): GFC_CANNOT_REVERSE changed to GFC_INHIBIT_REVERSE. * gfortran.h : Enum gfc_reverse is now GFC_ENABLE_REVERSE, GFC_FORWARD_SET, GFC_REVERSE_SET and GFC_INHIBIT_REVERSE. * dependency.c (gfc_dep_resolver): Change names for elements of gfc_reverse as necessary. Change the logic so that forward dependences are remembered as well as backward ones. When both have appeared, force a temporary. 2011-05-26 Thomas Koenig PR fortran/48955 * gfortran.dg/dependency_40.f90 : New test. Added: trunk/gcc/testsuite/gfortran.dg/dependency_40.f90 Modified: trunk/gcc/fortran/ChangeLog trunk/gcc/fortran/dependency.c trunk/gcc/fortran/gfortran.h trunk/gcc/fortran/trans-array.c trunk/gcc/fortran/trans-expr.c trunk/gcc/testsuite/ChangeLog
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #11 from paul.richard.thomas at gmail dot com 2011-05-24 09:43:32 UTC --- Dear Thomas, > With your patch, what is the difference between GFC_CAN_REVERSE > and GFC_REVERSE_NOT_SET? Perhaps GFC_REVERSE_ENABLED and GFC_REVERSE_INHIBITED would be better. > > And why do you initialize loop.reverse close to line 6052 and > close to line 6070 of trans-expr.c? Is something in between > changing this? Pure cock-up, I think. Clearly we only need one instance of this. However, when I monitored the beviour of the code in dependency.c, I noticed that the reversing was disabled and so added the second block, whilst failing to notice the first. Either I screwed up or your last question is pertinent. I will look this afternoon. Many thanks Paul PS I will post the patch this afternoon.
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #10 from Thomas Koenig 2011-05-23 20:01:10 UTC --- Hi Paul, just two questions, for my understanding: With your patch, what is the difference between GFC_CAN_REVERSE and GFC_REVERSE_NOT_SET? And why do you initialize loop.reverse close to line 6052 and close to line 6070 of trans-expr.c? Is something in between changing this? Regards Thomas
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #9 from Paul Thomas 2011-05-22 18:28:18 UTC --- Created attachment 24332 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24332 A fix for the PR This uses the same basic idea as Thomas' patch but is based on the original logic
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #8 from Thomas Koenig 2011-05-21 15:12:47 UTC --- Created attachment 24320 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24320 Tentative patch Paul, what do you think of this approach? It fixes the test case, and passes regression-testing.
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #7 from tkoenig at netcologne dot de 2011-05-16 18:10:03 UTC --- Hi Paul, > Indeed - I just need to find the time to sort out the logic. > Structurally the patch is OK. I think the logic could be as follows: You could have two flags, one FORWARD_ALLOWED and one BACKWARD_ALLOWED. Initialize both flags to true. Run through all the references which could introduce a dependency. If there is a GFC_EQUAL dependency, do nothing. If there is a GFC_FORWARD dependency, set BACKWARD_ALLOW to false. If there is a GFC_BACKWARD dependency, set FORWARD_ALLOW to false. When selecting the loop direction: - If FORWARD_ALLOW is set, select a forward loop; else If BACKWARD_ALLOW is set, select a forward loop; else mark this dimension as needing a temporary. Does this sound OK? Thomas
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #6 from paul.richard.thomas at gmail dot com 2011-05-16 12:48:32 UTC --- Indeed - I just need to find the time to sort out the logic. Structurally the patch is OK. Cheers Paul On Mon, May 16, 2011 at 9:56 AM, burnus at gcc dot gnu.org wrote: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 > > --- Comment #5 from Tobias Burnus 2011-05-16 > 07:27:07 UTC --- > Submitted patch: http://gcc.gnu.org/ml/fortran/2011-05/msg00090.html > It fixes the test case of comment 0, but (cf. review comment) it does not > handle a modified version. > > -- > Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email > --- You are receiving this mail because: --- > You are on the CC list for the bug. > You are the assignee for the bug. >
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #5 from Tobias Burnus 2011-05-16 07:27:07 UTC --- Submitted patch: http://gcc.gnu.org/ml/fortran/2011-05/msg00090.html It fixes the test case of comment 0, but (cf. review comment) it does not handle a modified version.
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #4 from Paul Thomas 2011-05-11 19:57:21 UTC --- Created attachment 24229 --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24229 A fix for the PR This fixes the problem in two steps: (i) It reverts r162289; and (ii) It adds the correct initialization for loop.reverse[] in gfc_trans_assignment_1. This was implemented incorrectly in the fix for PR24524 (in spite of the correct comment in dependency.c!) and removed at sometime, I do not know why. sigh Bootstraps and regtests on x86_64/FC9. I will add a testcase and submit formally tomorrow night. There are probably other assignements, such as in FOR and WHERE blocks that could do with reversing being enabled. I'll put thinking cap on after I have fixed this PR. Paul
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955
--- Comment #3 from Thomas Koenig 2011-05-11
19:00:06 UTC ---
Hmm... I wonder if this does the trick? It fixes the test case,
and passes all regression tests... Paul, what do you think?
Index: dependency.c
===
--- dependency.c(Revision 173389)
+++ dependency.c(Arbeitskopie)
@@ -1822,7 +1822,6 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf
/* Inhibit loop reversal if dependence not compatible. */
if (reverse && reverse[n] != GFC_REVERSE_NOT_SET
&& this_dep != GFC_DEP_EQUAL
- && this_dep != GFC_DEP_BACKWARD
&& this_dep != GFC_DEP_NODEP)
{
reverse[n] = GFC_CANNOT_REVERSE;
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 Jakub Jelinek changed: What|Removed |Added Priority|P3 |P4 CC||jakub at gcc dot gnu.org
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 Paul Thomas changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2011.05.11 09:25:24 AssignedTo|unassigned at gcc dot |pault at gcc dot gnu.org |gnu.org | Ever Confirmed|0 |1
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 --- Comment #2 from Tobias Burnus 2011-05-11 09:06:30 UTC --- The obvious problem is that there is no temporary needed for either of the variable expressions on the right hand side - only for their combination. Thus, one needs to check whether as some point whether one part of the RHS expression is GFC_DEP_FORWARD while another part is GFC_DEP_BACKWARD. My impression is that one needs an additional argument to gfc_check_dependency of the type "gfc_reverse *reverse" and two local variables like gfc_reverse reverse1, reverse2; which one passes to gfc_check_dependency/gfc_dep_resolver, and - if "reverse" is not NULL, propagates it to the caller. Then, for operators, the reverse setting can be compared.
[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955 Tobias Burnus changed: What|Removed |Added Known to work||4.3.4, 4.4.0, 4.5.3 Target Milestone|--- |4.6.1 Known to fail||4.1.2, 4.6.0, 4.7.0 --- Comment #1 from Tobias Burnus 2011-05-10 21:10:10 UTC --- Works: 2010-08-02 r162824 cca7236e5fabb3791d494683d1f53f3c09d545e5 Fails: 2010-08-02 r162829 644e9dad11b9ba317bd11726569b5d8bc648950f Thus, the culprit is: New Revision: 162829 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162829 2010-08-02 Thomas Koenig PR fortran/45159 * depencency.c (gfc_dep_resolver): Fix logic for when a loop can be reversed. 2010-08-02 Thomas Koenig PR fortran/45159 * gfortran.dg/dependency_29.f90: New test.
