[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at gcc dot gnu.org]

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

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenther at suse dot de
           Severity|normal                      |enhancement

[Bug tree-optimization/59660] New: We fail to optimize common boolean checks pre-inlining

[hubicka at gcc dot gnu.org]

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

            Bug ID: 59660
           Summary: We fail to optimize common boolean checks pre-inlining
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hubicka at gcc dot gnu.org

We fail to inline the following function from graphite-poly.h:
/* Determine whether CHREC is an affine evolution function or not.  */

static inline bool
evolution_function_is_affine_p (const_tree chrec)
{
  return chrec
    && TREE_CODE (chrec) == POLYNOMIAL_CHREC
    && evolution_function_is_invariant_p (CHREC_RIGHT (chrec),
                                          CHREC_VARIABLE (chrec))
    && (TREE_CODE (CHREC_RIGHT (chrec)) != POLYNOMIAL_CHREC
        || evolution_function_is_affine_p (CHREC_RIGHT (chrec)));
}


It is tail recursive, but the recursion is easy to remove and handled by late
tail recursion pass (post inlining).  Pre-inlining we stop at:
  <bb 6>:
  _15 = evolution_function_is_affine_p (_12);
  if (_15 != 0)
    goto <bb 7>;
  else
    goto <bb 8>;

  <bb 7>:

  <bb 8>:
  # iftmp.121_1 = PHI <1(7), 0(3), 0(2), 0(6), 0(4), 1(5)>
  return iftmp.121_1;


The conditional here is autogenerated by the boolean expression but is
pointless. Phiopt gets it into:
  <bb 6>:
  _15 = evolution_function_is_affine_p (_12);

  <bb 7>:
  # iftmp.121_1 = PHI <1(5), 0(3), 0(2), _15(6), 0(4)>
  return iftmp.121_1;


This seems rather common pattern suggesting that perhaps we may phiopt
pre-inline or do the same trick in one of existing early opts?

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-01-07
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I have noticed these, too (-Og is pessimzed by them).  The pattern is generated
by gimplifying.

I've pondered a bit for a good place to handle this special case (phiopt during
early isn't a good option) and the only ones I can think of is cfg-cleanup
(ugh)
and copyprop (at least looks like a copy-propagation somewhat, but that
likely runs too early).

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at ucw dot cz]

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

--- Comment #2 from Jan Hubicka <hubicka at ucw dot cz> ---
> I have noticed these, too (-Og is pessimzed by them).  The pattern is generated
> by gimplifying.

I wondered why we can't simply update gimplifier to not produce them?
(this is what I wanted to look into today, it seems pretty common pattern
confusing inliner).
For sure we should better also handle it either in cfg-cleanup or copyprop
for cases that arrise as a result of other optimizations, but it seems it
would speed things up if we did not wrap each predicate calls in its own three
BBs.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[rguenther at suse dot de]

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

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 7 Jan 2014, hubicka at ucw dot cz wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660
> 
> --- Comment #2 from Jan Hubicka <hubicka at ucw dot cz> ---
> > I have noticed these, too (-Og is pessimzed by them).  The pattern is generated
> > by gimplifying.
> 
> I wondered why we can't simply update gimplifier to not produce them?
> (this is what I wanted to look into today, it seems pretty common pattern
> confusing inliner).
> For sure we should better also handle it either in cfg-cleanup or copyprop
> for cases that arrise as a result of other optimizations, but it seems it
> would speed things up if we did not wrap each predicate calls in its own three
> BBs.

Not all testcases can be handled at gimplification time IIRC.  Which
means "testcases welcome" first, so we can look at them individually.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at ucw dot cz]

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

--- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> ---
> Not all testcases can be handled at gimplification time IIRC.  Which
> means "testcases welcome" first, so we can look at them individually.

The GCC one I saw was equivalent of:
#include <stdbool.h>
bool
m_is_less_than_n (int n, int m)
{
   return (n==m || m_is_less_than_n (n-1,m));
}

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 7 Jan 2014, hubicka at ucw dot cz wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660
> 
> --- Comment #4 from Jan Hubicka <hubicka at ucw dot cz> ---
> > Not all testcases can be handled at gimplification time IIRC.  Which
> > means "testcases welcome" first, so we can look at them individually.
> 
> The GCC one I saw was equivalent of:
> #include <stdbool.h>
> bool
> m_is_less_than_n (int n, int m)
> {
>    return (n==m || m_is_less_than_n (n-1,m));
> }

