| Message ID | CAKwh3qi-cPiexdr9+gPJwLQELX3u90tLchJ9L81cbkKwtbJQ+g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping! (Maybe I should have posted the follow-up patch in a separate thread to make it more visible.) 2011/8/13 Janus Weil <janus@gcc.gnu.org>: > Hi Thomas, hi all, > > 2011/8/7 Thomas Koenig <tkoenig@netcologne.de>: >> When extending the values of gfc_dep_compare_expr, we will need to go >> through all its uses (making sure we change == -2 to <= -2). > > attached is a patch which makes a start with this. > > For now, it changes the return value to "-3" for two cases: > 1) different expr_types > 2) non-identical variables > > I tried to take care of all places which are checking for a return > value of "-2" and I hope I missed none. > > Any objections or ok for trunk? (Regtested successfully.) > > Cheers, > Janus > > > 2011-08-13 Janus Weil <janus@gcc.gnu.org> > > PR fortran/49638 > * dependency.c (gfc_dep_compare_expr): Add new result value "-3". > (gfc_check_element_vs_section,gfc_check_element_vs_element): Handle > result value "-3". > * frontend-passes.c (optimize_comparison): Ditto. > * interface.c (gfc_check_typebound_override): Ditto. > > > 2011-08-13 Janus Weil <janus@gcc.gnu.org> > > PR fortran/49638 > * gfortran.dg/typebound_override_1.f90: Modified. >
On Friday 19 August 2011 12:05:02 Janus Weil wrote: > Ping! (Maybe I should have posted the follow-up patch in a separate > thread to make it more visible.) I saw it, had a quick glance, thought that Thomas would jump on it, and forgot. Sorry. > > 2011/8/13 Janus Weil <janus@gcc.gnu.org>: > > Hi Thomas, hi all, > > > > 2011/8/7 Thomas Koenig <tkoenig@netcologne.de>: > >> When extending the values of gfc_dep_compare_expr, we will need to go > >> through all its uses (making sure we change == -2 to <= -2). > > > > attached is a patch which makes a start with this. > > > > For now, it changes the return value to "-3" for two cases: > > 1) different expr_types > > 2) non-identical variables > > > > I tried to take care of all places which are checking for a return > > value of "-2" and I hope I missed none. > > > > Any objections or ok for trunk? (Regtested successfully.) OK from my side for the code proper. I have one comment though about this: +/* Compare two expressions. Return values: + * +1 if e1 > e2 + * 0 if e1 == e2 + * -1 if e1 < e2 + * -2 if the relationship could not be determined + * -3 if e1 /= e2, but we cannot tell which one is larger. */ I think this is misleading, as the function does not always return -3 when e1/=e2. There is for example (currently) no special handling for operators. Here is an attempt at expressing it: * -3 in some cases where we could determine that e1 and e2 have different data dependencies (and thus are not guaranteed to have always the same value), but we cannot tell whether one is greater than the other. Mikael
On 08/19/2011 01:55 PM, Mikael Morin wrote: > OK from my side for the code proper. > > I have one comment though about this: > +/* Compare two expressions. Return values: > + * +1 if e1> e2 > + * 0 if e1 == e2 > + * -1 if e1< e2 > + * -2 if the relationship could not be determined > + * -3 if e1 /= e2, but we cannot tell which one is larger. */ > > I think this is misleading, as the function does not always return -3 when > e1/=e2. There is for example (currently) no special handling for operators. > Here is an attempt at expressing it: > * -3 in some cases where we could determine that e1 and e2 have different > data dependencies (and thus are not guaranteed to have always the same value), > but we cannot tell whether one is greater than the other. Besides that issue, I am wondering whether we shouldn't start to use an ENUM for those. I think for "<" vs. "==" vs. ">" one can use a number (-1, 0, 1) and then compare the result against 0 (>0, == 0 etc.). However, for 5 values, I think it makes sense to do something else otherwise, someone write "... < 0" which not only matches -1 but also -2 or -3. I think this does not block the committal but one should think about whether one should do it as follow up. Tobias, who has not read the patch.
>> > 2011/8/7 Thomas Koenig <tkoenig@netcologne.de>: >> >> When extending the values of gfc_dep_compare_expr, we will need to go >> >> through all its uses (making sure we change == -2 to <= -2). >> > >> > attached is a patch which makes a start with this. >> > >> > For now, it changes the return value to "-3" for two cases: >> > 1) different expr_types >> > 2) non-identical variables >> > >> > I tried to take care of all places which are checking for a return >> > value of "-2" and I hope I missed none. >> > >> > Any objections or ok for trunk? (Regtested successfully.) > OK from my side for the code proper. Thanks for the review. > I have one comment though about this: > +/* Compare two expressions. Return values: > + * +1 if e1 > e2 > + * 0 if e1 == e2 > + * -1 if e1 < e2 > + * -2 if the relationship could not be determined > + * -3 if e1 /= e2, but we cannot tell which one is larger. */ > > I think this is misleading, as the function does not always return -3 when > e1/=e2. That's right. However, the same argument applies to the other values as well: The function does not always return 0 if e1==e2. There could be cases where the arguments are algebraically equal, but we fail to detect this (example: A+B+C vs C+B+A). This sort of "uncertainty" was not introduced by me, but was present before, and is not special to the value "-3". Describing the value -2 as "relationship could not be determined" sort of implies that this can happen. So I would tend to leave the description as it is. > There is for example (currently) no special handling for operators. Well, unfortunately one cannot just return "-3" for non-matching operators. Just think of cases like A*(B+C) vs A*B+A*C. One could try to handle such cases in a follow-up patch. I'll commit the patch (as posted) tomorrow, if Mikael agrees that the description is ok. Also I like Tobias' idea of using an enum, but I'll leave it for a follow-up. Cheers, Janus
On Friday 19 August 2011 23:54:45 Janus Weil wrote: > > I have one comment though about this: > > +/* Compare two expressions. Return values: > > + * +1 if e1 > e2 > > + * 0 if e1 == e2 > > + * -1 if e1 < e2 > > + * -2 if the relationship could not be determined > > + * -3 if e1 /= e2, but we cannot tell which one is larger. */ > > > > I think this is misleading, as the function does not always return -3 > > when e1/=e2. > > That's right. However, the same argument applies to the other values > as well: The function does not always return 0 if e1==e2. There could > be cases where the arguments are algebraically equal, but we fail to > detect this (example: A+B+C vs C+B+A). This sort of "uncertainty" was > not introduced by me, but was present before, and is not special to > the value "-3". > > Describing the value -2 as "relationship could not be determined" sort > of implies that this can happen. So I would tend to leave the > description as it is. OK, this comment really bugged me, but it's not that bad on second thought. > > > There is for example (currently) no special handling for operators. > > Well, unfortunately one cannot just return "-3" for non-matching > operators. Just think of cases like A*(B+C) vs A*B+A*C. Ah yes. I was thinking expressions themselves were compared; but only their values are. > One could try to handle such cases in a follow-up patch. If you want. I wasn't asking you (or anyone else) to do it. > > I'll commit the patch (as posted) tomorrow, if Mikael agrees that the > description is ok. It's fine. Thanks. Mikael.
>> > There is for example (currently) no special handling for operators. >> >> Well, unfortunately one cannot just return "-3" for non-matching >> operators. Just think of cases like A*(B+C) vs A*B+A*C. > Ah yes. I was thinking expressions themselves were compared; but only their > values are. I'm not sure I'm getting you right here. Of course we do compare the expressions themselves. However, for example things like commutativity of operators are taken into account, meaning we compare "A+B" equal to "B+A" (A and B being arbitrary expressions). Taking care of other algebraic transformations (like e.g. distributivity as mentioned above) will be a bit harder. And the question is we are even allowed to do it. Earlier in this thread Steve mentioned restrictions like Note 7.18. X*(Y-Z) -> X*Y - X*Z is a forbidden transformation (there is no noted restriction on Z > 0). >> I'll commit the patch (as posted) tomorrow, if Mikael agrees that the >> description is ok. > It's fine. Thanks. Committed as r177932. Thanks again for your review and comments. Cheers, Janus
On Saturday 20 August 2011 21:29:21 Janus Weil wrote: > >> > There is for example (currently) no special handling for operators. > >> > >> Well, unfortunately one cannot just return "-3" for non-matching > >> operators. Just think of cases like A*(B+C) vs A*B+A*C. > > > > Ah yes. I was thinking expressions themselves were compared; but only > > their values are. > > I'm not sure I'm getting you right here. Of course we do compare the > expressions themselves. Yes, what I mean is... > However, for example things like commutativity > of operators are taken into account, meaning we compare "A+B" equal to > "B+A" (A and B being arbitrary expressions). ... "A+B" and "B+A" are different expressions, with the same value. And we return 0 (<=> equality) in that case. So we are interested in same- value-ness, not same-expression-ness. And we do compare expressions, because same expresssion ===> same value. But different expression =/=> different value (as your example shows). Oh well, nevermind. Mikael
If we really wanted to do this The Right Way, there would be seven cases to be considered, best expressed as three flags. I'll call them CAN_BE_LESS, CAN_BE_EQUAL and CAN_BE_MORE. Comparing a vs. a+1 would yield CAN_BE_LESS for integers and CAN_BE_LESS | CAN_BE_EQUAL for floats. Comparing 3 vs. 4 would yield CAN_BE_LESS. Comparing a vs. 5 would yield CAN_BE_LESS | CAN_BE_EQUAL | CAN_BE_MORE. Comparing NaN against anything would yield 0. And so on... Thomas
Index: gcc/testsuite/gfortran.dg/typebound_override_1.f90 =================================================================== --- gcc/testsuite/gfortran.dg/typebound_override_1.f90 (revision 177733) +++ gcc/testsuite/gfortran.dg/typebound_override_1.f90 (working copy) @@ -23,7 +23,7 @@ module m procedure, nopass :: b => b2 ! { dg-error "should have matching result types and ranks" } procedure, nopass :: c => c2 ! { dg-warning "Possible character length mismatch" } procedure, nopass :: d => d2 ! valid, check for commutativity (+,*) - procedure, nopass :: e => e2 ! { dg-warning "Possible character length mismatch" } + procedure, nopass :: e => e2 ! { dg-error "Character length mismatch" } end type contains Index: gcc/fortran/interface.c =================================================================== --- gcc/fortran/interface.c (revision 177733) +++ gcc/fortran/interface.c (working copy) @@ -3574,7 +3574,8 @@ gfc_check_typebound_override (gfc_symtree* proc, g switch (compval) { case -1: - case 1: + case 1: + case -3: gfc_error ("Character length mismatch between '%s' at '%L' and " "overridden FUNCTION", proc->name, &where); return FAILURE; Index: gcc/fortran/frontend-passes.c =================================================================== --- gcc/fortran/frontend-passes.c (revision 177733) +++ gcc/fortran/frontend-passes.c (working copy) @@ -682,7 +682,7 @@ optimize_comparison (gfc_expr *e, gfc_intrinsic_op && op1->ts.type != BT_COMPLEX && op2->ts.type != BT_COMPLEX)) { eq = gfc_dep_compare_expr (op1, op2); - if (eq == -2) + if (eq <= -2) { /* Replace A // B < A // C with B < C, and A // B < C // B with A < C. */ Index: gcc/fortran/dependency.c =================================================================== --- gcc/fortran/dependency.c (revision 177733) +++ gcc/fortran/dependency.c (working copy) @@ -230,8 +230,12 @@ gfc_dep_compare_functions (gfc_expr *e1, gfc_expr return -2; } -/* Compare two values. Returns 0 if e1 == e2, -1 if e1 < e2, +1 if e1 > e2, - and -2 if the relationship could not be determined. */ +/* Compare two expressions. Return values: + * +1 if e1 > e2 + * 0 if e1 == e2 + * -1 if e1 < e2 + * -2 if the relationship could not be determined + * -3 if e1 /= e2, but we cannot tell which one is larger. */ int gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) @@ -304,9 +308,9 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) r = gfc_dep_compare_expr (e1->value.op.op2, e2->value.op.op2); if (l == 0 && r == 0) return 0; - if (l == 0 && r != -2) + if (l == 0 && r > -2) return r; - if (l != -2 && r == 0) + if (l > -2 && r == 0) return l; if (l == 1 && r == 1) return 1; @@ -317,9 +321,9 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) r = gfc_dep_compare_expr (e1->value.op.op2, e2->value.op.op1); if (l == 0 && r == 0) return 0; - if (l == 0 && r != -2) + if (l == 0 && r > -2) return r; - if (l != -2 && r == 0) + if (l > -2 && r == 0) return l; if (l == 1 && r == 1) return 1; @@ -354,9 +358,9 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) r = gfc_dep_compare_expr (e1->value.op.op2, e2->value.op.op2); if (l == 0 && r == 0) return 0; - if (l != -2 && r == 0) + if (l > -2 && r == 0) return l; - if (l == 0 && r != -2) + if (l == 0 && r > -2) return -r; if (l == 1 && r == -1) return 1; @@ -375,8 +379,8 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) l = gfc_dep_compare_expr (e1->value.op.op1, e2->value.op.op1); r = gfc_dep_compare_expr (e1->value.op.op2, e2->value.op.op2); - if (l == -2) - return -2; + if (l <= -2) + return l; if (l == 0) { @@ -387,7 +391,7 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) if (e1_left->expr_type == EXPR_CONSTANT && e2_left->expr_type == EXPR_CONSTANT && e1_left->value.character.length - != e2_left->value.character.length) + != e2_left->value.character.length) return -2; else return r; @@ -411,7 +415,7 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) } if (e1->expr_type != e2->expr_type) - return -2; + return -3; switch (e1->expr_type) { @@ -434,7 +438,7 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2) if (are_identical_variables (e1, e2)) return 0; else - return -2; + return -3; case EXPR_OP: /* Intrinsic operators are the same if their operands are the same. */ @@ -1406,7 +1410,7 @@ gfc_check_element_vs_section( gfc_ref *lref, gfc_r if (!start || !end) return GFC_DEP_OVERLAP; s = gfc_dep_compare_expr (start, end); - if (s == -2) + if (s <= -2) return GFC_DEP_OVERLAP; /* Assume positive stride. */ if (s == -1) @@ -1553,7 +1557,7 @@ gfc_check_element_vs_element (gfc_ref *lref, gfc_r if (contains_forall_index_p (r_start) || contains_forall_index_p (l_start)) return GFC_DEP_OVERLAP; - if (i != -2) + if (i > -2) return GFC_DEP_NODEP; return GFC_DEP_EQUAL; }