diff mbox

Fix PHI optimization issue with boolean types

Message ID 10860828.opkyFOrRK6@polaris
State Superseded
Headers show

Commit Message

Eric Botcazou Oct. 18, 2016, 6:35 a.m. UTC
Hi,

this is a regression present on the mainline and 6 branch: the compiler now 
generates wrong code for the attached testcase at -O because of an internal 
conflict about boolean types.  The sequence is as follows.  In .mergephi3:

  boolean _22;
  p__enum res;

  <bb 9>:
  if (_22 != 0)
    goto <bb 10>;
  else
    goto <bb 11>;

  <bb 10>:

  <bb 11>:
  # res_17 = PHI <2(8), 0(9), 1(10)>

is turned into:

COND_EXPR in block 9 and PHI in block 11 converted to straightline code.
PHI res_17 changed to factor conversion out from COND_EXPR.
New stmt with CAST that defines res_17.

  boolean _33;

  <bb 10>:
  # _33 = PHI <2(8), _22(9)>
  res_17 = (p__enum) _33;

[...]

  <bb 12>:
  if (res_17 != 0)
    goto <bb 13>;
  else
    goto <bb 14>;

  <bb 13>:
  _12 = res_17 == 2;
  _13 = (integer) _12

in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3 propagates _33
into the uses of res_17:

  <bb 12>:
  if (_33 != 0)
    goto <bb 13>;
  else
    goto <bb 14>;

  <bb 13>:
  _12 = _33 == 2;
  _13 = (integer) _12;

and DOM3 deduces:

  <bb 13>:
  _12 = 0;
  _13 = 0;

because it computes that _33 has value 1 in BB 13 since it's a boolean.

The problem was introduced by the new factor_out_conditional_conversion:

      /* If arg1 is an INTEGER_CST, fold it to new type.  */
      if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
	  && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
	{
	  if (gimple_assign_cast_p (arg0_def_stmt))
	    new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
	  else
	    return NULL;
	}
      else
	return NULL

int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked
on constant 2 and a BOOLEAN_TYPE and returns true.

BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so the
outcome of int_fits_type_p is not unreasonable.  But this goes against the
various transformations applied to boolean types in the compiler, which all
assume that they can only take values 0 or 1.

Hence the attached fix (which should be a no-op except for Ada), tested on 
x86_64-suse-linux, OK for mainline and 6 branch?


2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

	* tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.


2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt59.adb: New test.
	* gnat.dg/opt59_pkg.ad[sb]: New helper.

-- 
Eric Botcazou

Comments

Richard Biener Oct. 18, 2016, 8:35 a.m. UTC | #1
On Tue, Oct 18, 2016 at 8:35 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,

>

> this is a regression present on the mainline and 6 branch: the compiler now

> generates wrong code for the attached testcase at -O because of an internal

> conflict about boolean types.  The sequence is as follows.  In .mergephi3:

>

>   boolean _22;

>   p__enum res;

>

>   <bb 9>:

>   if (_22 != 0)

>     goto <bb 10>;

>   else

>     goto <bb 11>;

>

>   <bb 10>:

>

>   <bb 11>:

>   # res_17 = PHI <2(8), 0(9), 1(10)>

>

> is turned into:

>

> COND_EXPR in block 9 and PHI in block 11 converted to straightline code.

> PHI res_17 changed to factor conversion out from COND_EXPR.

> New stmt with CAST that defines res_17.

>

>   boolean _33;

>

>   <bb 10>:

>   # _33 = PHI <2(8), _22(9)>

>   res_17 = (p__enum) _33;

>

> [...]

>

>   <bb 12>:

>   if (res_17 != 0)

>     goto <bb 13>;

>   else

>     goto <bb 14>;

>

>   <bb 13>:

>   _12 = res_17 == 2;

>   _13 = (integer) _12

>

> in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3 propagates _33

> into the uses of res_17:

>

>   <bb 12>:

>   if (_33 != 0)

>     goto <bb 13>;

>   else

>     goto <bb 14>;

>

>   <bb 13>:

>   _12 = _33 == 2;

