Signedness of boolean types (and Ada)

Signedness of boolean types (and Ada)

[Andrew Pinski]

I noticed while debugging why my "A?CST1:CST0" patch was broken for
Ada, I noticed the following ranges for boolean types:
  # RANGE [0, 1] NONZERO 1
  _14 = checks__saved_checks_tos.482_2 > 0;
  # RANGE [0, 255] NONZERO 1
  _18 = _14 == 0;
  _19 = ~_18;
  # RANGE [0, 1] NONZERO 1
  _15 = _19;
  if (_15 != 0)
    goto <bb 7>; [50.00%]
  else
    goto <bb 6>; [50.00%]

Seems like there is a misunderstanding of TYPE_UNSIGNED for boolean
types and that is what is causing the problem in the end. As the above
gets optimized to be always true. My patch just happens to cause the
~_18 to be produced which is later on miscompiled.

Should TYPE_UNSIGNED be always set for boolean types?

I am testing the below patch to see if it fixes the problem, if we
should assume TYPE_UNSIGNED is true for boolean types.  If we should
not assume that, then there is a problem with conversion between
boolean types that have TYPE_UNSIGNED mismatched.

Thanks,
Andrew Pinski

diff --git a/gcc/ada/gcc-interface/decl.c b/gcc/ada/gcc-interface/decl.c
index 232b552a60c..4e839f3b8ab 100644
--- a/gcc/ada/gcc-interface/decl.c
+++ b/gcc/ada/gcc-interface/decl.c
@@ -1687,7 +1687,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entity, tree
gnu_expr, bool definition)

          gnu_type = make_node (is_boolean ? BOOLEAN_TYPE : ENUMERAL_TYPE);
          TYPE_PRECISION (gnu_type) = esize;
-         TYPE_UNSIGNED (gnu_type) = is_unsigned;
+         TYPE_UNSIGNED (gnu_type) = is_boolean ? true : is_unsigned;
          set_min_and_max_values_for_integral_type (gnu_type, esize,
                                                    TYPE_SIGN (gnu_type));
          process_attributes (&gnu_type, &attr_list, true, gnat_entity);

Re: Signedness of boolean types (and Ada)

[Andrew Pinski]

On Mon, May 17, 2021 at 6:52 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> I noticed while debugging why my "A?CST1:CST0" patch was broken for
> Ada, I noticed the following ranges for boolean types:
>   # RANGE [0, 1] NONZERO 1
>   _14 = checks__saved_checks_tos.482_2 > 0;
>   # RANGE [0, 255] NONZERO 1
>   _18 = _14 == 0;
>   _19 = ~_18;
>   # RANGE [0, 1] NONZERO 1
>   _15 = _19;
>   if (_15 != 0)
>     goto <bb 7>; [50.00%]
>   else
>     goto <bb 6>; [50.00%]
>
> Seems like there is a misunderstanding of TYPE_UNSIGNED for boolean
> types and that is what is causing the problem in the end. As the above
> gets optimized to be always true. My patch just happens to cause the
> ~_18 to be produced which is later on miscompiled.
>
> Should TYPE_UNSIGNED be always set for boolean types?
>
> I am testing the below patch to see if it fixes the problem, if we
> should assume TYPE_UNSIGNED is true for boolean types.  If we should
> not assume that, then there is a problem with conversion between
> boolean types that have TYPE_UNSIGNED mismatched.

I went back and noticed this has been discussed before but it does
look like the middle-end/gimple is still broken and even worse I have
done:
(bit_not:boolean_type_node (convert:boolean_type_node @0))
Which means I have done the conversions correctly but they are removed still.
So I am still thinking we need to have a discussion about what boolean
type node should be and even how boolean types should act during the
gimple phase.

Thanks,
Andrew Pinski

