On Dec 18, 2024, Jakub Jelinek <ja...@redhat.com> wrote: > 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. Ugh, indeed, thanks! > 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. */ *nod*, that one bugged me as well, but it resisted my attempts to come up with a counterexample. Fortunately, PR118046 came to the rescue, and there's a patch pending review that should alleviate this specific concern. https://gcc.gnu.org/pipermail/gcc-patches/2024-December/671809.html I see I failed to adjust that comment, though. Fixing... > 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. Yup. In the latter case, the final conversion to int is dropped before we reach ifcombine, so outer_type is unsigned char and it works just as expected. > 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? These combinations have come up often during testing, and I'm pretty confident about them. Their order in which they appear is relevant for the code, as it is, to be able to deal with them all. The last-minute rewrites, after years of not touching this intricate pair of functions in gimple-fold, have been a significant source of fallout, alas. > 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 > ? Yeah, that bit is sound, at least in theory. The mask is (supposed to be?) recognized and applied on both xor operands. That's the reason why *xor_cmp_op = *pexp rather than = exp. Please feel free to share any other concerns you might have, I'd be happy to double-check, and hopefully alleviate them, or at least follow leads to other problems I haven't considered. Thanks, -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer More tolerance and less prejudice are key for inclusion and diversity Excluding neuro-others for not behaving ""normal"" is *not* inclusive
