On 10/22/24 2:32 AM, Jovan Vukic wrote:
Currently, within the phiopt pass, under value_replacement, we have the
option to replace the expression "(cond & (a == b)) ? a : b" with "b". It
checks whether there is a BIT_AND_EXPR and verifies if one of the operands
contains the expression "a == b".
However, the same result should also apply to the expression with the
inverted condition "(cond | (a != b)) ? b : a," so this patch makes slight
modifications to the existing code to cover this situation as well. The
patch adds a check for whether there is a BIT_IOR_EXPR and if it contains
"a != b".
Good catch.
I added a test for both optimizations, as I could not find an adequate test
in the testsuite for the first one (even though that optimization exists).
It looks like phi-opt-11.c could be a suitable test for the original
commit. I found that by doing a git blame to find the commit id that
added this code, then I looked that the entire patch for that commit and
saw that it added phi-opt-11.c.
iff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 38cc8506d1f..956fe919cc0 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -1078,17 +1078,18 @@ jump_function_from_stmt (tree *arg, gimple *stmt)
- /* Verify the defining statement has an EQ_EXPR on the RHS. */
- if (is_gimple_assign (def1) && gimple_assign_rhs_code (def1) == EQ_EXPR)
+ /* Verify the defining statement has an EQ_EXPR or NE_EXPR on the RHS.
*/
+ if (is_gimple_assign (def1) &&
+ ((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)))
So a formatting nit. We bring the operator down, indented just inside
the appropriate open paren rather than having the operator at the end of
a line. ie
if (foo
&& bar
&& (com || baz))
As an example.
@@ -1119,8 +1124,9 @@ rhs_is_fed_for_value_replacement (const_tree arg0,
const_tree arg1,
/* Return TRUE if arg0/arg1 are equal to the rhs/lhs or lhs/rhs of COND.
- Also return TRUE if arg0/arg1 are equal to the source arguments of a
- an EQ comparison feeding a BIT_AND_EXPR which feeds COND.
+ Also return TRUE if arg0/arg1 are equal to the source arguments of an
+ EQ comparison feeding a BIT_AND_EXPR, or NE comparison feeding a
+ BIT_IOR_EXPR which feeds COND.
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?
As a patch to the generic part of the compiler, the required testing
would be to bootstrap a platform like x86 before the patch, run the
testsuite & capture the results. Then apply the patch, bootstrap again,
run the testsuite again and compare the test results to ensure there are
no regressions.
- /* Now ensure that SSA_NAME is set by a BIT_AND_EXPR. */
+ /* Now ensure that SSA_NAME is set by a BIT_AND_EXPR or BIT_OR_EXPR. */
def = SSA_NAME_DEF_STMT (lhs);
- if (!is_gimple_assign (def) || gimple_assign_rhs_code (def) != BIT_AND_EXPR)
+ if (!is_gimple_assign (def) ||
+ (gimple_assign_rhs_code (def) != BIT_AND_EXPR &&
+ gimple_assign_rhs_code (def) != BIT_IOR_EXPR))
return false;
So the formatting nit shows up here again.
So the biggest functional question is whether or not we're testing that
for the new BIT_IOR case that we have ? b : a for the TRUE/FALSE arms.
And we need to get a confirmation that this bootstraps & regression
tests cleanly on x86, aarch64 or one of the other tier1 platforms.
Thanks,
Jeff