That's only optimizable after the 'mergephi' pass.  Before the
temporary setting is shared by the n==m code.  Thus maybe
'mergephi' itself can handle this ...

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at ucw dot cz]

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

--- Comment #6 from Jan Hubicka <hubicka at ucw dot cz> ---
> That's only optimizable after the 'mergephi' pass.  Before the
> temporary setting is shared by the n==m code.  Thus maybe
> 'mergephi' itself can handle this ...

Yep, mergephi seems like resonable place (at least it shares a lot of
letters in the pass name as phiopt).

I still think gimplifier itself could produce instead of:

m_is_less_than_n (int n, int m)
{
  _Bool D.2089;
  int D.2088;
  int iftmp.0;
  _Bool D.2082;

  if (n == m) goto <D.2084>; else goto <D.2087>;
  <D.2087>:
  D.2088 = n + -1;
  D.2089 = m_is_less_than_n (D.2088, m);
  if (D.2089 != 0) goto <D.2084>; else goto <D.2085>;
  <D.2084>:
  iftmp.0 = 1;
  goto <D.2086>;
  <D.2085>:
  iftmp.0 = 0;
  <D.2086>:
  D.2082 = (_Bool) iftmp.0;
  goto <D.2090>;
  <D.2090>:
  return D.2082;
}

simplified version

m_is_less_than_n (int n, int m)
{
  _Bool D.2089;
  int D.2088;
  int iftmp.0;
  _Bool D.2082;

  if (n == m) goto <D.2084>; else goto <D.2087>;
  <D.2084>
  D.2082 = 0;
  goto D.2090
  <D.2087>:
  D.2088 = n + -1;
  D.2082 = m_is_less_than_n (D.2088, m);
  goto <D.2090>;
  <D.2090>:
  return D.2082;
}

Since this is pretty common pattern it should show as compile time improvement
even at -O0.

Honza

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[rguenther at suse dot de]

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

--- Comment #7 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 8 Jan 2014, hubicka at ucw dot cz wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660
> 
> --- Comment #6 from Jan Hubicka <hubicka at ucw dot cz> ---
> > That's only optimizable after the 'mergephi' pass.  Before the
> > temporary setting is shared by the n==m code.  Thus maybe
> > 'mergephi' itself can handle this ...
> 
> Yep, mergephi seems like resonable place (at least it shares a lot of
> letters in the pass name as phiopt).
> 
> I still think gimplifier itself could produce instead of:
> 
> m_is_less_than_n (int n, int m)
> {
>   _Bool D.2089;
>   int D.2088;
>   int iftmp.0;
>   _Bool D.2082;
> 
>   if (n == m) goto <D.2084>; else goto <D.2087>;
>   <D.2087>:
>   D.2088 = n + -1;
>   D.2089 = m_is_less_than_n (D.2088, m);
>   if (D.2089 != 0) goto <D.2084>; else goto <D.2085>;
>   <D.2084>:
>   iftmp.0 = 1;
>   goto <D.2086>;
>   <D.2085>:
>   iftmp.0 = 0;
>   <D.2086>:
>   D.2082 = (_Bool) iftmp.0;
>   goto <D.2090>;
>   <D.2090>:
>   return D.2082;
> }
> 
> simplified version
> 
> m_is_less_than_n (int n, int m)
> {
>   _Bool D.2089;
>   int D.2088;
>   int iftmp.0;
>   _Bool D.2082;
> 
>   if (n == m) goto <D.2084>; else goto <D.2087>;
>   <D.2084>
>   D.2082 = 0;
>   goto D.2090
>   <D.2087>:
>   D.2088 = n + -1;
>   D.2082 = m_is_less_than_n (D.2088, m);
>   goto <D.2090>;
>   <D.2090>:
>   return D.2082;
> }
> 
> Since this is pretty common pattern it should show as compile time improvement
> even at -O0.

Or simply assign to iftmp.0.  Not sure why iftmp.0 is int
in the first place.  Probably the FEs fault.

Richard.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at ucw dot cz]

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