>   _13 = (integer) _12;

>

> and DOM3 deduces:

>

>   <bb 13>:

>   _12 = 0;

>   _13 = 0;

>

> because it computes that _33 has value 1 in BB 13 since it's a boolean.

>

> The problem was introduced by the new factor_out_conditional_conversion:

>

>       /* If arg1 is an INTEGER_CST, fold it to new type.  */

>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))

>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))

>         {

>           if (gimple_assign_cast_p (arg0_def_stmt))

>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);

>           else

>             return NULL;

>         }

>       else

>         return NULL

>

> int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked

> on constant 2 and a BOOLEAN_TYPE and returns true.

>

> BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so the

> outcome of int_fits_type_p is not unreasonable.  But this goes against the

> various transformations applied to boolean types in the compiler, which all

> assume that they can only take values 0 or 1.

>

> Hence the attached fix (which should be a no-op except for Ada), tested on

> x86_64-suse-linux, OK for mainline and 6 branch?


Hmm, I wonder if the patch is a good approach as you are likely only papering
over a single of possibly very many affected places (wi::fits_to_tree_p comes
immediately to my mind).  I suppose a better way would be for Ada to not
lie about those types and not use BOOLEAN_TYPE but INTEGER_TYPE.
Because BOOLEAN_TYPE types only have two values as documented in
tree.def:

/* Boolean type (true or false are the only values).  Looks like an
   INTEGRAL_TYPE.  */
DEFTREECODE (BOOLEAN_TYPE, "boolean_type", tcc_type, 0)

There are not many references to BOOLEAN_TYPE in ada/gcc-interface
thus it shouldn't be hard to change ... (looks like Ada might even prefer
ENUMERAL_TYPE here).

Thanks,
Richard.

>

> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.

>

>

> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * gnat.dg/opt59.adb: New test.

>         * gnat.dg/opt59_pkg.ad[sb]: New helper.

>

> --

> Eric Botcazou
Jeff Law Oct. 18, 2016, 2:10 p.m. UTC | #2
On 10/18/2016 02:35 AM, Richard Biener wrote:
> On Tue, Oct 18, 2016 at 8:35 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:

>> Hi,

>>

>> this is a regression present on the mainline and 6 branch: the compiler now

>> generates wrong code for the attached testcase at -O because of an internal

>> conflict about boolean types.  The sequence is as follows.  In .mergephi3:

>>

>>   boolean _22;

>>   p__enum res;

>>

>>   <bb 9>:

>>   if (_22 != 0)

>>     goto <bb 10>;

>>   else

>>     goto <bb 11>;

>>

>>   <bb 10>:

>>

>>   <bb 11>:

>>   # res_17 = PHI <2(8), 0(9), 1(10)>

>>

>> is turned into:

>>

>> COND_EXPR in block 9 and PHI in block 11 converted to straightline code.

>> PHI res_17 changed to factor conversion out from COND_EXPR.

>> New stmt with CAST that defines res_17.

>>

>>   boolean _33;

>>

>>   <bb 10>:

>>   # _33 = PHI <2(8), _22(9)>

>>   res_17 = (p__enum) _33;

>>

>> [...]

>>

>>   <bb 12>:

>>   if (res_17 != 0)

>>     goto <bb 13>;

>>   else

>>     goto <bb 14>;

>>

>>   <bb 13>:

>>   _12 = res_17 == 2;

>>   _13 = (integer) _12

>>

>> in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3 propagates _33

>> into the uses of res_17:

>>

>>   <bb 12>:

>>   if (_33 != 0)

>>     goto <bb 13>;

>>   else

>>     goto <bb 14>;

>>

>>   <bb 13>:

>>   _12 = _33 == 2;

>>   _13 = (integer) _12;

>>

>> and DOM3 deduces:

>>

>>   <bb 13>:

>>   _12 = 0;

>>   _13 = 0;

>>

>> because it computes that _33 has value 1 in BB 13 since it's a boolean.

>>

>> The problem was introduced by the new factor_out_conditional_conversion:

>>

>>       /* If arg1 is an INTEGER_CST, fold it to new type.  */

