Patchwork [2/4] Outline fold_or_predicates from add_to_predicate_list.

* tree-if-conv.c (fold_or_predicates): New.
	(add_to_predicate_list): Call it.
---
 gcc/tree-if-conv.c |   43 ++++++++++++++++++++++---------------------
 1 files changed, 22 insertions(+), 21 deletions(-)

On Thu, 8 Jul 2010, Sebastian Pop wrote:

> 	* tree-if-conv.c (fold_or_predicates): New.
> 	(add_to_predicate_list): Call it.

I requested this change before the previous patch was checked in.
Please do not ignore reviews this way.  Thanks.

> ---
>  gcc/tree-if-conv.c |   43 ++++++++++++++++++++++---------------------
>  1 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
> index 34b4159..cac5a3b 100644
> --- a/gcc/tree-if-conv.c
> +++ b/gcc/tree-if-conv.c
> @@ -300,6 +300,27 @@ parse_predicate (tree cond, tree *op0, tree *op1)
>    return ERROR_MARK;
>  }
>  
> +/* Returns the fold of predicate C1 OR C2.  */
> +
> +static tree
> +fold_or_predicates (tree c1, tree c2)
> +{
> +  tree op1a, op1b, op2a, op2b;
> +  enum tree_code code1 = parse_predicate (c1, &op1a, &op1b);
> +  enum tree_code code2 = parse_predicate (c2, &op2a, &op2b);
> +
> +  if (code1 != ERROR_MARK && code2 != ERROR_MARK)
> +    {
> +      tree t = maybe_fold_or_comparisons (code1, op1a, op1b,
> +					  code2, op2a, op2b);
> +      if (t)
> +	return t;
> +    }
> +
> +  return fold_build2_loc (UNKNOWN_LOCATION, TRUTH_OR_EXPR,

Why did you change it to UNKNOWN_LOCATION?  Instead pass in
the location from the caller here.

Ok with that change (_not_ as a followup).

Thanks,
Richard.

> +			  boolean_type_node, c1, c2);
> +}
> +
>  /* Add condition NC to the predicate list of basic block BB.  */
>  
>  static inline void
> @@ -313,27 +334,7 @@ add_to_predicate_list (basic_block bb, tree nc)
>    if (!is_predicated (bb))
>      bc = nc;
>    else
> -    {
> -      enum tree_code code1, code2;
> -      tree op1a, op1b, op2a, op2b;
> -
> -      bc = bb_predicate (bb);
> -      code1 = parse_predicate (bc, &op1a, &op1b);
> -      code2 = parse_predicate (nc, &op2a, &op2b);
> -
> -      if (code1 != ERROR_MARK && code2 != ERROR_MARK)
> -	{
> -	  tree t = maybe_fold_or_comparisons (code1, op1a, op1b,
> -					      code2, op2a, op2b);
> -	  if (!t)
> -	    t = fold_build2_loc (EXPR_LOCATION (bc), TRUTH_OR_EXPR,
> -				 boolean_type_node, bc, nc);
> -	  bc = t;
> -	}
> -      else
> -	bc = fold_build2_loc (EXPR_LOCATION (bc), TRUTH_OR_EXPR,
> -			      boolean_type_node, bc, nc);
> -    }
> +    bc = fold_or_predicates (nc, bb_predicate (bb));
>  
>    if (!is_gimple_condexpr (bc))
>      {

On Fri, Jul 9, 2010 at 07:12, Richard Guenther <rguenther@suse.de> wrote:
> On Thu, 8 Jul 2010, Sebastian Pop wrote:
>
>>       * tree-if-conv.c (fold_or_predicates): New.
>>       (add_to_predicate_list): Call it.
>
> I requested this change before the previous patch was checked in.

No, you did not request this change.

> Please do not ignore reviews this way.  Thanks.

I do not ignore reviews.

>
>> ---
>>  gcc/tree-if-conv.c |   43 ++++++++++++++++++++++---------------------
>>  1 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
>> index 34b4159..cac5a3b 100644
>> --- a/gcc/tree-if-conv.c
>> +++ b/gcc/tree-if-conv.c
>> @@ -300,6 +300,27 @@ parse_predicate (tree cond, tree *op0, tree *op1)
>>    return ERROR_MARK;
>>  }
>>
>> +/* Returns the fold of predicate C1 OR C2.  */
>> +
>> +static tree
>> +fold_or_predicates (tree c1, tree c2)
>> +{
>> +  tree op1a, op1b, op2a, op2b;
>> +  enum tree_code code1 = parse_predicate (c1, &op1a, &op1b);
>> +  enum tree_code code2 = parse_predicate (c2, &op2a, &op2b);
>> +
>> +  if (code1 != ERROR_MARK && code2 != ERROR_MARK)
>> +    {
>> +      tree t = maybe_fold_or_comparisons (code1, op1a, op1b,
>> +                                       code2, op2a, op2b);
>> +      if (t)
>> +     return t;
>> +    }
>> +
>> +  return fold_build2_loc (UNKNOWN_LOCATION, TRUTH_OR_EXPR,
>
> Why did you change it to UNKNOWN_LOCATION?  Instead pass in
> the location from the caller here.
>
> Ok with that change (_not_ as a followup).
>

I will do this and I will post the updated patch.

Thanks,
Sebastian