--- Comment #8 from Jan Hubicka <hubicka at ucw dot cz> ---
Fe produces:
 <cond_expr 0x7ffff7004840
    type <integer_type 0x7ffff6ede690 int public SI
        size <integer_cst 0x7ffff6ee03c0 constant 32>
        unit size <integer_cst 0x7ffff6ee03e0 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff6ede690 precision
32 min <integer_cst 0x7ffff6ee0360 -2147483648> max <integer_cst 0x7ffff6ee0380
2147483647>
        pointer_to_this <pointer_type 0x7ffff6eea738>>
    side-effects
    arg 0 <truth_orif_expr 0x7ffff6fe5a78
        type <boolean_type 0x7ffff6edebd0 _Bool public unsigned QI
            size <integer_cst 0x7ffff6ee0200 constant 8>
            unit size <integer_cst 0x7ffff6ee0220 constant 1>
            align 8 symtab 0 alias set -1 canonical type 0x7ffff6edebd0
precision 1 min <integer_cst 0x7ffff6ee05c0 0> max <integer_cst 0x7ffff6ee0600
1>>
        side-effects
        arg 0 <eq_expr 0x7ffff6fe5a50 type <boolean_type 0x7ffff6edebd0 _Bool>
            arg 0 <parm_decl 0x7ffff7006000 n> arg 1 <parm_decl 0x7ffff7006080
m>
            /home/jh/t.c:5:13>
        arg 1 <call_expr 0x7ffff7009000 type <boolean_type 0x7ffff6edebd0
_Bool>
            side-effects
            fn <addr_expr 0x7ffff6ff4740 type <pointer_type 0x7ffff7008000>
                constant arg 0 <function_decl 0x7ffff6fea600 m_is_less_than_n>
                /home/jh/t.c:5:37>
            arg 0 <plus_expr 0x7ffff6fe5988 type <integer_type 0x7ffff6ede690
int>
                arg 0 <parm_decl 0x7ffff7006000 n>
                arg 1 <integer_cst 0x7ffff6ee0680 constant -1>
                /home/jh/t.c:5:39> arg 1 <parm_decl 0x7ffff7006080 m>
            /home/jh/t.c:5:37>
        /home/jh/t.c:5:17>
    arg 1 <integer_cst 0x7ffff6ee0640 type <integer_type 0x7ffff6ede690 int>
constant 1>
    arg 2 <integer_cst 0x7ffff6ee0620 type <integer_type 0x7ffff6ede690 int>
constant 0>
    /home/jh/t.c:5:17>

The sequence of jumps is produced in shortcut_cond_r an it seems we do this
silly thing for every conditional
where at least one of two has side effect.
The code produces basically

if (cond1)
  goto true_label
if (cond2)
  goto true_label
else
  goto false_laebl

true_label:
 tmp=1
goto jump_around

fasle_label;
 tmp=2
jump_around:


It would make more sense to do

if (cond1)
  goto true_label
tmp = cond2;
goto jump_around
true_label
tmp = true
jump_around:

It seems it would be always a win.  I will see if my gimplifier-fu is on par to
hack this in.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at ucw dot cz]

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

--- Comment #9 from Jan Hubicka <hubicka at ucw dot cz> ---
Hi,
this patch solves the testcase.
The first hunk gets rid of the redundant NOP_EXPR converting TRUTH_EXPR from
INT to BOOL.  The second improves gimplifier to not introduce unnecesary
control flow.

If this approach seems to make sense, I will extend it to other booleans and
prepare less hackish patch.

Honza

