[patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

Hello,

this patch adds a check to

ChangeLog

2011-05-13  Kai Tietz

          * gimplify.c (gimplify_expr): Make sure operand is boolified.
          * tree-cfg.c (verify_gimple_assign_unary): Check for boolean
compatible type
          for TRUTH_NOT_EXPR.

Bootstrapped for x86_64-pc-linux-gnu (multilib). Ok for apply?

Index: tree-cfg.c
===================================================================
--- tree-cfg.c  (revision 173725)
+++ tree-cfg.c  (working copy)
@@ -3342,6 +3342,14 @@
       return false;

     case TRUTH_NOT_EXPR:
+      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
+          || !useless_type_conversion_p (boolean_type_node,  lhs_type))
+        {
+           error ("invalid types in truth not");
+           debug_generic_expr (lhs_type);
+           debug_generic_expr (rhs1_type);
+           return true;
+        }
     case NEGATE_EXPR:
     case ABS_EXPR:
     case BIT_NOT_EXPR:

Index: gimplify.c
===================================================================
--- gimplify.c  (revision 173726)
+++ gimplify.c  (working copy)
@@ -6754,14 +6754,18 @@
          }

        case TRUTH_NOT_EXPR:
-         if (TREE_TYPE (*expr_p) != boolean_type_node)
-           {
-             tree type = TREE_TYPE (*expr_p);
-             *expr_p = fold_convert (type, gimple_boolify (*expr_p));
-             ret = GS_OK;
-             break;
-           }
+         {
+           tree org_type = TREE_TYPE (*expr_p);

+           *expr_p = gimple_boolify (*expr_p);
+           if (org_type != boolean_type_node)
+             {
+               *expr_p = fold_convert (org_type, *expr_p);
+               ret = GS_OK;
+               break;
+             }
+         }
+
          ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
                               is_gimple_val, fb_rvalue);
          recalculate_side_effects (*expr_p);

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Richard Guenther]

On Fri, May 13, 2011 at 1:31 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hello,
>
> this patch adds a check to
>
> ChangeLog
>
> 2011-05-13  Kai Tietz
>
>          * gimplify.c (gimplify_expr): Make sure operand is boolified.
>          * tree-cfg.c (verify_gimple_assign_unary): Check for boolean
> compatible type
>          for TRUTH_NOT_EXPR.
>
> Bootstrapped for x86_64-pc-linux-gnu (multilib). Ok for apply?

Ok with ....

> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 173725)
> +++ tree-cfg.c  (working copy)
> @@ -3342,6 +3342,14 @@
>       return false;
>
>     case TRUTH_NOT_EXPR:
> +      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
> +          || !useless_type_conversion_p (boolean_type_node,  lhs_type))

the 2nd check removed, it's redundant with the common check below

  /* For the remaining codes assert there is no conversion involved.  */
  if (!useless_type_conversion_p (lhs_type, rhs1_type))
    {
      error ("non-trivial conversion in unary operation");
      debug_generic_expr (lhs_type);
      debug_generic_expr (rhs1_type);
      return true;
    }

and ...

> +        {
> +           error ("invalid types in truth not");
> +           debug_generic_expr (lhs_type);
> +           debug_generic_expr (rhs1_type);
> +           return true;
> +        }

please do not fall through here but add a break.

Richard.

>     case NEGATE_EXPR:
>     case ABS_EXPR:
>     case BIT_NOT_EXPR:
>
> Index: gimplify.c
> ===================================================================
> --- gimplify.c  (revision 173726)
> +++ gimplify.c  (working copy)
> @@ -6754,14 +6754,18 @@
>          }
>
>        case TRUTH_NOT_EXPR:
> -         if (TREE_TYPE (*expr_p) != boolean_type_node)
> -           {
> -             tree type = TREE_TYPE (*expr_p);
> -             *expr_p = fold_convert (type, gimple_boolify (*expr_p));
> -             ret = GS_OK;
> -             break;
> -           }
> +         {
> +           tree org_type = TREE_TYPE (*expr_p);
>
> +           *expr_p = gimple_boolify (*expr_p);
> +           if (org_type != boolean_type_node)
> +             {
> +               *expr_p = fold_convert (org_type, *expr_p);
> +               ret = GS_OK;
> +               break;
> +             }
> +         }
> +
>          ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
>                               is_gimple_val, fb_rvalue);
>          recalculate_side_effects (*expr_p);
>

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

2011/5/13 Richard Guenther <richard.guenther@gmail.com>:
> On Fri, May 13, 2011 at 1:31 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> Hello,
>>
>> this patch adds a check to
>>
>> ChangeLog
>>
>> 2011-05-13  Kai Tietz
>>
>>          * gimplify.c (gimplify_expr): Make sure operand is boolified.
>>          * tree-cfg.c (verify_gimple_assign_unary): Check for boolean
>> compatible type
>>          for TRUTH_NOT_EXPR.
>>
>> Bootstrapped for x86_64-pc-linux-gnu (multilib). Ok for apply?
>
> Ok with ....
>
>> Index: tree-cfg.c
>> ===================================================================
>> --- tree-cfg.c  (revision 173725)
>> +++ tree-cfg.c  (working copy)
>> @@ -3342,6 +3342,14 @@
>>       return false;
>>
>>     case TRUTH_NOT_EXPR:
>> +      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
>> +          || !useless_type_conversion_p (boolean_type_node,  lhs_type))
>
> the 2nd check removed, it's redundant with the common check below
>
>  /* For the remaining codes assert there is no conversion involved.  */
>  if (!useless_type_conversion_p (lhs_type, rhs1_type))
>    {
>      error ("non-trivial conversion in unary operation");
>      debug_generic_expr (lhs_type);
>      debug_generic_expr (rhs1_type);
>      return true;
>    }
>
> and ...
>
>> +        {
>> +           error ("invalid types in truth not");
>> +           debug_generic_expr (lhs_type);
>> +           debug_generic_expr (rhs1_type);
>> +           return true;
>> +        }
>
> please do not fall through here but add a break.
>
> Richard.
>
>>     case NEGATE_EXPR:
>>     case ABS_EXPR:
>>     case BIT_NOT_EXPR:
>>
>> Index: gimplify.c
>> ===================================================================
>> --- gimplify.c  (revision 173726)
>> +++ gimplify.c  (working copy)
>> @@ -6754,14 +6754,18 @@
>>          }
>>
>>        case TRUTH_NOT_EXPR:
>> -         if (TREE_TYPE (*expr_p) != boolean_type_node)
>> -           {
>> -             tree type = TREE_TYPE (*expr_p);
>> -             *expr_p = fold_convert (type, gimple_boolify (*expr_p));
>> -             ret = GS_OK;
>> -             break;
>> -           }
>> +         {
>> +           tree org_type = TREE_TYPE (*expr_p);
>>
>> +           *expr_p = gimple_boolify (*expr_p);
>> +           if (org_type != boolean_type_node)
>> +             {
>> +               *expr_p = fold_convert (org_type, *expr_p);
>> +               ret = GS_OK;
>> +               break;
>> +             }
>> +         }
>> +
>>          ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
>>                               is_gimple_val, fb_rvalue);
>>          recalculate_side_effects (*expr_p);
>>
>

