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