RE: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 bit types [PR114666]

["Andrew Pinski (QUIC)" <quic_apinski@quicinc.com>]

> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Thursday, April 11, 2024 2:31 AM
> To: Andrew Pinski (QUIC) <quic_apinski@quicinc.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 bit
> types [PR114666]
> 
> On Thu, Apr 11, 2024 at 10:43 AM Andrew Pinski
> <quic_apinski@quicinc.com> wrote:
> >
> > The issue here is that the `a?~t:t` pattern assumed (maybe correctly)
> > that a here was always going to be a unsigned boolean type. This fixes
> > the problem in both patterns to cast the operand to boolean type first.
> >
> > I should note that VRP seems to be keep on wanting to produce `a ==
> > 0?1:-2` from `((int)a) ^ 1` is a bit odd and partly is the cause of
> > the issue and there seems to be some disconnect on what should be the
> > canonical form. That will be something to look at for GCC 15.
> >
> > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> >         PR tree-optimization/114666
> >
> > gcc/ChangeLog:
> >
> >         * match.pd (`!a?b:c`): Cast `a` to boolean type for cond for
> >         gimple.
> >         (`a?~t:t`): Cast `a` to boolean type before casting it
> >         to the type.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.c-torture/execute/bitfld-signed1-1.c: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> > ---
> >  gcc/match.pd                                        | 10 +++++++---
> >  .../gcc.c-torture/execute/bitfld-signed1-1.c        | 13 +++++++++++++
> >  2 files changed, 20 insertions(+), 3 deletions(-)  create mode 100644
> > gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd index
> > 15a1e7350d4..ffc928b656a 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -5895,7 +5895,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >   /* !A ? B : C -> A ? C : B.  */
> >   (simplify
> >    (cnd (logical_inverted_value truth_valued_p@0) @1 @2)
> > -  (cnd @0 @2 @1)))
> > +  /* For gimple, make sure the operand to COND is a boolean type,
> > +     truth_valued_p will match 1bit integers too. */  (if (GIMPLE &&
> > + cnd == COND_EXPR)
> > +   (cnd (convert:boolean_type_node @0) @2 @1)
> > +   (cnd @0 @2 @1))))
> 
> This looks "wrong" for GENERIC still?

I tired without the GIMPLE check and ran into the testcase gcc.dg/torture/builtins-isinf-sign-1.c failing. Because the extra convert was blocking seeing both sides of an equal was the same (I didn't look into it further than that). So I decided to limit it to GIMPLE only.

> But this is not really part of the fix but deciding we should not have
> signed:1 as
> cond operand?  I'll note that truth_valued_p allows signed:1.
> 
> Maybe as minimal surgery add a TYPE_UNSIGNED (TREE_TPE (@0)) check here
> instead?

That might work, let me try.

> 
> >  /* abs/negative simplifications moved from
> fold_cond_expr_with_comparison.
> >
> > @@ -7099,8 +7103,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >         && (!wascmp || TYPE_PRECISION (type) == 1))
> >     (if ((!TYPE_UNSIGNED (type) && TREE_CODE (type) == BOOLEAN_TYPE)
> >         || TYPE_PRECISION (type) == 1)
> > -    (bit_xor (convert:type @0) @2)
> > -    (bit_xor (negate (convert:type @0)) @2)))))
> > +    (bit_xor (convert:type (convert:boolean_type_node @0)) @2)
> > +    (bit_xor (negate (convert:type (convert:boolean_type_node @0)))
> > + @2)))))
> >  #endif
> 
> This looks OK, but then testing TYPE_UNSIGNED (TREE_TYPE (@0)) might be
> better?
> 

Let me do that just like the other pattern.

> Does this all just go downhill from what VRP creates?  That is, would IL
> checking have had a chance detecting it if we say signed:1 are not valid as
> condition?

Yes. So what VRP produces in the testcase is:
`_2 == 0 ? 1 : -2u` (where _2 is the signed 1bit integer).
Now maybe the COND_EXPR should be the canonical form for constants (but that is for a different patch I think, I added it to the list of things I should look into for GCC 15).

> 
> That said, the latter pattern definitely needs guarding/adjustment, I'm not
> sure the former is wrong?  Semantically [VEC_]COND_EXPR is op0 != 0 ? ... : ...

I forgot to mention that to fix the bug only one of the 2 hunks are needed.

> 
> Richard.
> 
> >  /* Simplify pointer equality compares using PTA.  */ diff --git
> > a/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> > b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> > new file mode 100644
> > index 00000000000..b0ff120ea51
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> > @@ -0,0 +1,13 @@
> > +/* PR tree-optimization/114666 */
> > +/* We used to miscompile this to be always aborting
> > +   due to the use of the signed 1bit into the COND_EXPR. */
> > +
> > +struct {
> > +  signed a : 1;
> > +} b = {-1};
> > +char c;
> > +int main()
> > +{
> > +  if ((b.a ^ 1UL) < 3)
> > +    __builtin_abort();
> > +}
> > --
> > 2.43.0
> >