Committed at revision 173732 with your suggested adjustments.

Thanks,
Kai

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[H.J. Lu]

On Fri, May 13, 2011 at 6:38 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2011/5/13 Richard Guenther <richard.guenther@gmail.com>:
>> On Fri, May 13, 2011 at 1:31 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> Hello,
>>>
>>> this patch adds a check to
>>>
>>> ChangeLog
>>>
>>> 2011-05-13  Kai Tietz
>>>
>>>          * gimplify.c (gimplify_expr): Make sure operand is boolified.
>>>          * tree-cfg.c (verify_gimple_assign_unary): Check for boolean
>>> compatible type
>>>          for TRUTH_NOT_EXPR.
>>>
>>> Bootstrapped for x86_64-pc-linux-gnu (multilib). Ok for apply?
>>
>> Ok with ....
>>
>
> Committed at revision 173732 with your suggested adjustments.
>

This may have caused:

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

-- 
H.J.

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Andrew Pinski]

On Fri, May 13, 2011 at 4:31 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>
>     case TRUTH_NOT_EXPR:
> +      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
> +          || !useless_type_conversion_p (boolean_type_node,  lhs_type))

In Fortran (and maybe other langauges) there are booleans with
different sizes but the same precision.
Can you explain how you handle those and why this can't just be a
BOOLEAN_TYPE type?

Thanks,
Andrew Pinski

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

2011/5/13 Andrew Pinski <pinskia@gmail.com>:
> On Fri, May 13, 2011 at 4:31 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>
>>     case TRUTH_NOT_EXPR:
>> +      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
>> +          || !useless_type_conversion_p (boolean_type_node,  lhs_type))
>
> In Fortran (and maybe other langauges) there are booleans with
> different sizes but the same precision.
> Can you explain how you handle those and why this can't just be a
> BOOLEAN_TYPE type?

Well, the issue is to be found in gimple_boolify. It is necessary that
it is ensured that operands getting boolified on demand. Doing this by
default boolean_type_node is used.
To verify that, patch converts also for wider-mode BOOLEAN-types back
to boolean_type_node type.
Before thi no boolification of operands were done for expressions with
any boolean type. As we want to make sure that operands getting fixed,
too, but gimplifier doesn't know which resulting type it should have,
we truncate it back to 1-bit boolean.
I assume that the underlying issue is here that some places introduce
- via the backdoor - gimple trees, with abitrary boolean-type. IMHO
those places should be fixed and use for this gimplication to make
sure things getting normalized.

Regards,
Kai

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

2011/5/13 Kai Tietz <ktietz70@googlemail.com>:
> 2011/5/13 Andrew Pinski <pinskia@gmail.com>:
>> On Fri, May 13, 2011 at 4:31 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>
>>>     case TRUTH_NOT_EXPR:
>>> +      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
>>> +          || !useless_type_conversion_p (boolean_type_node,  lhs_type))
>>
>> In Fortran (and maybe other langauges) there are booleans with
>> different sizes but the same precision.
>> Can you explain how you handle those and why this can't just be a
>> BOOLEAN_TYPE type?
>
> Well, the issue is to be found in gimple_boolify. It is necessary that
> it is ensured that operands getting boolified on demand. Doing this by
> default boolean_type_node is used.
> To verify that, patch converts also for wider-mode BOOLEAN-types back
> to boolean_type_node type.
> Before thi no boolification of operands were done for expressions with
> any boolean type. As we want to make sure that operands getting fixed,
> too, but gimplifier doesn't know which resulting type it should have,
> we truncate it back to 1-bit boolean.
> I assume that the underlying issue is here that some places introduce
> - via the backdoor - gimple trees, with abitrary boolean-type. IMHO
> those places should be fixed and use for this gimplication to make
> sure things getting normalized.

I investigate this a bit more in detail and indeed it is the fortran
FE which do here enter in trans-*.c files their own gimple code. And
this code doesn't run over the gimplify_expr routines and so it gets
in conflict with tree-cfg.c checks.
There are two ways to go. A) Adjust fortran's trans-* stuff so that it
uses for conditionals and logical-expressions boolean_type consequent
(as in some place this type is used, but sometimes artifical
boolean_types direct). Or B) We relax tree-cfg checks for logical ops
to allow also BOOLEAN types with wider size then 1-bit.

Regards,
Kai

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Andrew Pinski]

On Fri, May 13, 2011 at 11:24 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> boolean_types direct). Or B) We relax tree-cfg checks for logical ops
> to allow also BOOLEAN types with wider size then 1-bit.

The Boolean types which fortran produces are 1bit wide in precision
but not always the same width as the C  _Bool type.

That is TYPE_PRECISION is 1 for those types but TYPE_SIZE is not the same.

Thanks,
Andrew Pinski

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Eric Botcazou]

> In Fortran (and maybe other langauges) there are booleans with
> different sizes but the same precision.

Ada doesn't have a C-like boolean type either.  The patches have introduced:

FAIL: gnat.dg/lto1.adb (test for excess errors)


/home/eric/svn/gcc/gcc/testsuite/gnat.dg/lto1_pkg.adb:23:1: error: type 
mismatch in binary truth expression
boolean
boolean
boolean
D.2419_18 = D.2417_16 || D.2418_17;
+===========================GNAT BUG DETECTED==============================+
| 4.7.0 20110513 (experimental) [trunk revision 173737] (i586-suse-linux-gnu) 
GCC error:|
| verify_gimple failed                                          