Index: fold-const.c
===================================================================
--- fold-const.c    (revision 206346)
+++ fold-const.c    (working copy)
@@ -1977,6 +1977,8 @@ fold_convert_loc (location_t loc, tree t

     case INTEGER_TYPE: case ENUMERAL_TYPE: case BOOLEAN_TYPE:
     case OFFSET_TYPE:
+      if (TREE_CODE (arg) == TRUTH_ORIF_EXPR)
+    return fold_build2_loc (loc, TRUTH_ORIF_EXPR, type, TREE_OPERAND (arg, 0),
TREE_OPERAND (arg, 1));
       if (TREE_CODE (arg) == INTEGER_CST)
     {
       tem = fold_convert_const (NOP_EXPR, type, arg);
Index: gimplify.c
===================================================================
--- gimplify.c    (revision 206346)
+++ gimplify.c    (working copy)
@@ -2931,14 +2931,37 @@ gimplify_cond_expr (tree *expr_p, gimple
       result = build_simple_mem_ref_loc (loc, tmp);
     }

-      /* Build the new then clause, `tmp = then_;'.  But don't build the
-     assignment if the value is void; in C++ it can be if it's a throw.  */
-      if (!VOID_TYPE_P (TREE_TYPE (then_)))
-    TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp, then_);
+      /* Convert (A||B) ? true : false
+     as A ? tmp = true : tmp = B != 0.  */

-      /* Similarly, build the new else clause, `tmp = else_;'.  */
-      if (!VOID_TYPE_P (TREE_TYPE (else_)))
-    TREE_OPERAND (expr, 2) = build2 (MODIFY_EXPR, type, tmp, else_);
+      if (integer_zerop (else_) && integer_onep (then_)
+      && TREE_CODE (TREE_OPERAND (expr, 0)) == TRUTH_ORIF_EXPR)
+    {
+      tree pred = TREE_OPERAND (expr, 0);
+      location_t loc = EXPR_LOCATION (*expr_p);
+      TREE_OPERAND (expr, 0) = TREE_OPERAND (pred, 0);
+      /* Build the new then clause, `tmp = then_;'.  But don't build the
+         assignment if the value is void; in C++ it can be if it's a throw. 
*/
+      TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp,
+                       fold_convert (type, boolean_true_node));
+
+      /* Similarly, build the new else clause, `tmp = else_;'.  */
+      TREE_OPERAND (expr, 2) = build2 (MODIFY_EXPR, type, tmp,
+                       fold_build2_loc (loc, NE_EXPR, type, TREE_OPERAND
(pred, 1),
+                                fold_convert (TREE_TYPE (TREE_OPERAND (pred,
1)),
+                                      integer_zero_node)));
+    }
+      else
+    {
+      /* Build the new then clause, `tmp = then_;'.  But don't build the
+         assignment if the value is void; in C++ it can be if it's a throw. 
*/
+      if (!VOID_TYPE_P (TREE_TYPE (then_)))
+        TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp, then_);
+
+      /* Similarly, build the new else clause, `tmp = else_;'.  */
+      if (!VOID_TYPE_P (TREE_TYPE (else_)))
+        TREE_OPERAND (expr, 2) = build2 (MODIFY_EXPR, type, tmp, else_);
+    }

       TREE_TYPE (expr) = void_type_node;
       recalculate_side_effects (expr);

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[rguenth at gcc dot gnu.org]

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

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #9)
> Hi,
> this patch solves the testcase.
> The first hunk gets rid of the redundant NOP_EXPR converting TRUTH_EXPR from
> INT to BOOL.  The second improves gimplifier to not introduce unnecesary
> control flow.
> 
> If this approach seems to make sense, I will extend it to other booleans and
> prepare less hackish patch.
> 
> Honza
> 
> Index: fold-const.c
> ===================================================================
> --- fold-const.c	(revision 206346)
> +++ fold-const.c	(working copy)
> @@ -1977,6 +1977,8 @@ fold_convert_loc (location_t loc, tree t
>  
>      case INTEGER_TYPE: case ENUMERAL_TYPE: case BOOLEAN_TYPE:
>      case OFFSET_TYPE:
> +      if (TREE_CODE (arg) == TRUTH_ORIF_EXPR)
> +	return fold_build2_loc (loc, TRUTH_ORIF_EXPR, type, TREE_OPERAND (arg, 0),
> TREE_OPERAND (arg, 1));
>        if (TREE_CODE (arg) == INTEGER_CST)
>  	{
>  	  tem = fold_convert_const (NOP_EXPR, type, arg);

This looks bogus.  See where we do this folding and instead handle it
there (I suspect gimple_boolify?)

> Index: gimplify.c
> ===================================================================
> --- gimplify.c	(revision 206346)
> +++ gimplify.c	(working copy)
> @@ -2931,14 +2931,37 @@ gimplify_cond_expr (tree *expr_p, gimple
>  	  result = build_simple_mem_ref_loc (loc, tmp);
>  	}
>  
> -      /* Build the new then clause, `tmp = then_;'.  But don't build the
> -	 assignment if the value is void; in C++ it can be if it's a throw.  */
> -      if (!VOID_TYPE_P (TREE_TYPE (then_)))
> -	TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp, then_);
> +      /* Convert (A||B) ? true : false
> +	 as A ? tmp = true : tmp = B != 0.  */

So does it end up working for if (a || b || c)?  Otherwise this looks
sensible to me.  (A&&B) ? true : false -> A ? tmp = B != 0 : tmp = false
then?

> -      /* Similarly, build the new else clause, `tmp = else_;'.  */
> -      if (!VOID_TYPE_P (TREE_TYPE (else_)))
> -	TREE_OPERAND (expr, 2) = build2 (MODIFY_EXPR, type, tmp, else_);
> +      if (integer_zerop (else_) && integer_onep (then_)
> +	  && TREE_CODE (TREE_OPERAND (expr, 0)) == TRUTH_ORIF_EXPR)
> +	{
> +	  tree pred = TREE_OPERAND (expr, 0);
> +	  location_t loc = EXPR_LOCATION (*expr_p);
> +	  TREE_OPERAND (expr, 0) = TREE_OPERAND (pred, 0);
> +	  /* Build the new then clause, `tmp = then_;'.  But don't build the
> +	     assignment if the value is void; in C++ it can be if it's a throw.  */
> +	  TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp,
> +					   fold_convert (type, boolean_true_node));
> +
> +	  /* Similarly, build the new else clause, `tmp = else_;'.  */
> +	  TREE_OPERAND (expr, 2) = build2 (MODIFY_EXPR, type, tmp,
> +					   fold_build2_loc (loc, NE_EXPR, type, TREE_OPERAND (pred, 1),
> +							    fold_convert (TREE_TYPE (TREE_OPERAND (pred, 1)),
> +									  integer_zero_node)));
> +	}
> +      else
> +	{
> +	  /* Build the new then clause, `tmp = then_;'.  But don't build the
> +	     assignment if the value is void; in C++ it can be if it's a throw.  */
> +	  if (!VOID_TYPE_P (TREE_TYPE (then_)))
> +	    TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp, then_);
> +
> +	  /* Similarly, build the new else clause, `tmp = else_;'.  */
> +	  if (!VOID_TYPE_P (TREE_TYPE (else_)))
> +	    TREE_OPERAND (expr, 2) = build2 (MODIFY_EXPR, type, tmp, else_);
> +	}
>  
>        TREE_TYPE (expr) = void_type_node;
>        recalculate_side_effects (expr);
>From gcc-bugs-return-439736-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Jan 09 09:54:42 2014
Return-Path: <gcc-bugs-return-439736-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 7732 invoked by alias); 9 Jan 2014 09:54:42 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 7696 invoked by uid 48); 9 Jan 2014 09:54:38 -0000
From: "rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug target/59725] [4.9 Regression] ARM,AArch64 r206148 (PR tree-optimization/59544) caused regressions in pr52943
Date: Thu, 09 Jan 2014 09:54:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: target
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: rguenth at gcc dot gnu.org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 4.9.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: target_milestone short_desc
Message-ID: <bug-59725-4-GUXv03IYnZ@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-59725-4@http.gcc.gnu.org/bugzilla/>
References: <bug-59725-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-01/txt/msg00878.txt.bz2
Content-length: 634

