Patchwork Tighten checking of vector comparisons

login
register
mail settings
Submitter Marc Glisse
Date Nov. 1, 2012, 9:10 p.m.
Message ID <alpine.DEB.2.02.1211012153290.31724@stedding.saclay.inria.fr>
Download mbox | patch
Permalink /patch/196381/
State New
Headers show

Comments

Marc Glisse - Nov. 1, 2012, 9:10 p.m.
Hello,

this patch makes gimple checking of vector comparisons more strict by 
ensuring that it doesn't return a boolean. It also fixes a bug that this 
check detected in fold-const.c: for (void)(x<0), the C front-end was 
calling fold_unary_loc on the conversion to void, which was then converted 
to if(x<0)(void)0.

2012-11-02  Marc Glisse  <marc.glisse@inria.fr>

 	* tree-cfg.c (verify_gimple_comparison): Verify that vector
 	comparison returns a vector.
 	* fold-const.c (fold_unary_loc): Disable conversion optimization
 	for void type.
Richard Guenther - Nov. 4, 2012, 5 p.m.
On Thu, Nov 1, 2012 at 10:10 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> this patch makes gimple checking of vector comparisons more strict by
> ensuring that it doesn't return a boolean. It also fixes a bug that this
> check detected in fold-const.c: for (void)(x<0), the C front-end was calling
> fold_unary_loc on the conversion to void, which was then converted to
> if(x<0)(void)0.
>
> 2012-11-02  Marc Glisse  <marc.glisse@inria.fr>
>
>         * tree-cfg.c (verify_gimple_comparison): Verify that vector
>         comparison returns a vector.
>         * fold-const.c (fold_unary_loc): Disable conversion optimization
>         for void type.
>
> --
> Marc Glisse
> Index: gcc/fold-const.c
> ===================================================================
> --- gcc/fold-const.c    (revision 193060)
> +++ gcc/fold-const.c    (working copy)
> @@ -7742,21 +7742,22 @@ fold_unary_loc (location_t loc, enum tre
>           /* If we have (type) (a CMP b) and type is an integral type,
> return
>              new expression involving the new type.  Canonicalize
>              (type) (a CMP b) to (a CMP b) ? (type) true : (type) false for
>              non-integral type.
>              Do not fold the result as that would not simplify further, also
>              folding again results in recursions.  */
>           if (TREE_CODE (type) == BOOLEAN_TYPE)
>             return build2_loc (loc, TREE_CODE (op0), type,
>                                TREE_OPERAND (op0, 0),
>                                TREE_OPERAND (op0, 1));
> -         else if (!INTEGRAL_TYPE_P (type) && TREE_CODE (type) !=
> VECTOR_TYPE)
> +         else if (!INTEGRAL_TYPE_P (type) && !VOID_TYPE_P (type)
> +                  && TREE_CODE (type) != VECTOR_TYPE)
>             return build3_loc (loc, COND_EXPR, type, op0,
>                                constant_boolean_node (true, type),
>                                constant_boolean_node (false, type));
>         }
>
>        /* Handle cases of two conversions in a row.  */
>        if (CONVERT_EXPR_P (op0))
>         {
>           tree inside_type = TREE_TYPE (TREE_OPERAND (op0, 0));
>           tree inter_type = TREE_TYPE (op0);

Ok for this part.

> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c      (revision 193060)
> +++ gcc/tree-cfg.c      (working copy)
> @@ -3263,21 +3263,30 @@ verify_gimple_comparison (tree type, tre
>        error ("mismatching comparison operand types");
>        debug_generic_expr (op0_type);
>        debug_generic_expr (op1_type);
>        return true;
>      }
>
>    /* The resulting type of a comparison may be an effective boolean type.
> */
>    if (INTEGRAL_TYPE_P (type)
>        && (TREE_CODE (type) == BOOLEAN_TYPE
>           || TYPE_PRECISION (type) == 1))
> -    ;
> +    {
> +      if (TREE_CODE (op0_type) == VECTOR_TYPE
> +         || TREE_CODE (op1_type) == VECTOR_TYPE)
> +        {
> +          error ("vector comparison returning a boolean");
> +          debug_generic_expr (op0_type);
> +          debug_generic_expr (op1_type);
> +          return true;
> +        }

verify_gimple_* should have "positive" checks, thus, check that
if there are vector operands the comparison result should be a
vector.  Not complaining about a vector comparison having a
boolean result.

Richard.

> +    }
>    /* Or an integer vector type with the same size and element count
>       as the comparison operand types.  */
>    else if (TREE_CODE (type) == VECTOR_TYPE
>            && TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE)
>      {
>        if (TREE_CODE (op0_type) != VECTOR_TYPE
>           || TREE_CODE (op1_type) != VECTOR_TYPE)
>          {
>            error ("non-vector operands in vector comparison");
>            debug_generic_expr (op0_type);
>
Marc Glisse - Nov. 22, 2012, 10:59 a.m.
(I forgot to send this at the time)

On Sun, 4 Nov 2012, Richard Biener wrote:

>> -         else if (!INTEGRAL_TYPE_P (type) && TREE_CODE (type) !=
>> VECTOR_TYPE)
>> +         else if (!INTEGRAL_TYPE_P (type) && !VOID_TYPE_P (type)
>> +                  && TREE_CODE (type) != VECTOR_TYPE)
[...]
> Ok for this part.

Applied, thanks.

>> Index: gcc/tree-cfg.c
>> ===================================================================
>> --- gcc/tree-cfg.c      (revision 193060)
>> +++ gcc/tree-cfg.c      (working copy)
>> @@ -3263,21 +3263,30 @@ verify_gimple_comparison (tree type, tre
>>        error ("mismatching comparison operand types");
>>        debug_generic_expr (op0_type);
>>        debug_generic_expr (op1_type);
>>        return true;
>>      }
>>
>>    /* The resulting type of a comparison may be an effective boolean type.
>> */
>>    if (INTEGRAL_TYPE_P (type)
>>        && (TREE_CODE (type) == BOOLEAN_TYPE
>>           || TYPE_PRECISION (type) == 1))
>> -    ;
>> +    {
>> +      if (TREE_CODE (op0_type) == VECTOR_TYPE
>> +         || TREE_CODE (op1_type) == VECTOR_TYPE)
>> +        {
>> +          error ("vector comparison returning a boolean");
>> +          debug_generic_expr (op0_type);
>> +          debug_generic_expr (op1_type);
>> +          return true;
>> +        }
>
> verify_gimple_* should have "positive" checks, thus, check that
> if there are vector operands the comparison result should be a
> vector.  Not complaining about a vector comparison having a
> boolean result.

I wasn't sure what that was supposed to look like, so I dropped it for 
now.
Richard Guenther - Nov. 26, 2012, 3:36 p.m.
On Thu, Nov 22, 2012 at 11:59 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> (I forgot to send this at the time)
>
>
> On Sun, 4 Nov 2012, Richard Biener wrote:
>
>>> -         else if (!INTEGRAL_TYPE_P (type) && TREE_CODE (type) !=
>>> VECTOR_TYPE)
>>> +         else if (!INTEGRAL_TYPE_P (type) && !VOID_TYPE_P (type)
>>> +                  && TREE_CODE (type) != VECTOR_TYPE)
>
> [...]
>>
>> Ok for this part.
>
>
> Applied, thanks.
>
>
>>> Index: gcc/tree-cfg.c
>>> ===================================================================
>>> --- gcc/tree-cfg.c      (revision 193060)
>>> +++ gcc/tree-cfg.c      (working copy)
>>> @@ -3263,21 +3263,30 @@ verify_gimple_comparison (tree type, tre
>>>        error ("mismatching comparison operand types");
>>>        debug_generic_expr (op0_type);
>>>        debug_generic_expr (op1_type);
>>>        return true;
>>>      }
>>>
>>>    /* The resulting type of a comparison may be an effective boolean
>>> type.
>>> */
>>>    if (INTEGRAL_TYPE_P (type)
>>>        && (TREE_CODE (type) == BOOLEAN_TYPE
>>>           || TYPE_PRECISION (type) == 1))
>>> -    ;
>>> +    {
>>> +      if (TREE_CODE (op0_type) == VECTOR_TYPE
>>> +         || TREE_CODE (op1_type) == VECTOR_TYPE)
>>> +        {
>>> +          error ("vector comparison returning a boolean");
>>> +          debug_generic_expr (op0_type);
>>> +          debug_generic_expr (op1_type);
>>> +          return true;
>>> +        }
>>
>>
>> verify_gimple_* should have "positive" checks, thus, check that
>> if there are vector operands the comparison result should be a
>> vector.  Not complaining about a vector comparison having a
>> boolean result.
>
>
> I wasn't sure what that was supposed to look like, so I dropped it for now.

Ok, looking closer we have

  /* The resulting type of a comparison may be an effective boolean type.  */
  if (INTEGRAL_TYPE_P (type)
      && (TREE_CODE (type) == BOOLEAN_TYPE
          || TYPE_PRECISION (type) == 1))
    ;
  /* Or an integer vector type with the same size and element count
     as the comparison operand types.  */
  else if (TREE_CODE (type) == VECTOR_TYPE
           && TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE)
    {
...

Thus you are rejecting a boolean valued vector comparison which we
otherwise happily accept.  I suppose that makes sense (even though
at least equality compares can make sense).

Thus that hunk is ok as well.

Thanks,
Richard.

> --
> Marc Glisse
Marc Glisse - Nov. 27, 2012, 6:06 p.m.
On Mon, 26 Nov 2012, Richard Biener wrote:

> Thus you are rejecting a boolean valued vector comparison which we
> otherwise happily accept.  I suppose that makes sense (even though
> at least equality compares can make sense).

I agree that boolean equality comparison of vectors makes sense, but I 
don't think the code is ready to properly expand it. I guess we'll have to 
revisit this at some point.

> Thus that hunk is ok as well.

Done, thanks.

Patch

Index: gcc/fold-const.c

===================================================================
--- gcc/fold-const.c	(revision 193060)

+++ gcc/fold-const.c	(working copy)

@@ -7742,21 +7742,22 @@  fold_unary_loc (location_t loc, enum tre

 	  /* If we have (type) (a CMP b) and type is an integral type, return
 	     new expression involving the new type.  Canonicalize
 	     (type) (a CMP b) to (a CMP b) ? (type) true : (type) false for
 	     non-integral type.
 	     Do not fold the result as that would not simplify further, also
 	     folding again results in recursions.  */
 	  if (TREE_CODE (type) == BOOLEAN_TYPE)
 	    return build2_loc (loc, TREE_CODE (op0), type,
 			       TREE_OPERAND (op0, 0),
 			       TREE_OPERAND (op0, 1));
-	  else if (!INTEGRAL_TYPE_P (type) && TREE_CODE (type) != VECTOR_TYPE)

+	  else if (!INTEGRAL_TYPE_P (type) && !VOID_TYPE_P (type)

+		   && TREE_CODE (type) != VECTOR_TYPE)

 	    return build3_loc (loc, COND_EXPR, type, op0,
 			       constant_boolean_node (true, type),
 			       constant_boolean_node (false, type));
 	}
 
       /* Handle cases of two conversions in a row.  */
       if (CONVERT_EXPR_P (op0))
 	{
 	  tree inside_type = TREE_TYPE (TREE_OPERAND (op0, 0));
 	  tree inter_type = TREE_TYPE (op0);
Index: gcc/tree-cfg.c

===================================================================
--- gcc/tree-cfg.c	(revision 193060)

+++ gcc/tree-cfg.c	(working copy)

@@ -3263,21 +3263,30 @@  verify_gimple_comparison (tree type, tre

       error ("mismatching comparison operand types");
       debug_generic_expr (op0_type);
       debug_generic_expr (op1_type);
       return true;
     }
 
   /* The resulting type of a comparison may be an effective boolean type.  */
   if (INTEGRAL_TYPE_P (type)
       && (TREE_CODE (type) == BOOLEAN_TYPE
 	  || TYPE_PRECISION (type) == 1))
-    ;

+    {

+      if (TREE_CODE (op0_type) == VECTOR_TYPE

+	  || TREE_CODE (op1_type) == VECTOR_TYPE)

+        {

+          error ("vector comparison returning a boolean");

+          debug_generic_expr (op0_type);

+          debug_generic_expr (op1_type);

+          return true;

+        }

+    }

   /* Or an integer vector type with the same size and element count
      as the comparison operand types.  */
   else if (TREE_CODE (type) == VECTOR_TYPE
 	   && TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE)
     {
       if (TREE_CODE (op0_type) != VECTOR_TYPE
 	  || TREE_CODE (op1_type) != VECTOR_TYPE)
         {
           error ("non-vector operands in vector comparison");
           debug_generic_expr (op0_type);