-- 
Eric Botcazou

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

2011/5/14 Eric Botcazou <ebotcazou@adacore.com>:
>> In Fortran (and maybe other langauges) there are booleans with
>> different sizes but the same precision.
>
> Ada doesn't have a C-like boolean type either.  The patches have introduced:
>
> FAIL: gnat.dg/lto1.adb (test for excess errors)
>
>
> /home/eric/svn/gcc/gcc/testsuite/gnat.dg/lto1_pkg.adb:23:1: error: type
> mismatch in binary truth expression
> boolean
> boolean
> boolean
> D.2419_18 = D.2417_16 || D.2418_17;
> +===========================GNAT BUG DETECTED==============================+
> | 4.7.0 20110513 (experimental) [trunk revision 173737] (i586-suse-linux-gnu)
> GCC error:|
> | verify_gimple failed

Those issues should be fixed by the attached patch, which relaxes
strictness of logical operations in tree-cfg.c file.  Not sure if
Richard is ok by this, as it shows that FE are generating here
gimplify-incompatible SSA trees, which seems to me at least something
not that good here.

ChangeLog

2011-05-14  Kai Tietz

        * tree-cfg.c (verify_gimple_assign_unary): Don't enforce
boolean_type_node
        compatible lhs/rhs types for logical expression.
        (verify_gimple_assign_binary): Likewise.

Regards,
Kai

[-- Attachment #2: relax_tree_cfg.txt --]
[-- Type: text/plain, Size: 1522 bytes --]

Index: gcc/gcc/tree-cfg.c
===================================================================
--- gcc.orig/gcc/tree-cfg.c	2011-05-13 17:28:15.000000000 +0200
+++ gcc/gcc/tree-cfg.c	2011-05-14 16:04:31.995831500 +0200
@@ -3350,15 +3350,6 @@ verify_gimple_assign_unary (gimple stmt)
       return false;
 
     case TRUTH_NOT_EXPR:
-      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type))
-        {
-	    error ("invalid types in truth not");
-	    debug_generic_expr (lhs_type);
-	    debug_generic_expr (rhs1_type);
-	    return true;
-        }
-      break;
-
     case NEGATE_EXPR:
     case ABS_EXPR:
     case BIT_NOT_EXPR:
@@ -3558,10 +3549,13 @@ do_pointer_plus_expr_check:
     case TRUTH_OR_EXPR:
     case TRUTH_XOR_EXPR:
       {
-	/* We allow only boolean typed or compatible argument and result.  */
-	if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
-	    || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
-	    || !useless_type_conversion_p (boolean_type_node,  lhs_type))
+	/* The gimplify code ensures that just boolean_type_node types
+	   are present, but ADA and FORTRAN using artificial gimple generation
+	   code which is producing non-gimplify compatible trees.  So we need
+	   to allow here 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);

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Eric Botcazou]

> Those issues should be fixed by the attached patch, which relaxes
> strictness of logical operations in tree-cfg.c file.

Thanks.

> 2011-05-14  Kai Tietz
>
>         * tree-cfg.c (verify_gimple_assign_unary): Don't enforce
> boolean_type_node
>         compatible lhs/rhs types for logical expression.
>         (verify_gimple_assign_binary): Likewise.

-	/* We allow only boolean typed or compatible argument and result.  */
-	if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
-	    || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
-	    || !useless_type_conversion_p (boolean_type_node,  lhs_type))
+	/* The gimplify code ensures that just boolean_type_node types
+	   are present, but ADA and FORTRAN using artificial gimple generation
+	   code which is producing non-gimplify compatible trees.  So we need
+	   to allow here any kind of integral typed argument and result.  */
+	if (!INTEGRAL_TYPE_P (rhs1_type)
+	    || !INTEGRAL_TYPE_P (rhs2_type)
+	    || !INTEGRAL_TYPE_P (lhs_type))

What does "artificial gimple generation code" mean exactly?  The only thing 
different in Ada is the definition of boolean_type_node which isn't compatible 
with the C definition (its precision is 8 instead of 1).  That's all.

For Ada, you can just do:

Index: tree-cfg.c
===================================================================
--- tree-cfg.c  (revision 173756)
+++ tree-cfg.c  (working copy)
@@ -3350,7 +3350,7 @@ verify_gimple_assign_unary (gimple stmt)
       return false;

     case TRUTH_NOT_EXPR:
-      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type))
+      if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE)
         {
            error ("invalid types in truth not");
            debug_generic_expr (lhs_type);
@@ -3558,10 +3558,10 @@ do_pointer_plus_expr_check:
     case TRUTH_OR_EXPR:
     case TRUTH_XOR_EXPR:
       {
-       /* We allow only boolean typed or compatible argument and result.  */
-       if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
-           || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
-           || !useless_type_conversion_p (boolean_type_node,  lhs_type))
+       /* We allow only boolean-typed argument and result.  */
+       if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE
+           || TREE_CODE (rhs2_type) != BOOLEAN_TYPE
+           || TREE_CODE (lhs_type) != BOOLEAN_TYPE)
          {
            error ("type mismatch in binary truth expression");
            debug_generic_expr (lhs_type);

-- 
Eric Botcazou

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

2011/5/14 Eric Botcazou <ebotcazou@adacore.com>:
>> Those issues should be fixed by the attached patch, which relaxes
>> strictness of logical operations in tree-cfg.c file.
>
> Thanks.
>
>> 2011-05-14  Kai Tietz
>>
>>         * tree-cfg.c (verify_gimple_assign_unary): Don't enforce
>> boolean_type_node
>>         compatible lhs/rhs types for logical expression.
>>         (verify_gimple_assign_binary): Likewise.
>
> -       /* We allow only boolean typed or compatible argument and result.  */
> -       if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
> -           || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
> -           || !useless_type_conversion_p (boolean_type_node,  lhs_type))
> +       /* The gimplify code ensures that just boolean_type_node types
> +          are present, but ADA and FORTRAN using artificial gimple generation
> +          code which is producing non-gimplify compatible trees.  So we need
> +          to allow here any kind of integral typed argument and result.  */
> +       if (!INTEGRAL_TYPE_P (rhs1_type)
> +           || !INTEGRAL_TYPE_P (rhs2_type)
> +           || !INTEGRAL_TYPE_P (lhs_type))
>
> What does "artificial gimple generation code" mean exactly?  The only thing
> different in Ada is the definition of boolean_type_node which isn't compatible
> with the C definition (its precision is 8 instead of 1).  That's all.