http://gcc.gnu.org/bugzilla/show_bug.cgi?idY725

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.9.0
            Summary|[4.9 Regression             |[4.9 Regression]
                   |ARM,AArch64] r206148 (PR    |ARM,AArch64 r206148 (PR
                   |tree-optimization/59544)    |tree-optimization/59544)
                   |caused regressions in       |caused regressions in
                   |pr52943                     |pr52943

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[hubicka at ucw dot cz]

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

--- Comment #11 from Jan Hubicka <hubicka at ucw dot cz> ---
> >      case INTEGER_TYPE: case ENUMERAL_TYPE: case BOOLEAN_TYPE:
> >      case OFFSET_TYPE:
> > +      if (TREE_CODE (arg) == TRUTH_ORIF_EXPR)
> > +	return fold_build2_loc (loc, TRUTH_ORIF_EXPR, type, TREE_OPERAND (arg, 0),
> > TREE_OPERAND (arg, 1));
> >        if (TREE_CODE (arg) == INTEGER_CST)
> >  	{
> >  	  tem = fold_convert_const (NOP_EXPR, type, arg);
> 
> This looks bogus.  See where we do this folding and instead handle it
> there (I suspect gimple_boolify?)

I believe this transform apply for generic/FE trees, too. But indeed I also
think
it is not right spot, I just failed to quickly identify better.
I will check gimple_boolify.

> > -      /* Build the new then clause, `tmp = then_;'.  But don't build the
> > -	 assignment if the value is void; in C++ it can be if it's a throw.  */
> > -      if (!VOID_TYPE_P (TREE_TYPE (then_)))
> > -	TREE_OPERAND (expr, 1) = build2 (MODIFY_EXPR, type, tmp, then_);
> > +      /* Convert (A||B) ? true : false
> > +	 as A ? tmp = true : tmp = B != 0.  */
> 
> So does it end up working for if (a || b || c)?  Otherwise this looks
> sensible to me.  (A&&B) ? true : false -> A ? tmp = B != 0 : tmp = false
> then?

a||b||c results in sane code (I already tested it).  We need to also handle &&.
I implemented it and for some reason I end up producing more statements for
combine.c, so I think I may inhibit some other optimization - the later
functions
play with side effects.  Will dig into this further.

Honza
>From gcc-bugs-return-439760-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Jan 09 13:37:52 2014
Return-Path: <gcc-bugs-return-439760-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 5611 invoked by alias); 9 Jan 2014 13:37:51 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 5552 invoked by uid 55); 9 Jan 2014 13:37:46 -0000
From: "bviyer at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug c++/59631] ICE using _Cilk_spawn without -fcilkplus
Date: Thu, 09 Jan 2014 13:37:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: c++
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords: ice-on-valid-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: bviyer at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 4.9.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-59631-4-OWMOmwM3xI@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-59631-4@http.gcc.gnu.org/bugzilla/>
References: <bug-59631-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-01/txt/msg00902.txt.bz2
Content-length: 2288

