[Bug tree-optimization/53805] New: combine_comparisons changes trapping behavior

[Bug tree-optimization/53805] New: combine_comparisons changes trapping behavior

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

             Bug #: 53805
           Summary: combine_comparisons changes trapping behavior
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: glisse@gcc.gnu.org


Hello,

int f(double a,double b){
  if(a>b) if(a<b) return 1;
  return 0;
}

gets simplified by tree-ssa-ifcombine to just return 0; which is wrong if the
comparison would trap.

This is caused by combine_comparisons:

        bool trap = (compcode & COMPCODE_UNORD) == 0
                    && (compcode != COMPCODE_EQ)
                    && (compcode != COMPCODE_ORD);

which is missing:

                    && (compcode != COMPCODE_FALSE)

(this isn't needed for ltrap and rtrap)

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-29 12:01:51 UTC ---
We do not try to preserve traps instead we only try to not produce new ones.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #2 from Marc Glisse <glisse at gcc dot gnu.org> 2012-06-29 12:19:07 UTC ---
(In reply to comment #1)
> We do not try to preserve traps instead we only try to not produce new ones.

That would make a lot of sense, and assuming it is the official policy I am
happy to learn that, but then why don't we optimize this testcase:

int f(double a,double b){
  if(a>=b) if(a<=b) return 1;
  return 0;
}

to
  if(a==b) return 1;
?

The test in combine_comparisons seems to deliberately check for any change of
the trapping condition, not just false->true.

I am happy to relabel this bug (or file a new one if you prefer) as a missed
optimization.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-29 12:22:10 UTC ---
(In reply to comment #2)
> (In reply to comment #1)
> > We do not try to preserve traps instead we only try to not produce new ones.
> 
> That would make a lot of sense, and assuming it is the official policy I am
> happy to learn that, but then why don't we optimize this testcase:
> 
> int f(double a,double b){
>   if(a>=b) if(a<=b) return 1;
>   return 0;
> }
> 
> to
>   if(a==b) return 1;
> ?

Good question.  I suppose that's a missed optimization then.

> The test in combine_comparisons seems to deliberately check for any change of
> the trapping condition, not just false->true.
> 
> I am happy to relabel this bug (or file a new one if you prefer) as a missed
> optimization.

I'd say open a new one.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #4 from joseph at codesourcery dot com <joseph at codesourcery dot com> 2012-06-29 14:24:18 UTC ---
On Fri, 29 Jun 2012, rguenth at gcc dot gnu.org wrote:

> We do not try to preserve traps instead we only try to not produce new ones.

That's a bug.  -ftrapping-math should do both (as regards the set of 
exceptions raised between function calls or asms that might test them, not 
necessarily preserving the exact positive number of times an exception is 
raised).  If that's thought to be a performance issue maybe it should be 
split into separate options for the two cases.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-29 14:34:08 UTC ---
(In reply to comment #4)
> On Fri, 29 Jun 2012, rguenth at gcc dot gnu.org wrote:
> 
> > We do not try to preserve traps instead we only try to not produce new ones.
> 
> That's a bug.  -ftrapping-math should do both (as regards the set of 
> exceptions raised between function calls or asms that might test them, not 
> necessarily preserving the exact positive number of times an exception is 
> raised).  If that's thought to be a performance issue maybe it should be 
> split into separate options for the two cases.

We happily remove dead trapping statements:

void foo(double x, int y)
{
  1 / x;
  1 / y;
}

Eric recently (for the sake of Java) added fdelete-dead-exceptions (in Ada,
with -fnon-call-exceptions, you are allowed to remove the above statements).

We similarly happily remove dead indirect loads (that might trap).

I think it is preferable that we do all this, we can't always reasonably
prove that a path in the CFG is never reached.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[joseph at codesourcery dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> 2012-06-29 14:43:30 UTC ---
On Fri, 29 Jun 2012, rguenth at gcc dot gnu.org wrote:

> We happily remove dead trapping statements:
> 
> void foo(double x, int y)
> {
>   1 / x;

Bug with -ftrapping-math.  Maybe a known bug - there are plenty of open 
bugs for ways in which -ftrapping-math -frounding-math fail to implement 
standard C semantics - but a bug.

>   1 / y;

Arguably a bug with -ftrapv (again, the option could reasonably be split 
into different cases), but not otherwise.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-29 14:54:21 UTC ---
(In reply to comment #6)
> On Fri, 29 Jun 2012, rguenth at gcc dot gnu.org wrote:
> 
> > We happily remove dead trapping statements:
> > 
> > void foo(double x, int y)
> > {
> >   1 / x;
> 
> Bug with -ftrapping-math.  Maybe a known bug - there are plenty of open 
> bugs for ways in which -ftrapping-math -frounding-math fail to implement 
> standard C semantics - but a bug.
> 
> >   1 / y;
> 
> Arguably a bug with -ftrapv (again, the option could reasonably be split 
> into different cases), but not otherwise.

All CPUs I know trap for 1 / 0, even in the integer division case.  -ftrapv
is a completely different story - for it we'd even have to preserve

void foo (int x, int y)
{
  x + y;
}

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-07-02 07:27:40 UTC ---
Integer 1 / 0 is undefined behavior, so it is fine to optimize that away.
But floating point traps with -ftrapping-math aren't undefined behavior.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #9 from Marc Glisse <glisse at gcc dot gnu.org> 2012-07-06 21:53:32 UTC ---
Suspicious code:
combine_comparisons has special code that depends on which comparison comes
first, but maybe_fold_and_comparisons calls and_comparisons_1 (and thus
combine_comparisons) with arguments in both orders. I guess this would give a
wrong optimization for ORDERED_EXPR && LT_EXPR, but I am having a hard time
creating an ORDERED_EXPR.

It looks like invert_tree_comparison shouldn't return ERROR_MARK for
(UN)ORDERED_EXPR.

[Bug tree-optimization/53805] combine_comparisons changes trapping behavior

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53805

--- Comment #10 from Marc Glisse <glisse at gcc dot gnu.org> 2012-08-02 19:54:47 UTC ---
Author: glisse
Date: Thu Aug  2 19:54:43 2012
New Revision: 190100

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190100
Log:
2012-08-02  Marc Glisse  <marc.glisse@inria.fr>

    PR tree-optimization/53805
    * gcc/fold-const.c (invert_tree_comparison): Invert ORDERED_EXPR and
    UNORDERED_EXPR even for trapping floating point.
    * gcc/testsuite/gcc.dg/fold-notunord.c: New testcase.


Added:
    trunk/gcc/testsuite/gcc.dg/fold-notunord.c   (with props)
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog

Propchange: trunk/gcc/testsuite/gcc.dg/fold-notunord.c
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/gcc.dg/fold-notunord.c
            ('svn:keywords' added)