>>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))

>>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))

>>         {

>>           if (gimple_assign_cast_p (arg0_def_stmt))

>>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);

>>           else

>>             return NULL;

>>         }

>>       else

>>         return NULL

>>

>> int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked

>> on constant 2 and a BOOLEAN_TYPE and returns true.

>>

>> BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so the

>> outcome of int_fits_type_p is not unreasonable.  But this goes against the

>> various transformations applied to boolean types in the compiler, which all

>> assume that they can only take values 0 or 1.

>>

>> Hence the attached fix (which should be a no-op except for Ada), tested on

>> x86_64-suse-linux, OK for mainline and 6 branch?

>

> Hmm, I wonder if the patch is a good approach as you are likely only papering

> over a single of possibly very many affected places (wi::fits_to_tree_p comes

> immediately to my mind).  I suppose a better way would be for Ada to not

> lie about those types and not use BOOLEAN_TYPE but INTEGER_TYPE.

> Because BOOLEAN_TYPE types only have two values as documented in

> tree.def:

>

> /* Boolean type (true or false are the only values).  Looks like an

>    INTEGRAL_TYPE.  */

> DEFTREECODE (BOOLEAN_TYPE, "boolean_type", tcc_type, 0)

>

> There are not many references to BOOLEAN_TYPE in ada/gcc-interface

> thus it shouldn't be hard to change ... (looks like Ada might even prefer

> ENUMERAL_TYPE here).

>

> Thanks,

> Richard.

>

>>

>> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

>>

>>         * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.

Alternately we could check the precision/range in the optimizers.  We do 
that in various places because of this exact issue, I could well have 
missed one or more.

Though I would have a preference for fixing int_fits_type_p which seems 
like it'll catch the cases we care about without having to twiddle every 
optimizer.

jeff
Richard Biener Oct. 19, 2016, 10:05 a.m. UTC | #3
On Tue, Oct 18, 2016 at 4:10 PM, Jeff Law <law@redhat.com> wrote:
> On 10/18/2016 02:35 AM, Richard Biener wrote:

>>

>> On Tue, Oct 18, 2016 at 8:35 AM, Eric Botcazou <ebotcazou@adacore.com>

>> wrote:

>>>

>>> Hi,

>>>

>>> this is a regression present on the mainline and 6 branch: the compiler

>>> now

>>> generates wrong code for the attached testcase at -O because of an

>>> internal

>>> conflict about boolean types.  The sequence is as follows.  In

>>> .mergephi3:

>>>

>>>   boolean _22;

>>>   p__enum res;

>>>

>>>   <bb 9>:

>>>   if (_22 != 0)

>>>     goto <bb 10>;

>>>   else

>>>     goto <bb 11>;

>>>

>>>   <bb 10>:

>>>

>>>   <bb 11>:

>>>   # res_17 = PHI <2(8), 0(9), 1(10)>

>>>

>>> is turned into:

>>>

>>> COND_EXPR in block 9 and PHI in block 11 converted to straightline code.

>>> PHI res_17 changed to factor conversion out from COND_EXPR.

>>> New stmt with CAST that defines res_17.

>>>

>>>   boolean _33;

>>>

>>>   <bb 10>:

>>>   # _33 = PHI <2(8), _22(9)>

>>>   res_17 = (p__enum) _33;

>>>

>>> [...]

>>>

>>>   <bb 12>:

>>>   if (res_17 != 0)

>>>     goto <bb 13>;

>>>   else

>>>     goto <bb 14>;

>>>

>>>   <bb 13>:

>>>   _12 = res_17 == 2;

>>>   _13 = (integer) _12

>>>

>>> in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3

>>> propagates _33

>>> into the uses of res_17:

>>>

>>>   <bb 12>:

>>>   if (_33 != 0)

>>>     goto <bb 13>;

>>>   else

>>>     goto <bb 14>;

>>>

>>>   <bb 13>:

>>>   _12 = _33 == 2;

>>>   _13 = (integer) _12;

>>>

>>> and DOM3 deduces:

>>>