http://gcc.gnu.org/bugzilla/show_bug.cgi?idY631

--- Comment #4 from bviyer at gcc dot gnu.org ---
Author: bviyer
Date: Thu Jan  9 13:37:41 2014
New Revision: 206463

URL: http://gcc.gnu.org/viewcvs?rev 6463&root=gcc&view=rev
Log:
Fix for PR c++/59631.
+++ gcc/cp/ChangeLog
+2014-01-09  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+       PR c++/59631
+       * parser.c (cp_parser_postfix_expression): Added a new if-statement
+       and replaced an existing if-statement with else-if statement.
+       Changed an existing error message wording to match the one from the C
+       parser.
+

+++ gcc/testsuite/ChangeLog
+2014-01-09  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+       PR c++/59631
+       * gcc.dg/cilk-plus/cilk-plus.exp: Removed "-fcilkplus" from flags list.
+       * g++.dg/cilk-plus/cilk-plus.exp: Likewise.
+       * c-c++-common/cilk-plus/CK/spawnee_inline.c: Replaced second dg-option
+       with dg-additional-options.
+       * c-c++-common/cilk-plus/CK/varargs_test.c: Likewise.
+       * c-c++-common/cilk-plus/CK/steal_check.c: Likewise.
+       * c-c++-common/cilk-plus/CK/spawner_inline.c: Likewise.
+       * c-c++-common/cilk-plus/CK/spawning_arg.c: Likewise.
+       * c-c++-common/cilk-plus/CK/invalid_spawns.c: Added a dg-options tag.
+       * c-c++-common/cilk-plus/CK/pr59631.c: New testcase.
+

gcc/c/ChangeLog
+2014-01-09  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+       PR c++/59631
+       * c-parser.c (c_parser_postfix_expression): Replaced consecutive if
+       statements with if-elseif statements.
+


Added:
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/pr59631.c
Modified:
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-parser.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/parser.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_spawns.c
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/spawner_inline.c
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/steal_check.c
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/varargs_test.c
    trunk/gcc/testsuite/g++.dg/cilk-plus/cilk-plus.exp
    trunk/gcc/testsuite/gcc.dg/cilk-plus/cilk-plus.exp

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
   Last reconfirmed|2014-01-07 00:00:00         |2021-6-1

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #4)
> > Not all testcases can be handled at gimplification time IIRC.  Which
> > means "testcases welcome" first, so we can look at them individually.
> 
> The GCC one I saw was equivalent of:
> #include <stdbool.h>
> bool
> m_is_less_than_n (int n, int m)
> {
>    return (n==m || m_is_less_than_n (n-1,m));
> }

