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.
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).
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.
---
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-11.c | 31 +++++++++++++-
gcc/tree-ssa-phiopt.cc | 48 ++++++++++++++--------
2 files changed, 60 insertions(+), 19 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-11.c
b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-11.c
index 14c82cd5216..d1e284c5325 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-11.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-11.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-O1 -fdump-tree-optimized --param
logical-op-non-short-circuit=1" } */
+/* { dg-options "-O1 -fdump-tree-phiopt2 -fdump-tree-optimized --param
logical-op-non-short-circuit=1" } */
int f(int a, int b, int c)
{
@@ -22,4 +22,33 @@ int h(int a, int b, int c, int d)
return a;
}
+int i(int a, int b, int c)
+{
+ if ((a > c) & (a == b))
+ return a;
+ return b;
+}
+
+int j(int a, int b, int c)
+{
+ if ((a > c) & (a == b))
+ return b;
+ return a;
+}
+
+int k(int a, int b, int c)
+{
+ if ((a > c) | (a != b))
+ return b;
+ return a;
+}
+
+int l(int a, int b, int c)
+{
+ if ((a > c) | (a != b))
+ return a;
+ return b;
+}
+
+/* { dg-final { scan-tree-dump-times "if" 0 "phiopt2" } } */
/* { dg-final { scan-tree-dump-times "if" 0 "optimized" } } */
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index cffafe101a4..61b33bfc361 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)
return false;
}
-/* RHS is a source argument in a BIT_AND_EXPR which feeds a conditional
- of the form SSA_NAME NE 0.
+/* RHS is a source argument in a BIT_AND_EXPR or BIT_IOR_EXPR which feeds
+ a conditional of the form SSA_NAME NE 0.
- If RHS is fed by a simple EQ_EXPR comparison of two values, see if
- the two input values of the EQ_EXPR match arg0 and arg1.
+ If RHS is fed by a simple EQ_EXPR or NE_EXPR comparison of two values,
+ see if the two input values of the comparison match arg0 and arg1.
If so update *code and return TRUE. Otherwise return FALSE. */
static bool
rhs_is_fed_for_value_replacement (const_tree arg0, const_tree arg1,
- enum tree_code *code, const_tree rhs)
+ enum tree_code *code, const_tree rhs,
+ enum tree_code bit_expression_code)
{
/* Obviously if RHS is not an SSA_NAME, we can't look at the defining
statement. */
@@ -1096,11 +1097,15 @@ rhs_is_fed_for_value_replacement (const_tree arg0,
const_tree arg1,
{
gimple *def1 = SSA_NAME_DEF_STMT (rhs);
- /* 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)))
{
- /* Finally verify the source operands of the EQ_EXPR are equal
- to arg0 and arg1. */
+ /* Finally verify the source operands of the EQ_EXPR or NE_EXPR
+ are equal to arg0 and arg1. */
tree op0 = gimple_assign_rhs1 (def1);
tree op1 = gimple_assign_rhs2 (def1);
if ((operand_equal_for_phi_arg_p (arg0, op0)
@@ -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.
Return FALSE otherwise. */
@@ -1139,27 +1145,33 @@ operand_equal_for_value_replacement (const_tree arg0,
const_tree arg1,
return true;
/* Now handle more complex case where we have an EQ comparison
- which feeds a BIT_AND_EXPR which feeds COND.
+ feeding a BIT_AND_EXPR, or a NE comparison feeding a BIT_IOR_EXPR,
+ which then feeds into COND.
First verify that COND is of the form SSA_NAME NE 0. */
if (*code != NE_EXPR || !integer_zerop (rhs)
|| TREE_CODE (lhs) != SSA_NAME)
return false;
- /* 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;
- /* Now verify arg0/arg1 correspond to the source arguments of an
- EQ comparison feeding the BIT_AND_EXPR. */
+ /* Now verify arg0/arg1 correspond to the source arguments of an EQ
+ comparison feeding the BIT_AND_EXPR or a NE comparison feeding the
+ BIT_IOR_EXPR. */
tree tmp = gimple_assign_rhs1 (def);
- if (rhs_is_fed_for_value_replacement (arg0, arg1, code, tmp))
+ if (rhs_is_fed_for_value_replacement (arg0, arg1, code, tmp,
+ gimple_assign_rhs_code (def)))
return true;
tmp = gimple_assign_rhs2 (def);
- if (rhs_is_fed_for_value_replacement (arg0, arg1, code, tmp))
+ if (rhs_is_fed_for_value_replacement (arg0, arg1, code, tmp,
+ gimple_assign_rhs_code (def)))
return true;
return false;
--
2.43.0