>>>   <bb 13>:

>>>   _12 = 0;

>>>   _13 = 0;

>>>

>>> because it computes that _33 has value 1 in BB 13 since it's a boolean.

>>>

>>> The problem was introduced by the new factor_out_conditional_conversion:

>>>

>>>       /* If arg1 is an INTEGER_CST, fold it to new type.  */

>>>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))

>>>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))

>>>         {

>>>           if (gimple_assign_cast_p (arg0_def_stmt))

>>>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);

>>>           else

>>>             return NULL;

>>>         }

>>>       else

>>>         return NULL

>>>

>>> int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked

>>> on constant 2 and a BOOLEAN_TYPE and returns true.

>>>

>>> BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so

>>> the

>>> outcome of int_fits_type_p is not unreasonable.  But this goes against

>>> the

>>> various transformations applied to boolean types in the compiler, which

>>> all

>>> assume that they can only take values 0 or 1.

>>>

>>> Hence the attached fix (which should be a no-op except for Ada), tested

>>> on

>>> x86_64-suse-linux, OK for mainline and 6 branch?

>>

>>

>> Hmm, I wonder if the patch is a good approach as you are likely only

>> papering

>> over a single of possibly very many affected places (wi::fits_to_tree_p

>> comes

>> immediately to my mind).  I suppose a better way would be for Ada to not

>> lie about those types and not use BOOLEAN_TYPE but INTEGER_TYPE.

>> Because BOOLEAN_TYPE types only have two values as documented in

>> tree.def:

>>

>> /* Boolean type (true or false are the only values).  Looks like an

>>    INTEGRAL_TYPE.  */

>> DEFTREECODE (BOOLEAN_TYPE, "boolean_type", tcc_type, 0)

>>

>> There are not many references to BOOLEAN_TYPE in ada/gcc-interface

>> thus it shouldn't be hard to change ... (looks like Ada might even prefer

>> ENUMERAL_TYPE here).

>>

>> Thanks,

>> Richard.

>>

>>>

>>> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

>>>

>>>         * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean

>>> types.

>

> Alternately we could check the precision/range in the optimizers.  We do

> that in various places because of this exact issue, I could well have missed

> one or more.


I don't think we do.  Instead we have various places that do

  if ((TREE_CODE (TREE_TYPE (rhs1)) == BOOLEAN_TYPE
       || (INTEGRAL_TYPE_P (TREE_TYPE (rhs1))
           && TYPE_PRECISION (TREE_TYPE (rhs1)) == 1))

which basically means BOOLEAN_TYPEs have two values only regardless of
their precision or mode.

> Though I would have a preference for fixing int_fits_type_p which seems like

> it'll catch the cases we care about without having to twiddle every

> optimizer.


It doesn't catch the above.  If BOOLEAN_TYPE is not special why do we have
it at all?

Richard.

> jeff

>
Eric Botcazou Oct. 19, 2016, 10:50 p.m. UTC | #4
> Because BOOLEAN_TYPE types only have two values as documented in

> tree.def:

> 

> /* Boolean type (true or false are the only values).  Looks like an

>    INTEGRAL_TYPE.  */

> DEFTREECODE (BOOLEAN_TYPE, "boolean_type", tcc_type, 0)


Yes, but on the other hand they have a precision and a range.

> There are not many references to BOOLEAN_TYPE in ada/gcc-interface

> thus it shouldn't be hard to change ... (looks like Ada might even prefer

> ENUMERAL_TYPE here).


It was like that originally (an ENUMERAL_TYPE) but this blocked LTO (well, 
cross-language LTO) exactly like the sizetype discrepancy, so I don't really 
feel like going back in time.

-- 
Eric Botcazou
Richard Biener Oct. 20, 2016, 9:36 a.m. UTC | #5
On Thu, Oct 20, 2016 at 12:50 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Because BOOLEAN_TYPE types only have two values as documented in

>> tree.def:

>>

>> /* Boolean type (true or false are the only values).  Looks like an

>>    INTEGRAL_TYPE.  */

>> DEFTREECODE (BOOLEAN_TYPE, "boolean_type", tcc_type, 0)

