From patchwork Tue May 10 15:08:39 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kai Tietz X-Patchwork-Id: 95240 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 0EFBAB6F6E for ; Thu, 12 May 2011 13:09:04 +1000 (EST) Received: (qmail 20837 invoked by alias); 10 May 2011 15:09:03 -0000 Received: (qmail 20811 invoked by uid 22791); 10 May 2011 15:09:00 -0000 X-SWARE-Spam-Status: No, hits=1.7 required=5.0 tests=AWL, BAYES_99, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, RFC_ABUSE_POST X-Spam-Check-By: sourceware.org Received: from mail-qw0-f47.google.com (HELO mail-qw0-f47.google.com) (209.85.216.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 May 2011 15:08:40 +0000 Received: by qwh5 with SMTP id 5so4303407qwh.20 for ; Tue, 10 May 2011 08:08:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.51.214 with SMTP id e22mr6414105qcg.156.1305040119375; Tue, 10 May 2011 08:08:39 -0700 (PDT) Received: by 10.229.33.209 with HTTP; Tue, 10 May 2011 08:08:39 -0700 (PDT) In-Reply-To: References: Date: Tue, 10 May 2011 17:08:39 +0200 Message-ID: Subject: Re: [patch gimplifier]: Boolify more strict conditional expressions and transform simple form to binary From: Kai Tietz To: Richard Guenther Cc: GCC Patches X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 2011/5/10 Richard Guenther : > On Tue, May 10, 2011 at 3:56 PM, Kai Tietz wrote: >> Hello, >> >> this patch converts TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR >> expressions >> on gimplification to their binary form.  Additionally it takes care >> that conditions >> are getting boolified for operation. >> >> ChangeLog >> >> 2011-05-10  Kai Tietz >> >>        * gimplify.c (gimplify_exit_expr): Boolify conditional >>        expression part. >>        (shortcut_cond_r): Likewise. >>        (shortcut_cond_expr): Likewise. >>        (gimplify_cond_expr): Likewise. >>        (gimplify_modify_expr_rhs): Likewise. >>        (gimplify_boolean_expr): Likewise. >>        (gimple_boolify): Boolify operands for BOOLEAN typed >>        base expressions. >>        (gimplify_expr): Boolify TRUTH_ANDIF_EXPR, TRUTH_ORIF_EXPR, >>        TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR. Additionally >>        move TRUTH_AND|OR|XOR_EXPR to its binary form. >> >> Tested for x86_64-w64-mingw32 and x86_64-pc-linux-gnu. Ok for apply? > > Comments inline (the attachment makes my comments harder to spot, > look close). > > Index: gcc/gcc/gimplify.c > =================================================================== > --- gcc.orig/gcc/gimplify.c     2011-05-10 15:44:49.000000000 +0200 > +++ gcc/gcc/gimplify.c  2011-05-10 15:46:58.365473600 +0200 > @@ -1664,10 +1664,12 @@ build_and_jump (tree *label_p) >  static enum gimplify_status >  gimplify_exit_expr (tree *expr_p) >  { > -  tree cond = TREE_OPERAND (*expr_p, 0); > -  tree expr; > +  tree cond, expr; > > +  TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0)); > +  cond = TREE_OPERAND (*expr_p, 0); > > Why do you need to boolify things at all when we build a COND_EXPR > that gets re-gimplified anyway?  I'm confused by this (similar to other > places you do that).  I would expect that you at most would do this > when gimplifying COND_EXPRs, not when you build them. > >   expr = build_and_jump (&gimplify_ctxp->exit_label); > + >   expr = build3 (COND_EXPR, void_type_node, cond, expr, NULL_TREE); >   *expr_p = expr; > > @@ -2586,6 +2588,7 @@ shortcut_cond_r (tree pred, tree *true_l >       /* Keep the original source location on the first 'if'.  Set the source >         location of the ? on the second 'if'.  */ >       new_locus = EXPR_HAS_LOCATION (pred) ? EXPR_LOCATION (pred) : locus; > +      TREE_OPERAND (pred, 0) = gimple_boolify (TREE_OPERAND (pred, 0)); >       expr = build3 (COND_EXPR, void_type_node, TREE_OPERAND (pred, 0), >                     shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, >                                      false_label_p, locus), > @@ -2594,7 +2597,7 @@ shortcut_cond_r (tree pred, tree *true_l >     } >   else >     { > -      expr = build3 (COND_EXPR, void_type_node, pred, > +      expr = build3 (COND_EXPR, void_type_node, gimple_boolify (pred), >                     build_and_jump (true_label_p), >                     build_and_jump (false_label_p)); >       SET_EXPR_LOCATION (expr, locus); > @@ -2625,7 +2628,8 @@ shortcut_cond_expr (tree expr) >   bool emit_end, emit_false, jump_over_else; >   bool then_se = then_ && TREE_SIDE_EFFECTS (then_); >   bool else_se = else_ && TREE_SIDE_EFFECTS (else_); > - > + > +  pred = TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0)); >   /* First do simple transformations.  */ >   if (!else_se) >     { > @@ -2665,7 +2669,7 @@ shortcut_cond_expr (tree expr) >            SET_EXPR_LOCATION (expr, EXPR_LOCATION (pred)); >          else_ = shortcut_cond_expr (expr); >          else_se = else_ && TREE_SIDE_EFFECTS (else_); > -         pred = TREE_OPERAND (pred, 0); > +         pred = gimple_boolify (TREE_OPERAND (pred, 0)); >          expr = build3 (COND_EXPR, void_type_node, pred, NULL_TREE, else_); >          SET_EXPR_LOCATION (expr, locus); >        } > @@ -2824,9 +2828,6 @@ gimple_boolify (tree expr) >        } >     } > > -  if (TREE_CODE (type) == BOOLEAN_TYPE) > -    return expr; > - >   switch (TREE_CODE (expr)) >     { >     case TRUTH_AND_EXPR: > @@ -2851,6 +2852,8 @@ gimple_boolify (tree expr) >     default: >       /* Other expressions that get here must have boolean values, but >         might need to be converted to the appropriate mode.  */ > +      if (TREE_CODE (type) == BOOLEAN_TYPE) > +       return expr; > > Why do you need to move this? > >       return fold_convert_loc (loc, boolean_type_node, expr); >     } >  } > @@ -2865,7 +2868,7 @@ gimplify_pure_cond_expr (tree *expr_p, g >   enum gimplify_status ret, tret; >   enum tree_code code; > > -  cond = gimple_boolify (COND_EXPR_COND (expr)); > +  cond = COND_EXPR_COND (expr) = gimple_boolify (COND_EXPR_COND (expr)); > > why change COND_EXPR_COND? > >   /* We need to handle && and || specially, as their gimplification >      creates pure cond_expr, thus leading to an infinite cycle otherwise.  */ > @@ -2937,6 +2940,7 @@ gimplify_cond_expr (tree *expr_p, gimple >   enum tree_code pred_code; >   gimple_seq seq = NULL; > > +  TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0)); > > So you are doing it here - why do it everywhere else?  I'd like to see > this change at exactly _one_ place. > >   /* If this COND_EXPR has a value, copy the values into a temporary within >      the arms.  */ >   if (!VOID_TYPE_P (type)) > @@ -4276,6 +4280,7 @@ gimplify_modify_expr_rhs (tree *expr_p, >                                            false); > >        case COND_EXPR: > + >          /* If we're assigning to a non-register type, push the assignment >             down into the branches.  This is mandatory for ADDRESSABLE types, >             since we cannot generate temporaries for such, but it saves a > @@ -4287,6 +4292,7 @@ gimplify_modify_expr_rhs (tree *expr_p, >              tree cond = *from_p; >              tree result = *to_p; > > +             TREE_OPERAND (cond, 0) = gimple_boolify (TREE_OPERAND (cond, 0)); >              ret = gimplify_expr (&result, pre_p, post_p, >                                   is_gimple_lvalue, fb_lvalue); >              if (ret != GS_ERROR) > @@ -4710,6 +4716,7 @@ gimplify_boolean_expr (tree *expr_p, loc >  { >   /* Preserve the original type of the expression.  */ >   tree type = TREE_TYPE (*expr_p); > +  *expr_p = gimple_boolify (*expr_p); > >   *expr_p = build3 (COND_EXPR, type, *expr_p, >                    fold_convert_loc (locus, type, boolean_true_node), > @@ -6762,6 +6769,13 @@ gimplify_expr (tree *expr_p, gimple_seq > >        case TRUTH_ANDIF_EXPR: >        case TRUTH_ORIF_EXPR: > +         if (TREE_CODE (TREE_TYPE (*expr_p)) != BOOLEAN_TYPE) > +           { > +             tree type = TREE_TYPE (*expr_p); > +             *expr_p = fold_convert (type, gimple_boolify (*expr_p)); > +             ret = GS_OK; > +             break; > +           } > > Huh, that doesn't make sense.  Why convert back to the original > type? > > I know COND_EXPRs don't have a boolean value type-wise, but > they do have one semantically.  It looks like you are just papering > over the inconsistencies at a lot of places instead of trying to fix them > during gimplification. > >          /* Pass the source location of the outer expression.  */ >          ret = gimplify_boolean_expr (expr_p, saved_location); >          break; > @@ -7203,6 +7217,30 @@ gimplify_expr (tree *expr_p, gimple_seq >        case TRUTH_AND_EXPR: >        case TRUTH_OR_EXPR: >        case TRUTH_XOR_EXPR: > +         if (TREE_CODE (TREE_TYPE (*expr_p)) != BOOLEAN_TYPE) > +           { > +             tree type = TREE_TYPE (*expr_p); > +             *expr_p = fold_convert (type, gimple_boolify (*expr_p)); > +             ret = GS_OK; > +             break; > +           } > +         /* Call it to make sure that operands are boolified, too. */ > +         *expr_p = gimple_boolify (*expr_p); > +         switch (TREE_CODE (*expr_p)) > +           { > +           case TRUTH_AND_EXPR: > +             TREE_SET_CODE (*expr_p, BIT_AND_EXPR); > +             break; > +           case TRUTH_OR_EXPR: > +             TREE_SET_CODE (*expr_p, BIT_IOR_EXPR); > +             break; > +           case TRUTH_XOR_EXPR: > +             TREE_SET_CODE (*expr_p, BIT_XOR_EXPR); > +             break; > +           default: > +             break; > +           } > + >          /* Classified as tcc_expression.  */ >          goto expr_2; > > Please also change verify_gimple_assign_binary to barf on > the TRUTH_* codes.  We don't want other passes re-introducing them. > > Richard. ChangeLog 2011-05-10 Kai Tietz * tree-cfg.c (verify_gimple_assign_binary): Barf on TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR. * gimplify.c (gimplify_exit_expr): Boolify conditional expression part. (shortcut_cond_r): Likewise. (shortcut_cond_expr): Likewise. (gimplify_cond_expr): Likewise. (gimplify_modify_expr_rhs): Likewise. (gimplify_boolean_expr): Likewise. (gimple_boolify): Boolify operands for BOOLEAN typed base expressions. (gimplify_expr): Boolify TRUTH_ANDIF_EXPR, TRUTH_ORIF_EXPR, TRUTH_AND_EXPR, TRUTH_OR_EXPR, and TRUTH_XOR_EXPR. Additionally move TRUTH_AND|OR|XOR_EXPR to its binary form. The issue about this gimplification is just in case of freshly generated COND_EXPR and making sure its generated conditional is boolified before creating. As late we might run exactly in those retype-cast for conditional-part being result. The real pain are the comparison expressions, which getting generated in some cases with integer types, as they are often used as an implicit type cast. The re-type type-casts I introduced after seeing exactly for ANDIF/ORIF and TRUTH AND/OR/XOR the appearance of resulting types not boolean. Regards, Kai Index: gcc/gcc/gimplify.c =================================================================== --- gcc.orig/gcc/gimplify.c 2011-05-10 15:44:49.000000000 +0200 +++ gcc/gcc/gimplify.c 2011-05-10 15:46:58.365473600 +0200 @@ -1664,10 +1664,12 @@ build_and_jump (tree *label_p) static enum gimplify_status gimplify_exit_expr (tree *expr_p) { - tree cond = TREE_OPERAND (*expr_p, 0); - tree expr; + tree cond, expr; + TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0)); + cond = TREE_OPERAND (*expr_p, 0); expr = build_and_jump (&gimplify_ctxp->exit_label); + expr = build3 (COND_EXPR, void_type_node, cond, expr, NULL_TREE); *expr_p = expr; @@ -2586,6 +2588,7 @@ shortcut_cond_r (tree pred, tree *true_l /* Keep the original source location on the first 'if'. Set the source location of the ? on the second 'if'. */ new_locus = EXPR_HAS_LOCATION (pred) ? EXPR_LOCATION (pred) : locus; + TREE_OPERAND (pred, 0) = gimple_boolify (TREE_OPERAND (pred, 0)); expr = build3 (COND_EXPR, void_type_node, TREE_OPERAND (pred, 0), shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, false_label_p, locus), @@ -2594,7 +2597,7 @@ shortcut_cond_r (tree pred, tree *true_l } else { - expr = build3 (COND_EXPR, void_type_node, pred, + expr = build3 (COND_EXPR, void_type_node, gimple_boolify (pred), build_and_jump (true_label_p), build_and_jump (false_label_p)); SET_EXPR_LOCATION (expr, locus); @@ -2625,7 +2628,8 @@ shortcut_cond_expr (tree expr) bool emit_end, emit_false, jump_over_else; bool then_se = then_ && TREE_SIDE_EFFECTS (then_); bool else_se = else_ && TREE_SIDE_EFFECTS (else_); - + + pred = TREE_OPERAND (expr, 0) = gimple_boolify (TREE_OPERAND (expr, 0)); /* First do simple transformations. */ if (!else_se) { @@ -2665,7 +2669,7 @@ shortcut_cond_expr (tree expr) SET_EXPR_LOCATION (expr, EXPR_LOCATION (pred)); else_ = shortcut_cond_expr (expr); else_se = else_ && TREE_SIDE_EFFECTS (else_); - pred = TREE_OPERAND (pred, 0); + pred = gimple_boolify (TREE_OPERAND (pred, 0)); expr = build3 (COND_EXPR, void_type_node, pred, NULL_TREE, else_); SET_EXPR_LOCATION (expr, locus); } @@ -2824,9 +2828,6 @@ gimple_boolify (tree expr) } } - if (TREE_CODE (type) == BOOLEAN_TYPE) - return expr; - switch (TREE_CODE (expr)) { case TRUTH_AND_EXPR: @@ -2851,6 +2852,8 @@ gimple_boolify (tree expr) default: /* Other expressions that get here must have boolean values, but might need to be converted to the appropriate mode. */ + if (TREE_CODE (type) == BOOLEAN_TYPE) + return expr; return fold_convert_loc (loc, boolean_type_node, expr); } } @@ -2865,7 +2868,7 @@ gimplify_pure_cond_expr (tree *expr_p, g enum gimplify_status ret, tret; enum tree_code code; - cond = gimple_boolify (COND_EXPR_COND (expr)); + cond = COND_EXPR_COND (expr) = gimple_boolify (COND_EXPR_COND (expr)); /* We need to handle && and || specially, as their gimplification creates pure cond_expr, thus leading to an infinite cycle otherwise. */ @@ -2937,6 +2940,7 @@ gimplify_cond_expr (tree *expr_p, gimple enum tree_code pred_code; gimple_seq seq = NULL; + TREE_OPERAND (*expr_p, 0) = gimple_boolify (TREE_OPERAND (*expr_p, 0)); /* If this COND_EXPR has a value, copy the values into a temporary within the arms. */ if (!VOID_TYPE_P (type)) @@ -4276,6 +4280,7 @@ gimplify_modify_expr_rhs (tree *expr_p, false); case COND_EXPR: + /* If we're assigning to a non-register type, push the assignment down into the branches. This is mandatory for ADDRESSABLE types, since we cannot generate temporaries for such, but it saves a @@ -4287,6 +4292,7 @@ gimplify_modify_expr_rhs (tree *expr_p, tree cond = *from_p; tree result = *to_p; + TREE_OPERAND (cond, 0) = gimple_boolify (TREE_OPERAND (cond, 0)); ret = gimplify_expr (&result, pre_p, post_p, is_gimple_lvalue, fb_lvalue); if (ret != GS_ERROR) @@ -4710,6 +4716,7 @@ gimplify_boolean_expr (tree *expr_p, loc { /* Preserve the original type of the expression. */ tree type = TREE_TYPE (*expr_p); + *expr_p = gimple_boolify (*expr_p); *expr_p = build3 (COND_EXPR, type, *expr_p, fold_convert_loc (locus, type, boolean_true_node), @@ -6762,6 +6769,13 @@ gimplify_expr (tree *expr_p, gimple_seq case TRUTH_ANDIF_EXPR: case TRUTH_ORIF_EXPR: + if (TREE_CODE (TREE_TYPE (*expr_p)) != BOOLEAN_TYPE) + { + tree type = TREE_TYPE (*expr_p); + *expr_p = fold_convert (type, gimple_boolify (*expr_p)); + ret = GS_OK; + break; + } /* Pass the source location of the outer expression. */ ret = gimplify_boolean_expr (expr_p, saved_location); break; @@ -7203,6 +7217,30 @@ gimplify_expr (tree *expr_p, gimple_seq case TRUTH_AND_EXPR: case TRUTH_OR_EXPR: case TRUTH_XOR_EXPR: + if (TREE_CODE (TREE_TYPE (*expr_p)) != BOOLEAN_TYPE) + { + tree type = TREE_TYPE (*expr_p); + *expr_p = fold_convert (type, gimple_boolify (*expr_p)); + ret = GS_OK; + break; + } + /* Call it to make sure that operands are boolified, too. */ + *expr_p = gimple_boolify (*expr_p); + switch (TREE_CODE (*expr_p)) + { + case TRUTH_AND_EXPR: + TREE_SET_CODE (*expr_p, BIT_AND_EXPR); + break; + case TRUTH_OR_EXPR: + TREE_SET_CODE (*expr_p, BIT_IOR_EXPR); + break; + case TRUTH_XOR_EXPR: + TREE_SET_CODE (*expr_p, BIT_XOR_EXPR); + break; + default: + break; + } + /* Classified as tcc_expression. */ goto expr_2; Index: gcc/gcc/tree-cfg.c =================================================================== --- gcc.orig/gcc/tree-cfg.c 2011-05-04 15:59:07.000000000 +0200 +++ gcc/gcc/tree-cfg.c 2011-05-10 16:57:31.628029600 +0200 @@ -3540,21 +3540,7 @@ do_pointer_plus_expr_check: case TRUTH_AND_EXPR: case TRUTH_OR_EXPR: case TRUTH_XOR_EXPR: - { - /* We allow any kind of integral typed argument and result. */ - if (!INTEGRAL_TYPE_P (rhs1_type) - || !INTEGRAL_TYPE_P (rhs2_type) - || !INTEGRAL_TYPE_P (lhs_type)) - { - error ("type mismatch in binary truth expression"); - debug_generic_expr (lhs_type); - debug_generic_expr (rhs1_type); - debug_generic_expr (rhs2_type); - return true; - } - - return false; - } + gcc_unreachable (); case LT_EXPR: case LE_EXPR: