On Fri, Apr 12, 2024 at 1:25 AM Andrew Pinski (QUIC)
<quic_apin...@quicinc.com> wrote:
>
> > -----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).
Ah OK, so the !A ? B : C -> A ? C : B transform turns the
"proper" conditional into an improper one (if we want to restrict it).
And then the other pattern matches doing the wrong transform.
> >
> > 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
> > >