>

> Yes, but on the other hand they have a precision and a range.

>

>> There are not many references to BOOLEAN_TYPE in ada/gcc-interface

>> thus it shouldn't be hard to change ... (looks like Ada might even prefer

>> ENUMERAL_TYPE here).

>

> It was like that originally (an ENUMERAL_TYPE) but this blocked LTO (well,

> cross-language LTO) exactly like the sizetype discrepancy, so I don't really

> feel like going back in time.


Because boolean_type_node was different?  Well, are really _all_ boolean types
this way or can you keep using BOOLEAN_TYPE for boolean_type_node for
things like interfacing to C and C++ (and the builtins, etc.) and use
ENUMERAL_TYPE for the cases where bool can have non zero/one values?

Richard.

> --

> Eric Botcazou
Richard Biener Oct. 20, 2016, 9:47 a.m. UTC | #6
On Tue, Oct 18, 2016 at 8:35 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,

>

> this is a regression present on the mainline and 6 branch: the compiler now

> generates wrong code for the attached testcase at -O because of an internal

> conflict about boolean types.  The sequence is as follows.  In .mergephi3:

>

>   boolean _22;

>   p__enum res;

>

>   <bb 9>:

>   if (_22 != 0)

>     goto <bb 10>;

>   else

>     goto <bb 11>;

>

>   <bb 10>:

>

>   <bb 11>:

>   # res_17 = PHI <2(8), 0(9), 1(10)>

>

> is turned into:

>

> COND_EXPR in block 9 and PHI in block 11 converted to straightline code.

> PHI res_17 changed to factor conversion out from COND_EXPR.

> New stmt with CAST that defines res_17.

>

>   boolean _33;

>

>   <bb 10>:

>   # _33 = PHI <2(8), _22(9)>

>   res_17 = (p__enum) _33;

>

> [...]

>

>   <bb 12>:

>   if (res_17 != 0)

>     goto <bb 13>;

>   else

>     goto <bb 14>;

>

>   <bb 13>:

>   _12 = res_17 == 2;

>   _13 = (integer) _12

>

> in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3 propagates _33

> into the uses of res_17:

>

>   <bb 12>:

>   if (_33 != 0)

>     goto <bb 13>;

>   else

>     goto <bb 14>;

>

>   <bb 13>:

>   _12 = _33 == 2;

>   _13 = (integer) _12;

>

> and DOM3 deduces:

>

>   <bb 13>:

>   _12 = 0;

>   _13 = 0;

>

> because it computes that _33 has value 1 in BB 13 since it's a boolean.


Btw, isn't this the issue?  You'd need to avoid this as well I guess:

          /* Special case comparing booleans against a constant as we
             know the value of OP0 on both arms of the branch.  i.e., we
             can record an equivalence for OP0 rather than COND.

             However, don't do this if the constant isn't zero or one.
             Such conditionals will get optimized more thoroughly during
             the domwalk.  */
          if ((code == EQ_EXPR || code == NE_EXPR)
              && TREE_CODE (op0) == SSA_NAME
              && ssa_name_has_boolean_range (op0)
              && is_gimple_min_invariant (op1)
              && (integer_zerop (op1) || integer_onep (op1)))
            {
...

and thus:

bool
ssa_name_has_boolean_range (tree op)
{
  gcc_assert (TREE_CODE (op) == SSA_NAME);

  /* Boolean types always have a range [0..1].  */
  if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE)
    return true;

as said, there are _very_ many places in the compiler you'd need to "fix".

Basically treating BOOLEAN_TYPE this way is to cover frontends like
fortran who have multiple boolean types of different size (and not precision 1
IIRC).  An alternative would be to track them all down and force their
precision to 1 with unknown fallout.  And then document BOOLEAN_TYPE
isn't really special anymore (and fix all places).  But then, why do we have
BOOLEAN_TYPE ...

Richard.

> The problem was introduced by the new factor_out_conditional_conversion:

>

>       /* If arg1 is an INTEGER_CST, fold it to new type.  */

>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))

>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))