First we cannot optimize this into a loop until tailc rather than in either
tailr1 or tailr2.

In tailr1, we have:
  _2 = m_is_less_than_n (_1, m_7(D));
  if (_2 != 0)
    goto <bb 6>; [INV]
  else
    goto <bb 5>; [INV]

  <bb 6> :

  <bb 5> :
  # iftmp.0_3 = PHI <1(6), 0(3), 1(2)>
  _12 = (_Bool) iftmp.0_3;


While in tailr2, we have:
  _18 = m_is_less_than_n (_17, m_6(D));
  _4 = (int) _18;

  <bb 7> [local count: 1073741825]:
  # iftmp.0_3 = PHI <_4(6), 1(3), 1(2), 1(4), 1(5)>
  _8 = (_Bool) iftmp.0_3;

It is not until PRE where we remove the casts (to/from int).

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This has been fixed at r12-2039-gcd48e550d1dc583, if we use the C++ front-end. 

Using the C front-end we still have a casting issue.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |pinskia at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #14 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Mine, working on improving phi-opt here.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660

--- Comment #15 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #13)
> Using the C front-end we still have a casting issue.

There are a few issues of even for the C++ front-end IR:
In phiopt1 we have:
  if (n_6(D) == m_7(D))
    goto <bb 4>; [INV]
  else
    goto <bb 3>; [INV]

  <bb 3> :
  _1 = n_6(D) + -1;
  # .MEM_9 = VDEF <.MEM_8(D)>
  _10 = m_is_less_than_n (_1, m_7(D));
  if (_10 != 0)
    goto <bb 4>; [INV]
  else
    goto <bb 5>; [INV]

  <bb 4> :
  # .MEM_4 = PHI <.MEM_9(3), .MEM_8(D)(2)>

  <bb 5> :
  # iftmp.0_3 = PHI <1(4), 0(3)>
  # .MEM_5 = PHI <.MEM_4(4), .MEM_9(3)>

the conditional in "bb 3" could be replaced with tmp = _10.

I will have to think of how to handle this in the current code but it is
definitely more complex really because you have 2 phi nodes to worry about and
such.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660

--- Comment #16 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #15)
> (In reply to Andrew Pinski from comment #13)
> > Using the C front-end we still have a casting issue.
> 
> There are a few issues of even for the C++ front-end IR:
> I will have to think of how to handle this in the current code but it is
> definitely more complex really because you have 2 phi nodes to worry about
> and such.

So I accidently stumbled on why the C++ IR is this way.
make_forwarders_with_degenerate_phis for cd_dce undoes what mergephi did (which
is was before dse). I can fix this by moving the mergephi right after cd_dce. I
am still deciding if this is the best thing.

[Bug tree-optimization/59660] We fail to optimize common boolean checks pre-inlining

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59660

--- Comment #17 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #12)
> (In reply to Jan Hubicka from comment #4)
> > > Not all testcases can be handled at gimplification time IIRC.  Which
> > > means "testcases welcome" first, so we can look at them individually.
> > 
> > The GCC one I saw was equivalent of:
> > #include <stdbool.h>
> > bool
> > m_is_less_than_n (int n, int m)
> > {
> >    return (n==m || m_is_less_than_n (n-1,m));
> > }
> 
> First we cannot optimize this into a loop until tailc rather than in either
> tailr1 or tailr2.
> 
> In tailr1, we have:
>   _2 = m_is_less_than_n (_1, m_7(D));
>   if (_2 != 0)
>     goto <bb 6>; [INV]
>   else
>     goto <bb 5>; [INV]
> 
>   <bb 6> :
> 
>   <bb 5> :
>   # iftmp.0_3 = PHI <1(6), 0(3), 1(2)>
>   _12 = (_Bool) iftmp.0_3;
> 
> 
> While in tailr2, we have:
>   _18 = m_is_less_than_n (_17, m_6(D));
>   _4 = (int) _18;
> 
>   <bb 7> [local count: 1073741825]:
>   # iftmp.0_3 = PHI <_4(6), 1(3), 1(2), 1(4), 1(5)>
>   _8 = (_Bool) iftmp.0_3;
> 
> It is not until PRE where we remove the casts (to/from int).

I have a phiopt change which allows us now to optimize this in phiopt2 (and get
the tail call in tailr2).