| Message ID | AM6PR10MB2023248AD9AA865730010ECCE4989@AM6PR10MB2023.EURPRD10.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | ssa-ifcombine: combining comparisons [Bug 53806] | expand |
On Thu, Mar 4, 2021 at 1:23 AM Ivan Sučić via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi, > The patch solves the bug when in ifcombine are comparisons like <= and >= combined. I removed the trap check for last statement and that doesn't break the solution for bug 61383. In fold-const.c i removed the trap checks becase combining comparisons doesn't introduce new traps. I tested my patch on wsl2 debain and it doesn't have more failed tests then without my patch. Maybe the only problem with testing is that i get error tcl error sourcing gcc/testsuite/gcc.misc-tests/linkage.exp. That error I get without my patch, too. I hope you will accept my patch. diff --git a/ChangeLog b/ChangeLog index 6c07275d2b4..29176bf9607 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2021-03-03 Ivan Sucic <sucicf1@outlook.com> newline missing + PR 53806 should be <tab>PR tree-optimization/53806 + * tree-ssa-ifcombine.c: Checking trap not for + last statement should be <tab>* tree-ssa-ifcombine.c (bb_no_side_effects_p): Checking trap <tab>not for last statement. + * fold-const.c: The comparison combine makes + new comparison correct and the trap check is + not required. + * ssa-ifcombine-14.c: Testcase for if combine + comparisons of double type. we are no longer editing ChangeLog files manually, instead the changelog entry is now embedded within the git commit message and automatically transfered. So please for further submissions use git format-patch which will include the commit message as well. diff --git a/gcc/tree-ssa-ifcombine.c b/gcc/tree-ssa-ifcombine.c index 21a70f4386d..f2abaf18342 100644 --- a/gcc/tree-ssa-ifcombine.c +++ b/gcc/tree-ssa-ifcombine.c @@ -125,7 +125,8 @@ bb_no_side_effects_p (basic_block bb) if (gimple_has_side_effects (stmt) || gimple_uses_undefined_value_p (stmt) - || gimple_could_trap_p (stmt) + /* Last statement is taken care by combining ifs */ + || (!gsi_one_before_end_p (gsi) && gimple_could_trap_p (stmt)) || gimple_vuse (stmt) /* const calls don't match any of the above, yet they could still have some side-effects - they could contain might be easier to start the loop from the end of the BB and skip a gimple-cond if it appears there. diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 3744e4c2c2d..a83c98014fb 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -2860,40 +2860,6 @@ combine_comparisons (location_t loc, else if (compcode == COMPCODE_ORD) compcode = COMPCODE_TRUE; } - else if (flag_trapping_math) - { - /* Check that the original operation and the optimized ones will trap - under the same condition. */ - bool ltrap = (lcompcode & COMPCODE_UNORD) == 0 ... - /* If the comparison was short-circuited, and only the RHS - trapped, we may now generate a spurious trap. */ - if (rtrap && !ltrap - && (code == TRUTH_ANDIF_EXPR || code == TRUTH_ORIF_EXPR)) - return NULL_TREE; - - /* If we changed the conditions that cause a trap, we lose. */ - if ((ltrap || rtrap) != trap) - return NULL_TREE; - } if (compcode == COMPCODE_TRUE) return constant_boolean_node (true, truth_type); I don't think removing all code is OK since it would allow combining x == y || x > y to x >= y which may trap. The bugreport suggests to alter the last test to something like if (trap && !ltrap && !rtrap) return NULL_TREE; thus if we manage to remove a trap (!trap) the transform is OK. Thanks, Richard.
Richard Biener wrote: >I don't think removing all code is OK since it would >allow combining x == y || x > y to x >= y which may >trap. The bugreport suggests to alter the last >test to something like > > if (trap && !ltrap && !rtrap) > return NULL_TREE; > >thus if we manage to remove a trap (!trap) the transform >is OK. Hi, Could you please explain this: combining x == y || x > y to x >= y which may trap. If x is a nan then x == y II x > y are false. The expr x >= y is false, too. Where is the error?
On Tue, Mar 9, 2021 at 9:57 PM Ivan Sučić <sucicf1@outlook.com> wrote: > > Richard Biener wrote: > >I don't think removing all code is OK since it would > >allow combining x == y || x > y to x >= y which may > >trap. The bugreport suggests to alter the last > >test to something like > > > > if (trap && !ltrap && !rtrap) > > return NULL_TREE; > > > >thus if we manage to remove a trap (!trap) the transform > >is OK. > Hi, > Could you please explain this: > combining x == y || x > y to x >= y which may trap. > If x is a nan then x == y II x > y are false. The expr x >= y is false, too. Where is the error? Maybe my example was not good but neither the bugreport nor your submission explains that eliding the short-cutting is always OK. And I'm not too familiar with all the specialities of FP compares. I've reviewed as to what the bugreport suggests (allow traps to go away), but the patch does seemingly more. CCing Roger so he might explain. Richard.
diff --git a/ChangeLog b/ChangeLog index 6c07275d2b4..29176bf9607 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2021-03-03 Ivan Sucic <sucicf1@outlook.com> + PR 53806 + * tree-ssa-ifcombine.c: Checking trap not for + last statment + * fold-const.c: The comparison combine makes + new comparison correct and the trap check is + not required. + * ssa-ifcombine-14.c: Testcase for if combine + comparisons of double type. + 2020-07-23 Release Manager * GCC 10.2.0 released. diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 3744e4c2c2d..a83c98014fb 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -2860,40 +2860,6 @@ combine_comparisons (location_t loc, else if (compcode == COMPCODE_ORD) compcode = COMPCODE_TRUE; } - else if (flag_trapping_math) - { - /* Check that the original operation and the optimized ones will trap - under the same condition. */ - bool ltrap = (lcompcode & COMPCODE_UNORD) == 0 - && (lcompcode != COMPCODE_EQ) - && (lcompcode != COMPCODE_ORD); - bool rtrap = (rcompcode & COMPCODE_UNORD) == 0 - && (rcompcode != COMPCODE_EQ) - && (rcompcode != COMPCODE_ORD); - bool trap = (compcode & COMPCODE_UNORD) == 0 - && (compcode != COMPCODE_EQ) - && (compcode != COMPCODE_ORD); - - /* In a short-circuited boolean expression the LHS might be - such that the RHS, if evaluated, will never trap. For - example, in ORD (x, y) && (x < y), we evaluate the RHS only - if neither x nor y is NaN. (This is a mixed blessing: for - example, the expression above will never trap, hence - optimizing it to x < y would be invalid). */ - if ((code == TRUTH_ORIF_EXPR && (lcompcode & COMPCODE_UNORD)) - || (code == TRUTH_ANDIF_EXPR && !(lcompcode & COMPCODE_UNORD))) - rtrap = false; - - /* If the comparison was short-circuited, and only the RHS - trapped, we may now generate a spurious trap. */ - if (rtrap && !ltrap - && (code == TRUTH_ANDIF_EXPR || code == TRUTH_ORIF_EXPR)) - return NULL_TREE; - - /* If we changed the conditions that cause a trap, we lose. */ - if ((ltrap || rtrap) != trap) - return NULL_TREE; - } if (compcode == COMPCODE_TRUE) return constant_boolean_node (true, truth_type); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-ifcombine-14.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-ifcombine-14.c new file mode 100644 index 00000000000..f94bc1f2cbb --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-ifcombine-14.c @@ -0,0 +1,13 @@ +/* { do-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized" } */ + +int +f (double a,double b) +{ + if (a>=b) if (a<=b) return 1; + return 0; +} + +/* { dg-final { scan-tree-dump-times "==" 1 "optimized" } } */ +/* { dg-final { scan-tree-dump-not "<=" "optimized" } } */ +/* { dg-final { scan-tree-dump-not ">=" "optimized" } } */ diff --git a/gcc/tree-ssa-ifcombine.c b/gcc/tree-ssa-ifcombine.c index 21a70f4386d..f2abaf18342 100644 --- a/gcc/tree-ssa-ifcombine.c +++ b/gcc/tree-ssa-ifcombine.c @@ -125,7 +125,8 @@ bb_no_side_effects_p (basic_block bb) if (gimple_has_side_effects (stmt) || gimple_uses_undefined_value_p (stmt) - || gimple_could_trap_p (stmt) + /* Last statement is taken care by combining ifs */ + || (!gsi_one_before_end_p (gsi) && gimple_could_trap_p (stmt)) || gimple_vuse (stmt) /* const calls don't match any of the above, yet they could still have some side-effects - they could contain