>         {

>           if (gimple_assign_cast_p (arg0_def_stmt))

>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);

>           else

>             return NULL;

>         }

>       else

>         return NULL

>

> int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked

> on constant 2 and a BOOLEAN_TYPE and returns true.

>

> BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so the

> outcome of int_fits_type_p is not unreasonable.  But this goes against the

> various transformations applied to boolean types in the compiler, which all

> assume that they can only take values 0 or 1.

>

> Hence the attached fix (which should be a no-op except for Ada), tested on

> x86_64-suse-linux, OK for mainline and 6 branch?

>

>

> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.

>

>

> 2016-10-18  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * gnat.dg/opt59.adb: New test.

>         * gnat.dg/opt59_pkg.ad[sb]: New helper.

>

> --

> Eric Botcazou
Eric Botcazou Oct. 21, 2016, 7:27 a.m. UTC | #7
> Because boolean_type_node was different?  Well, are really _all_ boolean

> types this way or can you keep using BOOLEAN_TYPE for boolean_type_node for

> things like interfacing to C and C++ (and the builtins, etc.) and use

> ENUMERAL_TYPE for the cases where bool can have non zero/one values?


They are all like that.  In a valid program, they can have only zero or one as 
values but, unlike C, in Ada you cannot avert your yes on undefined behaviour, 
you need to handle it gracefully enough, hence the need to have a base type 
for them (i.e. size of mode == precision).  ENUMERAL_TYPE would not really 
work either for the same reason.  So we would need INTEGER_TYPE for strict 
correctness, but this would add casts from/to boolean_type_node all over the 
place so is not very appealing either.

-- 
Eric Botcazou
Eric Botcazou Oct. 21, 2016, 7:40 a.m. UTC | #8
> Btw, isn't this the issue?  You'd need to avoid this as well I guess:

> 

>           /* Special case comparing booleans against a constant as we

>              know the value of OP0 on both arms of the branch.  i.e., we

>              can record an equivalence for OP0 rather than COND.

> 

>              However, don't do this if the constant isn't zero or one.

>              Such conditionals will get optimized more thoroughly during

>              the domwalk.  */

>           if ((code == EQ_EXPR || code == NE_EXPR)

>               && TREE_CODE (op0) == SSA_NAME

>               && ssa_name_has_boolean_range (op0)

>               && is_gimple_min_invariant (op1)

>               && (integer_zerop (op1) || integer_onep (op1)))