Well, I mean by artificial here, that gimplification is done via
gimplify_expr API. As FE and ME have here different assumptions.  The
ME uses internally most boolean_type_node and IMHO it should be the
variant used there. As this conversation to a single boolean_type
(with recast to result FE's boolean type on demand) has some
advantages on optimization passes.  Additionally it simplifies logic
in passes on types.  For example there are some expressions, which are
in general unexpected in ME as they are transformed in gimplification
(like TRUTH_ANDIF/ORIF_EXPR).  By adding tree manual, you might cause
the same issue as for the logical-expression showing up now.

> For Ada, you can just do:
>
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 173756)
> +++ tree-cfg.c  (working copy)
> @@ -3350,7 +3350,7 @@ verify_gimple_assign_unary (gimple stmt)
>       return false;
>
>     case TRUTH_NOT_EXPR:
> -      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type))
> +      if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE)
>         {
>            error ("invalid types in truth not");
>            debug_generic_expr (lhs_type);
> @@ -3558,10 +3558,10 @@ do_pointer_plus_expr_check:
>     case TRUTH_OR_EXPR:
>     case TRUTH_XOR_EXPR:
>       {
> -       /* We allow only boolean typed or compatible argument and result.  */
> -       if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
> -           || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
> -           || !useless_type_conversion_p (boolean_type_node,  lhs_type))
> +       /* We allow only boolean-typed argument and result.  */
> +       if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE
> +           || TREE_CODE (rhs2_type) != BOOLEAN_TYPE
> +           || TREE_CODE (lhs_type) != BOOLEAN_TYPE)
>          {
>            error ("type mismatch in binary truth expression");
>            debug_generic_expr (lhs_type);
>
> --
> Eric Botcazou
>

Well, this patch might be an alternative, but I see here potential
issues in such none-gimplified expressions for comparision and logical
not, which not necessariily have BOOLEAN_TYPE.  See here the code for
fold_truth_not (and some other places) in fold-const.  So I think, as
long as we have here external gimplication it is more save to check
just for integral-kind.

At the moment I am discussing with Tobias Burnus about wrapping
fold_build calls for logical operations (COND_EXPR (conditional
operand); TRUTH..., and possibly comparison too) , so that the
operation is done on boolean_type_node and the result gets re-casted
back to FE's logical boolean type.  This would allow us to keep in
tree-cfg the check for one-bit operations for truth-expressions.
The general advantage here is that later passes (like reassoc - by
moving TRUTH-AND/OR/XOR to binary variant BIT_AND/OR/XOR) would show
effects and inner operands can be matched.  Also by oiperating on
one-bit types here allows us to assume even that TRUTH_NOT_EXPR can be
transformed to BIT_NOT_EXPR (or XOR by 1).

But well, I would like to know Richard's opinion here.

Regards,
Kai

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Eric Botcazou]

