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

login
register
mail settings
Submitter Kai Tietz
Date May 13, 2011, 11:31 a.m.
Message ID <BANLkTimCEzBM4KAzoOHFL4=39YzLU996-A@mail.gmail.com>
Download mbox | patch
Permalink /patch/95462/
State New
Headers show

Comments

Kai Tietz - May 13, 2011, 11:31 a.m.
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?
Richard Guenther - May 13, 2011, 12:43 p.m.
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);
>
Kai Tietz - May 13, 2011, 1:38 p.m.
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
H.J. Lu - May 13, 2011, 4 p.m.
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
Andrew Pinski - May 13, 2011, 4:06 p.m.
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
Kai Tietz - May 13, 2011, 4:28 p.m.
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
Kai Tietz - May 13, 2011, 6:24 p.m.
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
Andrew Pinski - May 13, 2011, 6:30 p.m.
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
Eric Botcazou - May 13, 2011, 10:26 p.m.
> 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

Patch

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);