>
> Thanks,
> Andrew Pinski
>
> diff --git a/gcc/ada/gcc-interface/decl.c b/gcc/ada/gcc-interface/decl.c
> index 232b552a60c..4e839f3b8ab 100644
> --- a/gcc/ada/gcc-interface/decl.c
> +++ b/gcc/ada/gcc-interface/decl.c
> @@ -1687,7 +1687,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entity, tree
> gnu_expr, bool definition)
>
>           gnu_type = make_node (is_boolean ? BOOLEAN_TYPE : ENUMERAL_TYPE);
>           TYPE_PRECISION (gnu_type) = esize;
> -         TYPE_UNSIGNED (gnu_type) = is_unsigned;
> +         TYPE_UNSIGNED (gnu_type) = is_boolean ? true : is_unsigned;
>           set_min_and_max_values_for_integral_type (gnu_type, esize,
>                                                     TYPE_SIGN (gnu_type));
>           process_attributes (&gnu_type, &attr_list, true, gnat_entity);

Re: Signedness of boolean types (and Ada)

[Richard Biener]

On Tue, May 18, 2021 at 4:51 AM Andrew Pinski via Gcc <gcc@gcc.gnu.org> wrote:
>
> On Mon, May 17, 2021 at 6:52 PM Andrew Pinski <pinskia@gmail.com> wrote:
> >
> > I noticed while debugging why my "A?CST1:CST0" patch was broken for
> > Ada, I noticed the following ranges for boolean types:
> >   # RANGE [0, 1] NONZERO 1
> >   _14 = checks__saved_checks_tos.482_2 > 0;
> >   # RANGE [0, 255] NONZERO 1
> >   _18 = _14 == 0;
> >   _19 = ~_18;
> >   # RANGE [0, 1] NONZERO 1
> >   _15 = _19;
> >   if (_15 != 0)
> >     goto <bb 7>; [50.00%]
> >   else
> >     goto <bb 6>; [50.00%]
> >
> > Seems like there is a misunderstanding of TYPE_UNSIGNED for boolean
> > types and that is what is causing the problem in the end. As the above
> > gets optimized to be always true. My patch just happens to cause the
> > ~_18 to be produced which is later on miscompiled.
> >
> > Should TYPE_UNSIGNED be always set for boolean types?
> >
> > I am testing the below patch to see if it fixes the problem, if we
> > should assume TYPE_UNSIGNED is true for boolean types.  If we should
> > not assume that, then there is a problem with conversion between
> > boolean types that have TYPE_UNSIGNED mismatched.
>
> I went back and noticed this has been discussed before but it does
> look like the middle-end/gimple is still broken and even worse I have
> done:
> (bit_not:boolean_type_node (convert:boolean_type_node @0))
> Which means I have done the conversions correctly but they are removed still.
> So I am still thinking we need to have a discussion about what boolean
> type node should be and even how boolean types should act during the
> gimple phase.

Note Adas boolean_type_node has precision 8 (but is unsigned).  I don't
think BOOLEAN_TYPEs are in any way restricted in precision or sign,
they are required to be two-valued though.  What kind of conversions
are elided?  Maybe the issue is in range analysis assuming comparisons
result in unsigned boolean values?  Because for GIMPLE we only
require they produce a BOOLEAN_TYPE or a 1-bit precision integral type
(see tree-cfg.c:verify_gimple_comparison).

That said, I suppose you need to track down where things go wrong.

Richard.

> Thanks,
> Andrew Pinski
>
> >
> > Thanks,
> > Andrew Pinski
> >
> > diff --git a/gcc/ada/gcc-interface/decl.c b/gcc/ada/gcc-interface/decl.c
> > index 232b552a60c..4e839f3b8ab 100644
> > --- a/gcc/ada/gcc-interface/decl.c
> > +++ b/gcc/ada/gcc-interface/decl.c
> > @@ -1687,7 +1687,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entity, tree
> > gnu_expr, bool definition)
> >
> >           gnu_type = make_node (is_boolean ? BOOLEAN_TYPE : ENUMERAL_TYPE);
> >           TYPE_PRECISION (gnu_type) = esize;
> > -         TYPE_UNSIGNED (gnu_type) = is_unsigned;
> > +         TYPE_UNSIGNED (gnu_type) = is_boolean ? true : is_unsigned;
> >           set_min_and_max_values_for_integral_type (gnu_type, esize,
> >                                                     TYPE_SIGN (gnu_type));
> >           process_attributes (&gnu_type, &attr_list, true, gnat_entity);

