> -----Original Message-----
> From: Richard Biener <richard.guent...@gmail.com>
> Sent: Thursday, April 11, 2024 2:31 AM
> To: Andrew Pinski (QUIC) <quic_apin...@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_apin...@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_apin...@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
> >
