On Wed, Oct 30, 2024 at 5:34 AM Jovan Vukic <jovan.vu...@rt-rk.com> wrote: > > Thanks for the feedback on the first version of the patch. Accordingly: > > I have corrected the code formatting as requested. I added new tests to > the existing file phi-opt-11.c, instead of creating a new one.
The previous practice has been adding a new file rather than appending to the old one. Though maybe this is a good exception to that rule. The main reason is to compare between a much older testsuite run and a testsuite run after this patch. I am not so picky about this either way though; just bring up the reasons why we did it differently before. > > I performed testing before and after applying the patch on the x86 > architecture, and I confirm that there are no new regressions. > > The logic and general code of the patch itself have not been changed. > > > So the A EQ/NE B expression, we can reverse A and B in the expression > > and still get the same result. But don't we have to be more careful for > > the TRUE/FALSE arms of the ternary? For BIT_AND we need ? a : b for > > BIT_IOR we need ? b : a. > > > > I don't see that gets verified in the existing code or after your > > change. I suspect I'm just missing something here. Can you clarify how > > we verify that BIT_AND gets ? a : b for the true/false arms and that > > BIT_IOR gets ? b : a for the true/false arms? > > I did not communicate this clearly last time, but the existing optimization > simplifies the expression "(cond & (a == b)) ? a : b" to the simpler "b". > Similarly, the expression "(cond & (a == b)) ? b : a" simplifies to "a". > > Thus, the existing and my optimization perform the following > simplifications: > > (cond & (a == b)) ? a : b -> b > (cond & (a == b)) ? b : a -> a > (cond | (a != b)) ? a : b -> a > (cond | (a != b)) ? b : a -> b > > For this reason, for BIT_AND_EXPR when we have A EQ B, it is sufficient to > confirm that one operand matches the true/false arm and the other matches > the false/true arm. In both cases, we simplify the expression to the third > operand of the ternary operation (i.e., OP0 ? OP1 : OP2 simplifies to OP2). > This is achieved in the value_replacement function after successfully > setting the value of *code within the rhs_is_fed_for_value_replacement > function to EQ_EXPR. > > For BIT_IOR_EXPR, the same check is performed for A NE B, except now > *code remains NE_EXPR, and then value_replacement returns the second > operand (i.e., OP0 ? OP1 : OP2 simplifies to OP1). I had started to rewrite this part of phiopt to use match-and-simplify and to handle these kind of things. I attached the first set of patches to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51030 and redid a simplified version attached here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61110 . I am not blocking this patch but mentioning that a rewrite is on the way (how long before I get back to this, I don't know yet). Also as far as reviewing the patch as it stands alone. + && ((bit_expression_code == BIT_AND_EXPR + && gimple_assign_rhs_code (def1) == EQ_EXPR) + || (bit_expression_code == BIT_IOR_EXPR + && gimple_assign_rhs_code (def1) == NE_EXPR))) This could be simplified down to I think: `gimple_assign_rhs_code (def1) == (bit_expression_code == BIT_AND_EXPR) ? EQ_EXPR : NE_EXPR` And then add an assert at the beginning of rhs_is_fed_for_value_replacement checking to make sure bit_expression_code is either `BIT_AND_EXPR` or `BIT_IOR_EXPR` or maybe return false right if it is not BIT_AND_EXPR/BIT_IOR_EXPR. Otherwise the change looks good to me (but I can't fully approve it). Thanks, Andrew Pinski > > 2024-10-30 Jovan Vukic <jovan.vu...@rt-rk.com> > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc > (rhs_is_fed_for_value_replacement): Add a new optimization opportunity > for BIT_IOR_EXPR and a != b. > (operand_equal_for_value_replacement): Ditto. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tree-ssa/phi-opt-11.c: Add more tests. > > > CONFIDENTIALITY: The contents of this e-mail are confidential and intended > only for the above addressee(s). If you are not the intended recipient, or > the person responsible for delivering it to the intended recipient, copying > or delivering it to anyone else or using it in any unauthorized manner is > prohibited and may be unlawful. If you receive this e-mail by mistake, please > notify the sender and the systems administrator at straym...@rt-rk.com > immediately.
