diff mbox

Fix inconsistency in invert_tree_comparison

Message ID 201110231056.34553.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou Oct. 23, 2011, 8:56 a.m. UTC 
Hi,

the comment of the function reads:

/* Given a tree comparison code, return the code that is the logical inverse
   of the given code.  It is not safe to do this for floating-point
   comparisons, except for NE_EXPR and EQ_EXPR, so we receive a machine mode
   as well: if reversing the comparison is unsafe, return ERROR_MARK.  */

but the function starts with:

  if (honor_nans && flag_trapping_math)
    return ERROR_MARK;

so, for example, it refuses to fold !(x == y) to x != y for FP, which is valid.

Fixed by letting EQ_EXPR and NE_EXPR go through.  This makes tree-opt/44683 
regress though, but it's clear that the original fix only papered over the 
problem, as you can't infer a simple equivalence from a condition when you can 
have signed zeros around; so the patch also includes the proper fix.

Tested on x86_64-suse-linux, OK for mainline?


2011-10-23  Eric Botcazou  <ebotcazou@adacore.com>

	* fold-const.c (invert_tree_comparison): Always invert EQ_EXPR/NE_EXPR.

	PR tree-optimization/44683
	* tree-ssa-dom.c (record_edge_info): Record simple equivalences only if
	we can be sure that there are no signed zeros involved.

Comments

Richard Biener Oct. 23, 2011, 9:28 a.m. UTC | #1 
On Sun, Oct 23, 2011 at 10:56 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> the comment of the function reads:
>
> /* Given a tree comparison code, return the code that is the logical inverse
>   of the given code.  It is not safe to do this for floating-point
>   comparisons, except for NE_EXPR and EQ_EXPR, so we receive a machine mode
>   as well: if reversing the comparison is unsafe, return ERROR_MARK.  */
>
> but the function starts with:
>
>  if (honor_nans && flag_trapping_math)
>    return ERROR_MARK;

Do you have an idea why we test flag_trapping_math here?

> so, for example, it refuses to fold !(x == y) to x != y for FP, which is valid.
>
> Fixed by letting EQ_EXPR and NE_EXPR go through.  This makes tree-opt/44683
> regress though, but it's clear that the original fix only papered over the
> problem, as you can't infer a simple equivalence from a condition when you can
> have signed zeros around; so the patch also includes the proper fix.
>
> Tested on x86_64-suse-linux, OK for mainline?

Ok.

Thanks,
Richard.

>
> 2011-10-23  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * fold-const.c (invert_tree_comparison): Always invert EQ_EXPR/NE_EXPR.
>
>        PR tree-optimization/44683
>        * tree-ssa-dom.c (record_edge_info): Record simple equivalences only if
>        we can be sure that there are no signed zeros involved.
>
>
> --
> Eric Botcazou
>
Eric Botcazou Oct. 23, 2011, 10:06 a.m. UTC | #2 
> Do you have an idea why we test flag_trapping_math here?

Not really, the test was added with the contradictory comment:
  http://gcc.gnu.org/ml/gcc-patches/2004-05/msg01674.html
diff mbox

Patch

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 180235)
+++ fold-const.c	(working copy)
@@ -2100,15 +2100,14 @@  pedantic_non_lvalue_loc (location_t loc,
   return protected_set_expr_location_unshare (x, loc);
 }
 
-/* Given a tree comparison code, return the code that is the logical inverse
-   of the given code.  It is not safe to do this for floating-point
-   comparisons, except for NE_EXPR and EQ_EXPR, so we receive a machine mode
-   as well: if reversing the comparison is unsafe, return ERROR_MARK.  */
+/* Given a tree comparison code, return the code that is the logical inverse.
+   It is generally not safe to do this for floating-point comparisons, except
+   for EQ_EXPR and NE_EXPR, so we return ERROR_MARK in this case.  */
 
 enum tree_code
 invert_tree_comparison (enum tree_code code, bool honor_nans)
 {
-  if (honor_nans && flag_trapping_math)
+  if (honor_nans && flag_trapping_math && code != EQ_EXPR && code != NE_EXPR)
     return ERROR_MARK;
 
   switch (code)
Index: tree-ssa-dom.c
===================================================================
--- tree-ssa-dom.c	(revision 180235)
+++ tree-ssa-dom.c	(working copy)
@@ -1610,12 +1610,15 @@  record_edge_info (basic_block bb)
             {
               tree cond = build2 (code, boolean_type_node, op0, op1);
               tree inverted = invert_truthvalue_loc (loc, cond);
+              enum machine_mode mode = TYPE_MODE (TREE_TYPE (op0));
+              bool can_infer_simple_equiv
+                = !(HONOR_SIGNED_ZEROS (mode) && real_zerop (op0));
               struct edge_info *edge_info;
 
               edge_info = allocate_edge_info (true_edge);
               record_conditions (edge_info, cond, inverted);
 
-              if (code == EQ_EXPR)
+              if (can_infer_simple_equiv && code == EQ_EXPR)
                 {
                   edge_info->lhs = op1;
                   edge_info->rhs = op0;
@@ -1624,7 +1627,7 @@  record_edge_info (basic_block bb)
               edge_info = allocate_edge_info (false_edge);
               record_conditions (edge_info, inverted, cond);
 
-              if (TREE_CODE (inverted) == EQ_EXPR)
+              if (can_infer_simple_equiv && TREE_CODE (inverted) == EQ_EXPR)
                 {
                   edge_info->lhs = op1;
                   edge_info->rhs = op0;
@@ -1632,17 +1635,21 @@  record_edge_info (basic_block bb)
             }
 
           else if (TREE_CODE (op0) == SSA_NAME
-                   && (is_gimple_min_invariant (op1)
-                       || TREE_CODE (op1) == SSA_NAME))
+                   && (TREE_CODE (op1) == SSA_NAME
+                       || is_gimple_min_invariant (op1)))
             {
               tree cond = build2 (code, boolean_type_node, op0, op1);
               tree inverted = invert_truthvalue_loc (loc, cond);
+              enum machine_mode mode = TYPE_MODE (TREE_TYPE (op1));
+              bool can_infer_simple_equiv
+                = !(HONOR_SIGNED_ZEROS (mode)
+                    && (TREE_CODE (op1) == SSA_NAME || real_zerop (op1)));
               struct edge_info *edge_info;
 
               edge_info = allocate_edge_info (true_edge);
               record_conditions (edge_info, cond, inverted);
 
-              if (code == EQ_EXPR)
+              if (can_infer_simple_equiv && code == EQ_EXPR)
                 {
                   edge_info->lhs = op0;
                   edge_info->rhs = op1;
@@ -1651,7 +1658,7 @@  record_edge_info (basic_block bb)
               edge_info = allocate_edge_info (false_edge);
               record_conditions (edge_info, inverted, cond);
 
-              if (TREE_CODE (inverted) == EQ_EXPR)
+              if (can_infer_simple_equiv && TREE_CODE (inverted) == EQ_EXPR)
                 {
                   edge_info->lhs = op0;
                   edge_info->rhs = op1;