> Am 18.12.2024 um 10:10 schrieb Jakub Jelinek <ja...@redhat.com>:
>
> Hi!
>
> The function comment says:
> *XOR_P is to be FALSE if EXP might be a XOR used in a compare, in which
> case, if XOR_CMP_OP is a zero constant, it will be overridden with *PEXP,
> *XOR_P will be set to TRUE, and the left-hand operand of the XOR will be
> decoded. If *XOR_P is TRUE, XOR_CMP_OP is supposed to be NULL, and then the
> right-hand operand of the XOR will be decoded.
> and the comment right above the xor_p handling says
> /* Turn (a ^ b) [!]= 0 into a [!]= b. */
> but I don't see anything that would actually check that the other operand is
> 0, in the testcase below it happily optimizes (a ^ 1) == 8 into a == 1.
>
> The following patch adds that check.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
This is ok, please allow Alex to comment.
Richard
> Note, there are various other parts of the function I'm worried about, but
> haven't had time to construct counterexamples yet.
>
> One worrying thing is the
> /* Drop casts, only save the outermost type. We need not worry about
> narrowing then widening casts, or vice-versa, for those that are not
> essential for the compare have already been optimized out at this
> point. */
> comment, while obviously there are various optimizations which do optimize
> nested casts and the like, I'm not really sure it is safe to rely on them
> happening always before this optimization, there are various options to
> disable certain optimizations and some IL could appear right before
> ifcombine without being optimized yet the way this routine expects.
> Plus, the 3 casts are looked through in between various optimizations which
> might make those narrowing/widening or vice versa cases necessary.
>
> Also, e.g. for the xor optimization, I think there is a difference between
> int a and
> (a ^ 0x23) == 0
> and
> ((int) (((unsigned char) a) ^ (unsigned char) 0x23)) == 0
> etc.
>
> Another thing I'm worrying about are mixing up the different patterns
> together, there is the BIT_AND_EXPR handling, BIT_XOR_EXPR handling,
> RSHIFT_EXPR handling and then load handling.
> What if all 4 appear together, or 3 of them, 2 of them?
> Is the xor optimization still valid if there is BIT_AND_EXPR in between?
> I.e. instead of
> (a ^ 123) == 0
> there is
> ((a ^ 123) & 234) == 0
> ?
I was also worried during review about the very generic conversion stripping
but it was said the fold-const code does the same.
> 2024-12-18 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/118081
> * gimple-fold.cc (decode_field_reference): Only set *xor_p to true
> if *xor_cmp_op is integer_zerop.
>
> * gcc.dg/pr118081.c: New test.
>
> --- gcc/gimple-fold.cc.jj 2024-12-14 11:28:08.000000000 +0100
> +++ gcc/gimple-fold.cc 2024-12-17 17:27:31.294874825 +0100
> @@ -7572,7 +7572,7 @@ decode_field_reference (tree *pexp, HOST
> /* Not much we can do when xor appears in the right-hand compare
> operand. */
> return NULL_TREE;
> - else
> + else if (integer_zerop (*xor_cmp_op))
> {
> *xor_p = true;
> exp = res_ops[0];
> --- gcc/testsuite/gcc.dg/pr118081.c.jj 2024-12-17 17:31:07.522860128 +0100
> +++ gcc/testsuite/gcc.dg/pr118081.c 2024-12-17 17:29:59.131813646 +0100
> @@ -0,0 +1,28 @@
> +/* PR tree-optimization/118081 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-tree-vrp -fno-expensive-optimizations" } */
> +
> +int a, b;
> +
> +int
> +foo (int f)
> +{
> + return f ? f || 0 : f;
> +}
> +
> +void
> +bar (void)
> +{
> + b = a ? a : 1;
> + int i = foo (1 ^ b);
> + signed char h = i - 8;
> + if (h)
> + return;
> + __builtin_abort ();
> +}
> +
> +int
> +main ()
> +{
> + bar ();
> +}
>
> Jakub
>