> Well, I mean by artificial here, that gimplification is done via
> gimplify_expr API. As FE and ME have here different assumptions.  The
> ME uses internally most boolean_type_node and IMHO it should be the
> variant used there. As this conversation to a single boolean_type
> (with recast to result FE's boolean type on demand) has some
> advantages on optimization passes.  Additionally it simplifies logic
> in passes on types.  For example there are some expressions, which are
> in general unexpected in ME as they are transformed in gimplification
> (like TRUTH_ANDIF/ORIF_EXPR).  By adding tree manual, you might cause
> the same issue as for the logical-expression showing up now.

OK, then that's definitely not the case for Ada, so the comment is incorrect.

> Well, this patch might be an alternative, but I see here potential
> issues in such none-gimplified expressions for comparision and logical
> not, which not necessariily have BOOLEAN_TYPE.  See here the code for
> fold_truth_not (and some other places) in fold-const.  So I think, as
> long as we have here external gimplication it is more save to check
> just for integral-kind.

Note that, even without "external gimplication", we still have integral types 
down to the tree-cfg.c check.  Take ACATS c52103x at -O0.  The Ada FE hands 
over a valid TRUTH_AND_EXPR, i.e. (BOOLEAN_TYPE, BOOLEAN_TYPE, BOOLEAN_TYPE) 
but the gimplifier builds a (BOOLEAN_TYPE, INTEGER_TYPE, BOOLEAN_TYPE) as it 
strips, then adds, then re-strips a cast to BOOLEAN_TYPE in gimplify_expr.

-- 
Eric Botcazou

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

2011/5/15 Eric Botcazou <ebotcazou@adacore.com>:
>> Well, I mean by artificial here, that gimplification is done via
>> gimplify_expr API. As FE and ME have here different assumptions.  The
>> ME uses internally most boolean_type_node and IMHO it should be the
>> variant used there. As this conversation to a single boolean_type
>> (with recast to result FE's boolean type on demand) has some
>> advantages on optimization passes.  Additionally it simplifies logic
>> in passes on types.  For example there are some expressions, which are
>> in general unexpected in ME as they are transformed in gimplification
>> (like TRUTH_ANDIF/ORIF_EXPR).  By adding tree manual, you might cause
>> the same issue as for the logical-expression showing up now.
>
> OK, then that's definitely not the case for Ada, so the comment is incorrect.

Yes, I will adjust comment here about ADA. Code for ADA looks sane.
Just one nit I saw in trans.c, which might be a cause here.

>> Well, this patch might be an alternative, but I see here potential
>> issues in such none-gimplified expressions for comparision and logical
>> not, which not necessariily have BOOLEAN_TYPE.  See here the code for
>> fold_truth_not (and some other places) in fold-const.  So I think, as
>> long as we have here external gimplication it is more save to check
>> just for integral-kind.
>
> Note that, even without "external gimplication", we still have integral types
> down to the tree-cfg.c check.  Take ACATS c52103x at -O0.  The Ada FE hands
> over a valid TRUTH_AND_EXPR, i.e. (BOOLEAN_TYPE, BOOLEAN_TYPE, BOOLEAN_TYPE)
> but the gimplifier builds a (BOOLEAN_TYPE, INTEGER_TYPE, BOOLEAN_TYPE) as it
> strips, then adds, then re-strips a cast to BOOLEAN_TYPE in gimplify_expr.

With this patch (which would describe why it gimplifier sees
integer-type nodes here):

Index: gcc/gcc/ada/gcc-interface/trans.c
===================================================================
--- gcc.orig/gcc/ada/gcc-interface/trans.c      2011-05-12
20:06:01.000000000 +0200
+++ gcc/gcc/ada/gcc-interface/trans.c   2011-05-15 15:33:32.305516200 +0200
@@ -7101,7 +7110,7 @@ convert_with_check (Entity_Id gnat_type,
     {
       /* Ensure GNU_EXPR only gets evaluated once.  */
       tree gnu_input = gnat_protect_expr (gnu_result);
-      tree gnu_cond = integer_zero_node;
+      tree gnu_cond = boolean_false_node;
       tree gnu_in_lb = TYPE_MIN_VALUE (gnu_in_basetype);
       tree gnu_in_ub = TYPE_MAX_VALUE (gnu_in_basetype);
       tree gnu_out_lb = TYPE_MIN_VALUE (gnu_base_type);

I was able to do a bootstrap for ada and run 'make check-ada' without
seeing gimplification errors.

The only failure I see in testrun is 'cxg2001.adb' test with 'GCC
error: in compensate_edge, at reg-stach.c:2781' Error detect around
cxg2001.adb:322:5.  But well, this bug seems to me unrelated here to
gimplication. But maybe I am wrong here.

Regards,
Kai

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

2011/5/15 Kai Tietz <ktietz70@googlemail.com>:
> 2011/5/15 Eric Botcazou <ebotcazou@adacore.com>:
>>> Well, I mean by artificial here, that gimplification is done via
>>> gimplify_expr API. As FE and ME have here different assumptions.  The
>>> ME uses internally most boolean_type_node and IMHO it should be the
>>> variant used there. As this conversation to a single boolean_type
>>> (with recast to result FE's boolean type on demand) has some
>>> advantages on optimization passes.  Additionally it simplifies logic
>>> in passes on types.  For example there are some expressions, which are
>>> in general unexpected in ME as they are transformed in gimplification
>>> (like TRUTH_ANDIF/ORIF_EXPR).  By adding tree manual, you might cause
>>> the same issue as for the logical-expression showing up now.
>>
>> OK, then that's definitely not the case for Ada, so the comment is incorrect.
>
> Yes, I will adjust comment here about ADA. Code for ADA looks sane.
> Just one nit I saw in trans.c, which might be a cause here.
>
>>> Well, this patch might be an alternative, but I see here potential
>>> issues in such none-gimplified expressions for comparision and logical
>>> not, which not necessariily have BOOLEAN_TYPE.  See here the code for
>>> fold_truth_not (and some other places) in fold-const.  So I think, as
>>> long as we have here external gimplication it is more save to check
>>> just for integral-kind.
>>
>> Note that, even without "external gimplication", we still have integral types
>> down to the tree-cfg.c check.  Take ACATS c52103x at -O0.  The Ada FE hands
>> over a valid TRUTH_AND_EXPR, i.e. (BOOLEAN_TYPE, BOOLEAN_TYPE, BOOLEAN_TYPE)
>> but the gimplifier builds a (BOOLEAN_TYPE, INTEGER_TYPE, BOOLEAN_TYPE) as it
>> strips, then adds, then re-strips a cast to BOOLEAN_TYPE in gimplify_expr.
>
> With this patch (which would describe why it gimplifier sees
> integer-type nodes here):
>
> Index: gcc/gcc/ada/gcc-interface/trans.c
> ===================================================================
> --- gcc.orig/gcc/ada/gcc-interface/trans.c      2011-05-12
> 20:06:01.000000000 +0200
> +++ gcc/gcc/ada/gcc-interface/trans.c   2011-05-15 15:33:32.305516200 +0200
> @@ -7101,7 +7110,7 @@ convert_with_check (Entity_Id gnat_type,
>     {
>       /* Ensure GNU_EXPR only gets evaluated once.  */
>       tree gnu_input = gnat_protect_expr (gnu_result);
> -      tree gnu_cond = integer_zero_node;
> +      tree gnu_cond = boolean_false_node;
>       tree gnu_in_lb = TYPE_MIN_VALUE (gnu_in_basetype);
>       tree gnu_in_ub = TYPE_MAX_VALUE (gnu_in_basetype);
>       tree gnu_out_lb = TYPE_MIN_VALUE (gnu_base_type);
>
> I was able to do a bootstrap for ada and run 'make check-ada' without
> seeing gimplification errors.
>
> The only failure I see in testrun is 'cxg2001.adb' test with 'GCC
> error: in compensate_edge, at reg-stach.c:2781' Error detect around
> cxg2001.adb:322:5.  But well, this bug seems to me unrelated here to
> gimplication. But maybe I am wrong here.
>
> Regards,
> Kai

PS: There are more places to fix, I will sent tomorrow a full patch
for this after bootstrap and testsuite-run was completetly successful.
 I saw in later gnat.dg the described error (but no more the truth-not
issue). So I was a bit to early to post here.

Regards,
Kai

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Eric Botcazou]

> With this patch (which would describe why it gimplifier sees
> integer-type nodes here):
>
> Index: gcc/gcc/ada/gcc-interface/trans.c
> ===================================================================
> --- gcc.orig/gcc/ada/gcc-interface/trans.c      2011-05-12
> 20:06:01.000000000 +0200
> +++ gcc/gcc/ada/gcc-interface/trans.c   2011-05-15 15:33:32.305516200 +0200
> @@ -7101,7 +7110,7 @@ convert_with_check (Entity_Id gnat_type,
>      {
>        /* Ensure GNU_EXPR only gets evaluated once.  */
>        tree gnu_input = gnat_protect_expr (gnu_result);
> -      tree gnu_cond = integer_zero_node;
> +      tree gnu_cond = boolean_false_node;
>        tree gnu_in_lb = TYPE_MIN_VALUE (gnu_in_basetype);
>        tree gnu_in_ub = TYPE_MAX_VALUE (gnu_in_basetype);
>        tree gnu_out_lb = TYPE_MIN_VALUE (gnu_base_type);
>
> I was able to do a bootstrap for ada and run 'make check-ada' without
> seeing gimplification errors.

The patch is OK, but it doesn't change anything for c52103x as this is a pure 
gimplifier problem.  Try running ACATS at -O0:

Index: ada/acats/run_all.sh
===================================================================
--- ada/acats/run_all.sh        (revision 173756)
+++ ada/acats/run_all.sh        (working copy)
@@ -9,7 +9,7 @@
 # gccflags="-O3 -fomit-frame-pointer -funroll-all-loops -finline-functions"
 # gnatflags="-gnatN"

-gccflags="-O2"
+gccflags=""
 gnatflags="-gnatws"

 target_run () {

and you'll see the failures.

> The only failure I see in testrun is 'cxg2001.adb' test with 'GCC
> error: in compensate_edge, at reg-stach.c:2781' Error detect around
> cxg2001.adb:322:5.  But well, this bug seems to me unrelated here to
> gimplication.

Yes, the cxg2001 failure is PR rtl-optimization/48633.

-- 
Eric Botcazou

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

> The patch is OK, but it doesn't change anything for c52103x as this is a
> pure gimplifier problem.  Try running ACATS at -O0:

Or just compile the attached reduced testcase at -O0:

c52103x.adb: In function 'C52103X':
c52103x.adb:1:1: error: type mismatch in binary truth expression
boolean
system__unsigned_types__packed_byte
boolean
D.2363 = D.2361 && D.2362;

-- 
Eric Botcazou

[-- Attachment #2: c52103x.adb --]
[-- Type: text/x-adasrc, Size: 1814 bytes --]

PROCEDURE  C52103X  IS

BEGIN

CONSTR_ERR:         -- THIS BLOCK CATCHES CONSTRAINT_ERROR
                    -- FOR THE TYPE DECLARATION.
     BEGIN

DCL_ARR:  DECLARE   -- THIS BLOCK DECLARES THE ARRAY TYPE

               TYPE  TA42  IS  ARRAY(
                    INTEGER RANGE -2..INTEGER'LAST
                                    )  OF BOOLEAN ;
               -- CONSTRAINT_ERROR MAY BE RAISED BY THE
               -- ARRAY TYPE DECLARATION.
               PRAGMA PACK (TA42);

               SUBTYPE  TA41  IS  TA42 ;

          BEGIN

OBJ_DCL:       DECLARE   -- THIS BLOCK DECLARES TWO BOOLEAN ARRAYS THAT
                         -- HAVE INTEGER'LAST + 3 COMPONENTS;
                         -- STORAGE_ERROR MAY BE RAISED.
                    ARR41  :  TA41 ;
                    ARR42  :  TA42 ;

               BEGIN

DO_SLICE:      BEGIN
                    -- SLICE ASSIGNMENT:

                    ARR42(  -1..INTEGER'LAST  ) :=
                         ARR41(
                            -2..INTEGER'LAST-1) ;

     CHK_SLICE:     BEGIN
                         FOR  I  IN  -1..2  LOOP

                              IF  ARR42( I )  /=  FALSE  AND  I /= 0
                              THEN
                                   raise Program_Error;
                              ELSIF  ARR42( I ) /= TRUE  AND  I  = 0
                              THEN
                                   raise Program_Error;
                              END IF;

                         END LOOP;

                         IF  ARR42( -2 )  /=  TRUE
                         THEN
                            raise Program_Error;
                         END IF;

                    END CHK_SLICE;

               END DO_SLICE;

          END OBJ_DCL;

          END DCL_ARR;

     END CONSTR_ERR;


END C52103X;

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Richard Guenther]

On Sat, May 14, 2011 at 6:14 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Those issues should be fixed by the attached patch, which relaxes
>> strictness of logical operations in tree-cfg.c file.
>
> Thanks.
>
>> 2011-05-14  Kai Tietz
>>
>>         * tree-cfg.c (verify_gimple_assign_unary): Don't enforce
>> boolean_type_node
>>         compatible lhs/rhs types for logical expression.
>>         (verify_gimple_assign_binary): Likewise.
>
> -       /* We allow only boolean typed or compatible argument and result.  */
> -       if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
> -           || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
> -           || !useless_type_conversion_p (boolean_type_node,  lhs_type))
> +       /* The gimplify code ensures that just boolean_type_node types
> +          are present, but ADA and FORTRAN using artificial gimple generation
> +          code which is producing non-gimplify compatible trees.  So we need
> +          to allow here any kind of integral typed argument and result.  */
> +       if (!INTEGRAL_TYPE_P (rhs1_type)
> +           || !INTEGRAL_TYPE_P (rhs2_type)
> +           || !INTEGRAL_TYPE_P (lhs_type))
>
> What does "artificial gimple generation code" mean exactly?  The only thing
> different in Ada is the definition of boolean_type_node which isn't compatible
> with the C definition (its precision is 8 instead of 1).  That's all.
>
> For Ada, you can just do:
>
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 173756)
> +++ tree-cfg.c  (working copy)
> @@ -3350,7 +3350,7 @@ verify_gimple_assign_unary (gimple stmt)
>       return false;
>
>     case TRUTH_NOT_EXPR:
> -      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type))
> +      if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE)
>         {
>            error ("invalid types in truth not");
>            debug_generic_expr (lhs_type);
> @@ -3558,10 +3558,10 @@ do_pointer_plus_expr_check:
>     case TRUTH_OR_EXPR:
>     case TRUTH_XOR_EXPR:
>       {
> -       /* We allow only boolean typed or compatible argument and result.  */
> -       if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
> -           || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
> -           || !useless_type_conversion_p (boolean_type_node,  lhs_type))
> +       /* We allow only boolean-typed argument and result.  */
> +       if (TREE_CODE (rhs1_type) != BOOLEAN_TYPE
> +           || TREE_CODE (rhs2_type) != BOOLEAN_TYPE
> +           || TREE_CODE (lhs_type) != BOOLEAN_TYPE)
>          {
>            error ("type mismatch in binary truth expression");
>            debug_generic_expr (lhs_type);

We can't use a test for BOOLEAN_TYPE as the middle-end considers
a INTEGER_TYPE with same precision/signedness as compatible
and thus may propagate a variable of INTEGER_TYPE there.  I don't
understand why promoting bools to boolean_type_node during
gimplification does not work (it might be slightly undesirable as it
might introduce conversions from one boolean type to another).

So, to relax the above check to allow any of the boolean types
we'd need a way to enumerate them.  Or make conversions to/from
BOOLEAN_TYPEs not useless.

Kai - your testing should have catched the Ada testsuite fail, please
inverstigate why your setup didn't catch it.

Richard.

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Michael Matz]

Hi,

On Mon, 16 May 2011, Richard Guenther wrote:

> We can't use a test for BOOLEAN_TYPE as the middle-end considers a 
> INTEGER_TYPE with same precision/signedness as compatible and thus may 
> propagate a variable of INTEGER_TYPE there.  I don't understand why 
> promoting bools to boolean_type_node during gimplification does not work 
> (it might be slightly undesirable as it might introduce conversions from 
> one boolean type to another).
> 
> So, to relax the above check to allow any of the boolean types we'd need 
> a way to enumerate them.  Or make conversions to/from BOOLEAN_TYPEs not 
> useless.

I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the grounds 
that it requires booleanization (at least conceptually), i.e. conversion 
to a set of two values (no matter the precision or size) based on the 
outcome of comparing the RHS value with false_pre_image(TREE_TYPE(RHS)).

Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the 
conversions from false or true into false_pre_image or true_pre_image 
always is simply an embedding of 0 or 1/-1 (depending on target type 
signedness).  And if the BOOLEAN_TYPE and the LHS have same signedness the 
bit representation of boolean_true_type is (or should be) the same as the 
one converted to LHS (namely either 1 or -1).


Ciao,
Michael.

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Richard Guenther]

On Mon, May 16, 2011 at 3:17 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Mon, 16 May 2011, Richard Guenther wrote:
>
>> We can't use a test for BOOLEAN_TYPE as the middle-end considers a
>> INTEGER_TYPE with same precision/signedness as compatible and thus may
>> propagate a variable of INTEGER_TYPE there.  I don't understand why
>> promoting bools to boolean_type_node during gimplification does not work
>> (it might be slightly undesirable as it might introduce conversions from
>> one boolean type to another).
>>
>> So, to relax the above check to allow any of the boolean types we'd need
>> a way to enumerate them.  Or make conversions to/from BOOLEAN_TYPEs not
>> useless.
>
> I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the grounds
> that it requires booleanization (at least conceptually), i.e. conversion
> to a set of two values (no matter the precision or size) based on the
> outcome of comparing the RHS value with false_pre_image(TREE_TYPE(RHS)).
>
> Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the
> conversions from false or true into false_pre_image or true_pre_image
> always is simply an embedding of 0 or 1/-1 (depending on target type
> signedness).  And if the BOOLEAN_TYPE and the LHS have same signedness the
> bit representation of boolean_true_type is (or should be) the same as the
> one converted to LHS (namely either 1 or -1).

Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs
be used where BOOLEAN_TYPE nodes were used before.  It still will
cause an artificial conversion from a single-bit bitfield read to a bool.

Richard.

>
> Ciao,
> Michael.
>

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Michael Matz]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1371 bytes --]

Hi,

On Mon, 16 May 2011, Richard Guenther wrote:

> > I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the 
> > grounds that it requires booleanization (at least conceptually), i.e. 
> > conversion to a set of two values (no matter the precision or size) 
> > based on the outcome of comparing the RHS value with 
> > false_pre_image(TREE_TYPE(RHS)).
> >
> > Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the 
> > conversions from false or true into false_pre_image or true_pre_image 
> > always is simply an embedding of 0 or 1/-1 (depending on target type 
> > signedness).  And if the BOOLEAN_TYPE and the LHS have same signedness 
> > the bit representation of boolean_true_type is (or should be) the same 
> > as the one converted to LHS (namely either 1 or -1).
> 
> Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs be used 
> where BOOLEAN_TYPE nodes were used before.  It still will cause an 
> artificial conversion from a single-bit bitfield read to a bool.

Not if you're special casing single-bit conversions (on the grounds that a 
booleanization from two-valued set to a different two-valued set of 
the same signedness will not actually require a comparison).  I think it's 
better to be very precise in our base predicates than to add various hacks 
over the place to care for imprecision.


Ciao,
Michael.

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Richard Guenther]

On Mon, May 16, 2011 at 3:45 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Mon, 16 May 2011, Richard Guenther wrote:
>
>> > I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the
>> > grounds that it requires booleanization (at least conceptually), i.e.
>> > conversion to a set of two values (no matter the precision or size)
>> > based on the outcome of comparing the RHS value with
>> > false_pre_image(TREE_TYPE(RHS)).
>> >
>> > Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the
>> > conversions from false or true into false_pre_image or true_pre_image
>> > always is simply an embedding of 0 or 1/-1 (depending on target type
>> > signedness).  And if the BOOLEAN_TYPE and the LHS have same signedness
>> > the bit representation of boolean_true_type is (or should be) the same
>> > as the one converted to LHS (namely either 1 or -1).
>>
>> Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs be used
>> where BOOLEAN_TYPE nodes were used before.  It still will cause an
>> artificial conversion from a single-bit bitfield read to a bool.
>
> Not if you're special casing single-bit conversions (on the grounds that a
> booleanization from two-valued set to a different two-valued set of
> the same signedness will not actually require a comparison).  I think it's
> better to be very precise in our base predicates than to add various hacks
> over the place to care for imprecision.

Or require a 1-bit integral type for TRUTH_* operands only (which ensures
the two-valueness which is what we really want).  That can be done
by either fixing the frontends to make boolean_type_node have 1-bit
precision or to build a middle-end private type with that constraints
(though that's the more difficult route as we still do not have a strong
FE - middle-end hand-off point, and it certainly is not the gimplifier).

Long term all the global trees should be FE private and the middle-end
should have its own set.

Richard.

>
> Ciao,
> Michael.

Re: [patch gimplifier]: Make sure TRUTH_NOT_EXPR has boolean_type_node type and argument

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

2011/5/16 Richard Guenther <richard.guenther@gmail.com>:
> On Mon, May 16, 2011 at 3:45 PM, Michael Matz <matz@suse.de> wrote:
>> Hi,
>>
>> On Mon, 16 May 2011, Richard Guenther wrote:
>>
>>> > I think conversion _to_ BOOLEAN_TYPE shouldn't be useless, on the
>>> > grounds that it requires booleanization (at least conceptually), i.e.
>>> > conversion to a set of two values (no matter the precision or size)
>>> > based on the outcome of comparing the RHS value with
>>> > false_pre_image(TREE_TYPE(RHS)).
>>> >
>>> > Conversion _from_ BOOLEAN_TYPE can be regarded as useless, as the
>>> > conversions from false or true into false_pre_image or true_pre_image
>>> > always is simply an embedding of 0 or 1/-1 (depending on target type
>>> > signedness).  And if the BOOLEAN_TYPE and the LHS have same signedness
>>> > the bit representation of boolean_true_type is (or should be) the same
>>> > as the one converted to LHS (namely either 1 or -1).
>>>
>>> Sure, that would probably be enough to prevent non-BOOLEAN_TYPEs be used
>>> where BOOLEAN_TYPE nodes were used before.  It still will cause an
>>> artificial conversion from a single-bit bitfield read to a bool.
>>
>> Not if you're special casing single-bit conversions (on the grounds that a
>> booleanization from two-valued set to a different two-valued set of
>> the same signedness will not actually require a comparison).  I think it's
>> better to be very precise in our base predicates than to add various hacks
>> over the place to care for imprecision.
>
> Or require a 1-bit integral type for TRUTH_* operands only (which ensures
> the two-valueness which is what we really want).  That can be done
> by either fixing the frontends to make boolean_type_node have 1-bit
> precision or to build a middle-end private type with that constraints
> (though that's the more difficult route as we still do not have a strong
> FE - middle-end hand-off point, and it certainly is not the gimplifier).
>
> Long term all the global trees should be FE private and the middle-end
> should have its own set.
>
> Richard.
>
>>
>> Ciao,
>> Michael.
>