Re: Signedness of boolean types (and Ada)

[Eric Botcazou]

> I noticed while debugging why my "A?CST1:CST0" patch was broken for
> Ada, I noticed the following ranges for boolean types:
>   # RANGE [0, 1] NONZERO 1
>   _14 = checks__saved_checks_tos.482_2 > 0;
>   # RANGE [0, 255] NONZERO 1
>   _18 = _14 == 0;
>   _19 = ~_18;

The '~' looks problematic if this is for Ada code, it ought to be:

  _19 = _18 ^ 1;

See below.

> Should TYPE_UNSIGNED be always set for boolean types?
> 
> I am testing the below patch to see if it fixes the problem, if we
> should assume TYPE_UNSIGNED is true for boolean types.  If we should
> not assume that, then there is a problem with conversion between
> boolean types that have TYPE_UNSIGNED mismatched.

The patch is a nop, boolean types are always unsigned in Ada, see cstand.adb:

      Set_Is_Unsigned_Type (Standard_Boolean);

but they have 8-bit precision instead of 1-bit precision so must be handled 
with care, in particular BIT_NOT_EXPR should be avoided for them, see e.g. the 
gimplifier:

	    if (TYPE_PRECISION (TREE_TYPE (*expr_p)) == 1)
	      *expr_p = build1_loc (input_location, BIT_NOT_EXPR,
				    TREE_TYPE (*expr_p),
				    TREE_OPERAND (*expr_p, 0));
	    else
	      *expr_p = build2_loc (input_location, BIT_XOR_EXPR,
				    TREE_TYPE (*expr_p),
				    TREE_OPERAND (*expr_p, 0),
				    build_int_cst (TREE_TYPE 
(*expr_p), 1));

-- 
Eric Botcazou

Re: Signedness of boolean types (and Ada)

[Richard Biener]

On Tue, May 18, 2021 at 11:41 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > I noticed while debugging why my "A?CST1:CST0" patch was broken for
> > Ada, I noticed the following ranges for boolean types:
> >   # RANGE [0, 1] NONZERO 1
> >   _14 = checks__saved_checks_tos.482_2 > 0;
> >   # RANGE [0, 255] NONZERO 1
> >   _18 = _14 == 0;
> >   _19 = ~_18;
>
> The '~' looks problematic if this is for Ada code, it ought to be:
>
>   _19 = _18 ^ 1;
>
> See below.

So we do in fact require that all BOOLEAN_TYPEs are unsigned and that
the two (valid) values it represents are 0 and 1?  Because with a signed
boolean and 0 / -1 the ^ 1 operation would be wrong.

I suppose it should be ^ constant_boolean_node (type) which also
works for singed bools.  vector bools are signed after all ...

> > Should TYPE_UNSIGNED be always set for boolean types?
> >
> > I am testing the below patch to see if it fixes the problem, if we
> > should assume TYPE_UNSIGNED is true for boolean types.  If we should
> > not assume that, then there is a problem with conversion between
> > boolean types that have TYPE_UNSIGNED mismatched.
>
> The patch is a nop, boolean types are always unsigned in Ada, see cstand.adb:
>
>       Set_Is_Unsigned_Type (Standard_Boolean);
>
> but they have 8-bit precision instead of 1-bit precision so must be handled
> with care, in particular BIT_NOT_EXPR should be avoided for them, see e.g. the
> gimplifier:
>
>             if (TYPE_PRECISION (TREE_TYPE (*expr_p)) == 1)
>               *expr_p = build1_loc (input_location, BIT_NOT_EXPR,
>                                     TREE_TYPE (*expr_p),
>                                     TREE_OPERAND (*expr_p, 0));
>             else
>               *expr_p = build2_loc (input_location, BIT_XOR_EXPR,
>                                     TREE_TYPE (*expr_p),
>                                     TREE_OPERAND (*expr_p, 0),
>                                     build_int_cst (TREE_TYPE
> (*expr_p), 1));
>
> --
> Eric Botcazou
>
>