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
>
