This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch gimplifier]: Boolify more strict conditional expressions and transform simple form to binary

On Tue, May 10, 2011 at 3:56 PM, Kai Tietz <ktietz70@googlemail.com> 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.



> Regards,
> Kai
>
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]