>             {

> ...

> 

> and thus:

> 

> bool

> ssa_name_has_boolean_range (tree op)

> {

>   gcc_assert (TREE_CODE (op) == SSA_NAME);

> 

>   /* Boolean types always have a range [0..1].  */

>   if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE)

>     return true;

> 

> as said, there are _very_ many places in the compiler you'd need to "fix".


I don't think so, you just need to prevent the compiler from creating boolean 
values that are not zero and one, there is none in a valid Ada program.  This 
has worked perfectly for years, it's because factor_out_conditional_conversion 
breaks the documented interface of int_fits_type_p by sending it a boolean 
type instead of an integer type that the regression was introduced.  This was 
unfortunatelty not caught in the review, so we need to do damage control now, 
including on the 6 branch, and my patch is a rather safe way of doing it.

> Basically treating BOOLEAN_TYPE this way is to cover frontends like

> fortran who have multiple boolean types of different size (and not precision

> 1 IIRC).  An alternative would be to track them all down and force their

> precision to 1 with unknown fallout.  And then document BOOLEAN_TYPE

> isn't really special anymore (and fix all places).  But then, why do we have

> BOOLEAN_TYPE ...


I think BOOLEAN_TYPE essentially works fine for Ada if you don't try to treat 
it like an INTEGER_TYPE, as the new factor_out_conditional_conversion does...

-- 
Eric Botcazou
Richard Biener Oct. 21, 2016, 8:25 a.m. UTC | #9
On Fri, Oct 21, 2016 at 9:40 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Btw, isn't this the issue?  You'd need to avoid this as well I guess:

>>

>>           /* Special case comparing booleans against a constant as we

>>              know the value of OP0 on both arms of the branch.  i.e., we

>>              can record an equivalence for OP0 rather than COND.

>>

>>              However, don't do this if the constant isn't zero or one.

>>              Such conditionals will get optimized more thoroughly during

>>              the domwalk.  */

>>           if ((code == EQ_EXPR || code == NE_EXPR)

>>               && TREE_CODE (op0) == SSA_NAME

>>               && ssa_name_has_boolean_range (op0)

>>               && is_gimple_min_invariant (op1)

>>               && (integer_zerop (op1) || integer_onep (op1)))

>>             {

>> ...

>>

>> and thus:

>>

>> bool

>> ssa_name_has_boolean_range (tree op)

>> {

>>   gcc_assert (TREE_CODE (op) == SSA_NAME);

>>

>>   /* Boolean types always have a range [0..1].  */

>>   if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE)

>>     return true;

>>

>> as said, there are _very_ many places in the compiler you'd need to "fix".

>

> I don't think so, you just need to prevent the compiler from creating boolean

> values that are not zero and one, there is none in a valid Ada program.  This

> has worked perfectly for years, it's because factor_out_conditional_conversion

> breaks the documented interface of int_fits_type_p by sending it a boolean

> type instead of an integer type that the regression was introduced.  This was

> unfortunatelty not caught in the review, so we need to do damage control now,

> including on the 6 branch, and my patch is a rather safe way of doing it.


Sure it's "safe", but I pointed you to wi::fits_tree_p which is the more modern
variant of int_fits_type_p - you at least should fix that for
consistency as well.

Btw, if int_fits_type_p is supposed to be only called for INTEGER_TYPEs
then

      /* If arg1 is an INTEGER_CST, fold it to new type.  */
      if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
          && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
        {
          if (gimple_assign_cast_p (arg0_def_stmt))
            new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
          else
            return NULL;
        }

should maybe be changed to check for INTEGER_TYPE instead of
INTEGRAL_TYPE_P?  And int_fits_type_p should have an assert
asserting it is fed only INTEGER_TYPEs?

Alternatively int_fits_type_p looks at TYPE_MIN/MAX_VALUE, would
it work to set that to [0, 1] for your Ada BOOLEAN_TYPEs?

For your patch, can you use integer_zerop () || integer_truep () and also
fix wi::fits_to_tree_p?

Would it work to have wide_int_to_tree check that we never generate
a BOOLEAN_TYPE constant that is not zero or one?  That is, does
the Ada FE create those?

Thanks,
Richard.

>> Basically treating BOOLEAN_TYPE this way is to cover frontends like

>> fortran who have multiple boolean types of different size (and not precision

>> 1 IIRC).  An alternative would be to track them all down and force their

>> precision to 1 with unknown fallout.  And then document BOOLEAN_TYPE

>> isn't really special anymore (and fix all places).  But then, why do we have

>> BOOLEAN_TYPE ...

>

> I think BOOLEAN_TYPE essentially works fine for Ada if you don't try to treat

> it like an INTEGER_TYPE, as the new factor_out_conditional_conversion does...

> --

> Eric Botcazou
Eric Botcazou Oct. 24, 2016, 9:15 a.m. UTC | #10
> Btw, if int_fits_type_p is supposed to be only called for INTEGER_TYPEs

> then

> 

>       /* If arg1 is an INTEGER_CST, fold it to new type.  */

>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))

>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))

>         {

>           if (gimple_assign_cast_p (arg0_def_stmt))

>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);

>           else

>             return NULL;

>         }

> 

> should maybe be changed to check for INTEGER_TYPE instead of

> INTEGRAL_TYPE_P?  And int_fits_type_p should have an assert

> asserting it is fed only INTEGER_TYPEs?


Yes, that would certainly solve the problem too.  No strong opinion, my 
proposed fix was a compromise aimed at changing nothing except for Ada.

> Alternatively int_fits_type_p looks at TYPE_MIN/MAX_VALUE, would

> it work to set that to [0, 1] for your Ada BOOLEAN_TYPEs?