Hello,

initial idea was to check for logical operations that the conversion
to boolean_type_node
is useless.  This assumption was flawed by the fact that
boolean_type_node gets re-defined
in free_lang_decl to a 1-bit precision BOOL_TYPE_SIZE-ed type, if FE's
boolean_type_node
is incompatible to this.  By this FE's boolean_type_node gets via the
back-door incompatible
in tree-cfg checks.
So for all languages - but ADA - logical types have precision set to
one. Just for ADA case,
which requires a different boolean_type_node kind, we need to inspect
the inner type to be
a boolean.  As Fortran has also integer typed boolean compatible
types, we can't simply check
for BOOLEAN_TYPE here and need to check for precision first.

ChangeLog

2011-05-18  Kai Tietz

	PR middle-end/48989
	* tree-cfg.c (verify_gimple_assign_binary): Check lhs type
	for being compatible to boolean for logical operations.
	(verify_gimple_assign_unary): Likewise.
	(compatible_boolean_type_p): New helper.

Bootstrapped on x86_64-pc-linux-gnu. And regression tested for ADA
and Fortran.

Ok for apply?

Regards,
Kai

[-- Attachment #2: relax_tree_cfg.txt --]
[-- Type: text/plain, Size: 2589 bytes --]

Index: gcc/gcc/tree-cfg.c
===================================================================
--- gcc.orig/gcc/tree-cfg.c	2011-05-16 14:26:12.369031500 +0200
+++ gcc/gcc/tree-cfg.c	2011-05-18 08:20:34.935819100 +0200
@@ -3220,6 +3220,31 @@ verify_gimple_comparison (tree type, tre
   return false;
 }
 
+/* Checks TYPE for being compatible to boolean. Returns
+   FALSE, if type is not compatible, otherwise TRUE.
+
+   A type is compatible if
+   a) TYPE_PRECISION is one.
+   b) The type - or the inner type - is of kind BOOLEAN_TYPE.  */
+
+static bool
+compatible_boolean_type_p (tree type)
+{
+  if (!type)
+    return false;
+  if (TYPE_PRECISION (type) == 1)
+    return true;
+
+  /* We try to look here into inner type, as ADA uses
+     boolean_type_node with type precision != 1.  */
+  while (TREE_TYPE (type)
+	 && (TREE_CODE (type) == INTEGER_TYPE
+	     || TREE_CODE (type) == REAL_TYPE))
+    type = TREE_TYPE (type);
+
+  return TYPE_PRECISION (type) == 1 || TREE_CODE (type) == BOOLEAN_TYPE;
+}
+
 /* Verify a gimple assignment statement STMT with an unary rhs.
    Returns true if anything is wrong.  */
 
@@ -3350,15 +3375,16 @@ verify_gimple_assign_unary (gimple stmt)
       return false;
 
     case TRUTH_NOT_EXPR:
-      if (!useless_type_conversion_p (boolean_type_node,  rhs1_type))
+
+      if (!useless_type_conversion_p (lhs_type,  rhs1_type)
+          || !compatible_boolean_type_p (lhs_type))
         {
-	    error ("invalid types in truth not");
-	    debug_generic_expr (lhs_type);
-	    debug_generic_expr (rhs1_type);
-	    return true;
+           error ("invalid types in truth not");
+           debug_generic_expr (lhs_type);
+           debug_generic_expr (rhs1_type);
+           return true;
         }
       break;
-
     case NEGATE_EXPR:
     case ABS_EXPR:
     case BIT_NOT_EXPR:
@@ -3558,10 +3584,11 @@ do_pointer_plus_expr_check:
     case TRUTH_OR_EXPR:
     case TRUTH_XOR_EXPR:
       {
-	/* We allow only boolean typed or compatible argument and result.  */
-	if (!useless_type_conversion_p (boolean_type_node,  rhs1_type)
-	    || !useless_type_conversion_p (boolean_type_node,  rhs2_type)
-	    || !useless_type_conversion_p (boolean_type_node,  lhs_type))
+	/* We allow only boolean_typed or types with precision of one,
+	   or compatible argument and result.  */
+	if (!useless_type_conversion_p (lhs_type,  rhs1_type)
+	    || !useless_type_conversion_p (lhs_type,  rhs2_type)
+	    || !compatible_boolean_type_p (lhs_type))
 	  {
 	    error ("type mismatch in binary truth expression");
 	    debug_generic_expr (lhs_type);