No, BOOLEAN_TYPEs are base types in Ada so must have maximal range.

> For your patch, can you use integer_zerop () || integer_truep () and also

> fix wi::fits_to_tree_p?


But integer_truep is just integer_onep for BOOLEAN_TYPEs.  And fits_to_tree_p 
doesn't really need to be changed if int_fits_type_p is and the API layering 
is properly enforced, does it?

> Would it work to have wide_int_to_tree check that we never generate

> a BOOLEAN_TYPE constant that is not zero or one?  That is, does

> the Ada FE create those?


In a valid program, no.  In an invalid program, maybe, in which case we don't 
want to ICE so we would need to filter them out and raise Constraint_Error 
instead when we run into one; certainly doable I'd say.

-- 
Eric Botcazou
Richard Biener Oct. 24, 2016, 9:49 a.m. UTC | #11
On Mon, Oct 24, 2016 at 11:15 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Btw, if int_fits_type_p is supposed to be only called for INTEGER_TYPEs

>> then

>>

>>       /* If arg1 is an INTEGER_CST, fold it to new type.  */

>>       if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))

>>           && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))

>>         {

>>           if (gimple_assign_cast_p (arg0_def_stmt))

>>             new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);

>>           else

>>             return NULL;

>>         }

>>

>> should maybe be changed to check for INTEGER_TYPE instead of

>> INTEGRAL_TYPE_P?  And int_fits_type_p should have an assert

>> asserting it is fed only INTEGER_TYPEs?

>

> Yes, that would certainly solve the problem too.  No strong opinion, my

> proposed fix was a compromise aimed at changing nothing except for Ada.

>

>> Alternatively int_fits_type_p looks at TYPE_MIN/MAX_VALUE, would

>> it work to set that to [0, 1] for your Ada BOOLEAN_TYPEs?

>

> No, BOOLEAN_TYPEs are base types in Ada so must have maximal range.

>

>> For your patch, can you use integer_zerop () || integer_truep () and also

>> fix wi::fits_to_tree_p?

>

> But integer_truep is just integer_onep for BOOLEAN_TYPEs.


Yes, but it's more descriptive IMHO.

>  And fits_to_tree_p

> doesn't really need to be changed if int_fits_type_p is and the API layering

> is properly enforced, does it?


Well. fits_to_tree_p avoids creating an INTEGER_CST in ggc memory and thus
is the prefered way to test if you have a wide-int but not yet an INTEGER_CST.

>> Would it work to have wide_int_to_tree check that we never generate

>> a BOOLEAN_TYPE constant that is not zero or one?  That is, does

>> the Ada FE create those?

>

> In a valid program, no.  In an invalid program, maybe, in which case we don't

> want to ICE so we would need to filter them out and raise Constraint_Error

> instead when we run into one; certainly doable I'd say.


Ok, just looking for a way to "easily" fend off other cases that are not catched
by patching the fits-tree routines.

Richard.

> --

> Eric Botcazou
diff mbox

Patch

Index: tree.c
===================================================================
--- tree.c	(revision 241294)
+++ tree.c	(working copy)
@@ -9065,8 +9065,8 @@  get_narrower (tree op, int *unsignedp_pt
   return win;
 }
 
-/* Returns true if integer constant C has a value that is permissible
-   for type TYPE (an INTEGER_TYPE).  */
+/* Return true if integer constant C has a value that is permissible
+   for TYPE, an integral type.  */
 
 bool
 int_fits_type_p (const_tree c, const_tree type)
@@ -9075,6 +9075,11 @@  int_fits_type_p (const_tree c, const_tre
   bool ok_for_low_bound, ok_for_high_bound;
   signop sgn_c = TYPE_SIGN (TREE_TYPE (c));
 
+  /* Short-circuit boolean types since various transformations assume that
+     they can only take values 0 and 1.  */
+  if (TREE_CODE (type) == BOOLEAN_TYPE)
+    return integer_zerop (c) || integer_onep (c);
+
 retry:
   type_low_bound = TYPE_MIN_VALUE (type);
   type_high_bound = TYPE_MAX_VALUE (type);