* [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT
@ 2021-07-04 18:37 apinski
2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: apinski @ 2021-07-04 18:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
From: Andrew Pinski <apinski@marvell.com>
So the problem here is that replace_phi_edge_with_variable
will copy range information to a already (not newly) defined
ssa name. This causes wrong code later on.
This patch fixes the problem by requiring there to be statements
that are to be placed before the conditional to be able to
copy the range info; this assumes the statements will define
the ssa name.
gcc/ChangeLog:
PR tree-optimization/101256
* dbgcnt.def (phiopt_edge_range): New counter.
* tree-ssa-phiopt.c (replace_phi_edge_with_variable):
Add optional sequence which will be added before the old
conditional. Check sequence for non-null if we want to
update the range.
(two_value_replacement): Instead of inserting the sequence,
update the call to replace_phi_edge_with_variable.
(match_simplify_replacement): Likewise.
(minmax_replacement): Likewise.
(value_replacement): Create a sequence of statements
which would have defined the ssa name. Update call
to replace_phi_edge_with_variable.
gcc/testsuite/ChangeLog:
PR tree-optimization/101256
* g++.dg/torture/pr101256.C: New test.
---
gcc/dbgcnt.def | 1 +
gcc/testsuite/g++.dg/torture/pr101256.C | 28 +++++++++++++
gcc/tree-ssa-phiopt.c | 52 ++++++++++++++-----------
3 files changed, 59 insertions(+), 22 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/torture/pr101256.C
diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
index 93e7b4fd30e..2345899ba68 100644
--- a/gcc/dbgcnt.def
+++ b/gcc/dbgcnt.def
@@ -183,6 +183,7 @@ DEBUG_COUNTER (lim)
DEBUG_COUNTER (local_alloc_for_sched)
DEBUG_COUNTER (match)
DEBUG_COUNTER (merged_ipa_icf)
+DEBUG_COUNTER (phiopt_edge_range)
DEBUG_COUNTER (postreload_cse)
DEBUG_COUNTER (pre)
DEBUG_COUNTER (pre_insn)
diff --git a/gcc/testsuite/g++.dg/torture/pr101256.C b/gcc/testsuite/g++.dg/torture/pr101256.C
new file mode 100644
index 00000000000..973a8b4caf3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr101256.C
@@ -0,0 +1,28 @@
+// { dg-do run }
+
+template<class T>
+const T& max(const T& a, const T& b)
+{
+ return (a < b) ? b : a;
+}
+
+signed char var_5 = -128;
+unsigned int var_11 = 2144479212U;
+unsigned long long int arr [22];
+
+void
+__attribute__((noipa))
+test(signed char var_5, unsigned var_11) {
+ for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
+ arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
+}
+
+int main() {
+ for (int i_0 = 0; i_0 < 22; ++i_0)
+ arr [i_0] = 11834725929543695741ULL;
+
+ test(var_5, var_11);
+ if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
+ __builtin_abort ();
+ return 0;
+}
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index ab12e85569d..71f0019d877 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -50,6 +50,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-fold.h"
#include "internal-fn.h"
#include "gimple-range.h"
+#include "dbgcnt.h"
static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
@@ -73,7 +74,8 @@ static bool cond_store_replacement (basic_block, basic_block, edge, edge,
hash_set<tree> *);
static bool cond_if_else_store_replacement (basic_block, basic_block, basic_block);
static hash_set<tree> * get_non_trapping ();
-static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree);
+static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree,
+ gimple_seq = NULL);
static void hoist_adjacent_loads (basic_block, basic_block,
basic_block, basic_block);
static bool gate_hoist_loads (void);
@@ -382,18 +384,20 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
/* Replace PHI node element whose edge is E in block BB with variable NEW.
Remove the edge from COND_BLOCK which does not lead to BB (COND_BLOCK
- is known to have two edges, one of which must reach BB). */
+ is known to have two edges, one of which must reach BB).
+ Optionally insert stmts before the old condition. */
static void
replace_phi_edge_with_variable (basic_block cond_block,
- edge e, gphi *phi, tree new_tree)
+ edge e, gphi *phi, tree new_tree,
+ gimple_seq stmts)
{
basic_block bb = gimple_bb (phi);
basic_block block_to_remove;
gimple_stmt_iterator gsi;
tree phi_result = PHI_RESULT (phi);
- /* Duplicate range info if we're the only things setting the target PHI.
+ /* Duplicate range info if they are the only things setting the target PHI.
This is needed as later on, the new_tree will be replacing
The assignement of the PHI.
For an example:
@@ -401,19 +405,23 @@ replace_phi_edge_with_variable (basic_block cond_block,
_4 = min<a_1, 255>
goto bb2
- range<-INF,255>
+ # RANGE [-INF, 255]
a_3 = PHI<_4(1)>
bb3:
use(a_3)
- And _4 gets prograted into the use of a_3 and losing the range info.
- This can't be done for more than 2 incoming edges as the progration
- won't happen. */
+ And _4 gets propagated into the use of a_3 and losing the range info.
+ This can't be done for more than 2 incoming edges as the propagation
+ won't happen.
+ This also cannot be done if the name is not defined by statements to
+ be placed before the conditional. */
if (TREE_CODE (new_tree) == SSA_NAME
&& EDGE_COUNT (gimple_bb (phi)->preds) == 2
&& INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
&& !SSA_NAME_RANGE_INFO (new_tree)
- && SSA_NAME_RANGE_INFO (phi_result))
+ && SSA_NAME_RANGE_INFO (phi_result)
+ && stmts != NULL
+ && dbg_cnt (phiopt_edge_range))
duplicate_ssa_name_range_info (new_tree,
SSA_NAME_RANGE_TYPE (phi_result),
SSA_NAME_RANGE_INFO (phi_result));
@@ -443,6 +451,8 @@ replace_phi_edge_with_variable (basic_block cond_block,
/* Eliminate the COND_EXPR at the end of COND_BLOCK. */
gsi = gsi_last_bb (cond_block);
+ if (stmts)
+ gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
gsi_remove (&gsi, true);
statistics_counter_event (cfun, "Replace PHI with variable", 1);
@@ -802,10 +812,8 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
else
new_rhs = gimple_build (&stmts, MINUS_EXPR, type, arg, lhs);
new_rhs = gimple_convert (&stmts, TREE_TYPE (arg0), new_rhs);
- gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
- gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
- replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs);
+ replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs, stmts);
/* Note that we optimized this PHI. */
return true;
@@ -920,10 +928,8 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
gsi_move_before (&gsi1, &gsi);
reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
}
- if (seq)
- gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
- replace_phi_edge_with_variable (cond_bb, e1, phi, result);
+ replace_phi_edge_with_variable (cond_bb, e1, phi, result, seq);
/* Add Statistic here even though replace_phi_edge_with_variable already
does it as we want to be able to count when match-simplify happens vs
@@ -1396,6 +1402,7 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
&& absorbing_element_p (code_def,
cond_rhs, false, rhs2))))))
{
+ gimple_seq stmts = NULL;
gsi = gsi_for_stmt (cond);
/* Moving ASSIGN might change VR of lhs, e.g. when moving u_6
def-stmt in:
@@ -1417,11 +1424,15 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
tree plhs = gimple_assign_lhs (prep_stmt[i]);
reset_flow_sensitive_info (plhs);
gsi_from = gsi_for_stmt (prep_stmt[i]);
- gsi_move_before (&gsi_from, &gsi);
+ gsi_remove (&gsi_from, false);
+
+ gimple_seq_add_stmt (&stmts, prep_stmt[i]);
}
gsi_from = gsi_for_stmt (assign);
- gsi_move_before (&gsi_from, &gsi);
- replace_phi_edge_with_variable (cond_bb, e1, phi, lhs);
+ gsi_remove (&gsi_from, false);
+ gimple_seq_add_stmt (&stmts, assign);
+
+ replace_phi_edge_with_variable (cond_bb, e1, phi, lhs, stmts);
return 2;
}
@@ -1811,10 +1822,7 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
tree phi_result = PHI_RESULT (phi);
result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
- gsi = gsi_last_bb (cond_bb);
- gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
-
- replace_phi_edge_with_variable (cond_bb, e1, phi, result);
+ replace_phi_edge_with_variable (cond_bb, e1, phi, result, stmts);
return true;
}
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
@ 2021-07-04 18:37 ` apinski
2021-07-05 11:18 ` Richard Biener
2021-07-04 18:37 ` [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt apinski
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: apinski @ 2021-07-04 18:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
From: Andrew Pinski <apinski@marvell.com>
HONOR_SIGNED_ZEROS, HONOR_SIGN_DEPENDENT_ROUNDING, and HONOR_SNANS all
have an overload for taking a tree type now, so we should do that instead.
OK? Bootstrapped and tested on x86_64-linux-gnu.
gcc/ChangeLog:
PR middle-end/101237
* fold-const.c (negate_expr_p): Remove call to element_mode
and TREE_MODE/TREE_TYPE when calling HONOR_SIGNED_ZEROS,
HONOR_SIGN_DEPENDENT_ROUNDING, and HONOR_SNANS.
(fold_negate_expr_1): Likewise.
(const_unop): Likewise.
(fold_cond_expr_with_comparison): Likewise.
(fold_binary_loc): Likewise.
(fold_ternary_loc): Likewise.
(tree_call_nonnegative_warnv_p): Likewise.
* match.pd (-(A + B) -> (-B) - A): Likewise.
---
gcc/fold-const.c | 46 +++++++++++++++++++++++-----------------------
gcc/match.pd | 4 ++--
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index dfccbaec683..e0cdb75fb26 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -432,8 +432,8 @@ negate_expr_p (tree t)
return negate_expr_p (TREE_OPERAND (t, 0));
case PLUS_EXPR:
- if (HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
- || HONOR_SIGNED_ZEROS (element_mode (type))
+ if (HONOR_SIGN_DEPENDENT_ROUNDING (type)
+ || HONOR_SIGNED_ZEROS (type)
|| (ANY_INTEGRAL_TYPE_P (type)
&& ! TYPE_OVERFLOW_WRAPS (type)))
return false;
@@ -445,8 +445,8 @@ negate_expr_p (tree t)
case MINUS_EXPR:
/* We can't turn -(A-B) into B-A when we honor signed zeros. */
- return !HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
- && !HONOR_SIGNED_ZEROS (element_mode (type))
+ return !HONOR_SIGN_DEPENDENT_ROUNDING (type)
+ && !HONOR_SIGNED_ZEROS (type)
&& (! ANY_INTEGRAL_TYPE_P (type)
|| TYPE_OVERFLOW_WRAPS (type));
@@ -468,7 +468,7 @@ negate_expr_p (tree t)
/* Fall through. */
case RDIV_EXPR:
- if (! HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (TREE_TYPE (t))))
+ if (! HONOR_SIGN_DEPENDENT_ROUNDING (t))
return negate_expr_p (TREE_OPERAND (t, 1))
|| negate_expr_p (TREE_OPERAND (t, 0));
break;
@@ -605,8 +605,8 @@ fold_negate_expr_1 (location_t loc, tree t)
break;
case PLUS_EXPR:
- if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
- && !HONOR_SIGNED_ZEROS (element_mode (type)))
+ if (!HONOR_SIGN_DEPENDENT_ROUNDING (type)
+ && !HONOR_SIGNED_ZEROS (type))
{
/* -(A + B) -> (-B) - A. */
if (negate_expr_p (TREE_OPERAND (t, 1)))
@@ -628,8 +628,8 @@ fold_negate_expr_1 (location_t loc, tree t)
case MINUS_EXPR:
/* - (A - B) -> B - A */
- if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
- && !HONOR_SIGNED_ZEROS (element_mode (type)))
+ if (!HONOR_SIGN_DEPENDENT_ROUNDING (type)
+ && !HONOR_SIGNED_ZEROS (type))
return fold_build2_loc (loc, MINUS_EXPR, type,
TREE_OPERAND (t, 1), TREE_OPERAND (t, 0));
break;
@@ -641,7 +641,7 @@ fold_negate_expr_1 (location_t loc, tree t)
/* Fall through. */
case RDIV_EXPR:
- if (! HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)))
+ if (! HONOR_SIGN_DEPENDENT_ROUNDING (type))
{
tem = TREE_OPERAND (t, 1);
if (negate_expr_p (tem))
@@ -1725,7 +1725,7 @@ const_unop (enum tree_code code, tree type, tree arg0)
/* Don't perform the operation, other than NEGATE and ABS, if
flag_signaling_nans is on and the operand is a signaling NaN. */
if (TREE_CODE (arg0) == REAL_CST
- && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
+ && HONOR_SNANS (arg0)
&& REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg0))
&& code != NEGATE_EXPR
&& code != ABS_EXPR
@@ -2135,7 +2135,7 @@ fold_convert_const_real_from_real (tree type, const_tree arg1)
/* Don't perform the operation if flag_signaling_nans is on
and the operand is a signaling NaN. */
- if (HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg1)))
+ if (HONOR_SNANS (arg1)
&& REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg1)))
return NULL_TREE;
@@ -5773,7 +5773,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type,
Note that all these transformations are correct if A is
NaN, since the two alternatives (A and -A) are also NaNs. */
- if (!HONOR_SIGNED_ZEROS (element_mode (type))
+ if (!HONOR_SIGNED_ZEROS (type)
&& (FLOAT_TYPE_P (TREE_TYPE (arg01))
? real_zerop (arg01)
: integer_zerop (arg01))
@@ -5842,7 +5842,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type,
both transformations are correct when A is NaN: A != 0
is then true, and A == 0 is false. */
- if (!HONOR_SIGNED_ZEROS (element_mode (type))
+ if (!HONOR_SIGNED_ZEROS (type)
&& integer_zerop (arg01) && integer_zerop (arg2))
{
if (comp_code == NE_EXPR)
@@ -5877,7 +5877,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type,
a number and A is not. The conditions in the original
expressions will be false, so all four give B. The min()
and max() versions would give a NaN instead. */
- if (!HONOR_SIGNED_ZEROS (element_mode (type))
+ if (!HONOR_SIGNED_ZEROS (type)
&& operand_equal_for_comparison_p (arg01, arg2)
/* Avoid these transformations if the COND_EXPR may be used
as an lvalue in the C++ front-end. PR c++/19199. */
@@ -11005,8 +11005,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
/* Fold __complex__ ( x, 0 ) + __complex__ ( 0, y )
to __complex__ ( x, y ). This is not the same for SNaNs or
if signed zeros are involved. */
- if (!HONOR_SNANS (element_mode (arg0))
- && !HONOR_SIGNED_ZEROS (element_mode (arg0))
+ if (!HONOR_SNANS (arg0)
+ && !HONOR_SIGNED_ZEROS (arg0)
&& COMPLEX_FLOAT_TYPE_P (TREE_TYPE (arg0)))
{
tree rtype = TREE_TYPE (TREE_TYPE (arg0));
@@ -11404,8 +11404,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
/* Fold __complex__ ( x, 0 ) - __complex__ ( 0, y ) to
__complex__ ( x, -y ). This is not the same for SNaNs or if
signed zeros are involved. */
- if (!HONOR_SNANS (element_mode (arg0))
- && !HONOR_SIGNED_ZEROS (element_mode (arg0))
+ if (!HONOR_SNANS (arg0)
+ && !HONOR_SIGNED_ZEROS (arg0)
&& COMPLEX_FLOAT_TYPE_P (TREE_TYPE (arg0)))
{
tree rtype = TREE_TYPE (TREE_TYPE (arg0));
@@ -11509,7 +11509,7 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
This is not the same for NaNs or if signed zeros are
involved. */
if (!HONOR_NANS (arg0)
- && !HONOR_SIGNED_ZEROS (element_mode (arg0))
+ && !HONOR_SIGNED_ZEROS (arg0)
&& COMPLEX_FLOAT_TYPE_P (TREE_TYPE (arg0))
&& TREE_CODE (arg1) == COMPLEX_CST
&& real_zerop (TREE_REALPART (arg1)))
@@ -12819,7 +12819,7 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type,
Also try swapping the arguments and inverting the conditional. */
if (COMPARISON_CLASS_P (arg0)
&& operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op1)
- && !HONOR_SIGNED_ZEROS (element_mode (op1)))
+ && !HONOR_SIGNED_ZEROS (op1))
{
tem = fold_cond_expr_with_comparison (loc, type, TREE_CODE (arg0),
TREE_OPERAND (arg0, 0),
@@ -12831,7 +12831,7 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type,
if (COMPARISON_CLASS_P (arg0)
&& operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op2)
- && !HONOR_SIGNED_ZEROS (element_mode (op2)))
+ && !HONOR_SIGNED_ZEROS (op2))
{
enum tree_code comp_code = TREE_CODE (arg0);
tree arg00 = TREE_OPERAND (arg0, 0);
@@ -14713,7 +14713,7 @@ tree_call_nonnegative_warnv_p (tree type, combined_fn fn, tree arg0, tree arg1,
CASE_CFN_SQRT:
CASE_CFN_SQRT_FN:
/* sqrt(-0.0) is -0.0. */
- if (!HONOR_SIGNED_ZEROS (element_mode (type)))
+ if (!HONOR_SIGNED_ZEROS (type))
return true;
return RECURSE (arg0);
diff --git a/gcc/match.pd b/gcc/match.pd
index 82052714e1c..4e10d54383c 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1458,8 +1458,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
/* -(A + B) -> (-B) - A. */
(simplify
(negate (plus:c @0 negate_expr_p@1))
- (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
- && !HONOR_SIGNED_ZEROS (element_mode (type)))
+ (if (!HONOR_SIGN_DEPENDENT_ROUNDING (type)
+ && !HONOR_SIGNED_ZEROS (type))
(minus (negate @1) @0)))
/* -(A - B) -> B - A. */
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
@ 2021-07-04 18:37 ` apinski
2021-07-05 11:26 ` Richard Biener
2021-07-04 18:37 ` [PATCH 4/5] Try inverted comparison for match_simplify in phiopt apinski
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: apinski @ 2021-07-04 18:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
From: Andrew Pinski <apinski@marvell.com>
To move a few things more to match-and-simplify from phiopt,
we need to allow match_simplify_replacement to run in early
phiopt. To do this we add a replacement for gimple_simplify
that is explictly for phiopt.
OK? Bootstrapped and tested on x86_64-linux-gnu with no
regressions.
gcc/ChangeLog:
* tree-ssa-phiopt.c (match_simplify_replacement):
Add early_p argument. Call gimple_simplify_phiopt
instead of gimple_simplify.
(tree_ssa_phiopt_worker): Update call to
match_simplify_replacement and allow unconditionally.
(phiopt_early_allow): New function.
(gimple_simplify_phiopt): New function.
---
gcc/tree-ssa-phiopt.c | 89 ++++++++++++++++++++++++++++++++++---------
1 file changed, 70 insertions(+), 19 deletions(-)
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 71f0019d877..d4449afcdca 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -50,13 +50,14 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-fold.h"
#include "internal-fn.h"
#include "gimple-range.h"
+#include "gimple-match.h"
#include "dbgcnt.h"
static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
tree, tree);
static bool match_simplify_replacement (basic_block, basic_block,
- edge, edge, gphi *, tree, tree);
+ edge, edge, gphi *, tree, tree, bool);
static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
gimple *);
static int value_replacement (basic_block, basic_block,
@@ -347,9 +348,9 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
/* Do the replacement of conditional if it can be done. */
if (!early_p && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
cfgchanged = true;
- else if (!early_p
- && match_simplify_replacement (bb, bb1, e1, e2, phi,
- arg0, arg1))
+ else if (match_simplify_replacement (bb, bb1, e1, e2, phi,
+ arg0, arg1,
+ early_p))
cfgchanged = true;
else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
cfgchanged = true;
@@ -819,6 +820,67 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
return true;
}
+/* Return TRUE if CODE should be allowed during early phiopt.
+ Currently this is to allow MIN/MAX and ABS/NEGATE. */
+static bool
+phiopt_early_allow (enum tree_code code)
+{
+ switch (code)
+ {
+ case MIN_EXPR:
+ case MAX_EXPR:
+ case ABS_EXPR:
+ case ABSU_EXPR:
+ case NEGATE_EXPR:
+ case SSA_NAME:
+ return true;
+ default:
+ return false;
+ }
+}
+
+/* gimple_simplify_phiopt is like gimple_simplify but designed for PHIOPT.
+ Return NULL if nothing can be simplified or the resulting simplified value
+ with parts pushed if EARLY_P was true. Also rejects non allowed tree code
+ if EARLY_P is set.
+ Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
+ to simplify CMP ? ARG0 : ARG1. */
+static tree
+gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
+ tree arg0, tree arg1,
+ gimple_seq *seq)
+{
+ tree result;
+ enum tree_code comp_code = gimple_cond_code (comp_stmt);
+ location_t loc = gimple_location (comp_stmt);
+ tree cmp0 = gimple_cond_lhs (comp_stmt);
+ tree cmp1 = gimple_cond_rhs (comp_stmt);
+ /* To handle special cases like floating point comparison, it is easier and
+ less error-prone to build a tree and gimplify it on the fly though it is
+ less efficient.
+ Don't use fold_build2 here as that might create (bool)a instead of just
+ "a != 0". */
+ tree cond = build2_loc (loc, comp_code, boolean_type_node,
+ cmp0, cmp1);
+ gimple_match_op op (gimple_match_cond::UNCOND,
+ COND_EXPR, type, cond, arg0, arg1);
+
+ if (op.resimplify (early_p ? NULL : seq, follow_all_ssa_edges))
+ {
+ /* Early we want only to allow some generated tree codes. */
+ if (!early_p
+ || op.code.is_tree_code ()
+ || phiopt_early_allow ((tree_code)op.code))
+ {
+ result = maybe_push_res_to_seq (&op, seq);
+ if (result)
+ return result;
+ }
+ }
+
+ return NULL;
+}
+
/* The function match_simplify_replacement does the main work of doing the
replacement using match and simplify. Return true if the replacement is done.
Otherwise return false.
@@ -828,10 +890,9 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
static bool
match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
edge e0, edge e1, gphi *phi,
- tree arg0, tree arg1)
+ tree arg0, tree arg1, bool early_p)
{
gimple *stmt;
- tree cond;
gimple_stmt_iterator gsi;
edge true_edge, false_edge;
gimple_seq seq = NULL;
@@ -892,15 +953,6 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
stmt = last_stmt (cond_bb);
- /* To handle special cases like floating point comparison, it is easier and
- less error-prone to build a tree and gimplify it on the fly though it is
- less efficient.
- Don't use fold_build2 here as that might create (bool)a instead of just
- "a != 0". */
- cond = build2_loc (gimple_location (stmt),
- gimple_cond_code (stmt), boolean_type_node,
- gimple_cond_lhs (stmt), gimple_cond_rhs (stmt));
-
/* We need to know which is the true edge and which is the false
edge so that we know when to invert the condition below. */
extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
@@ -908,10 +960,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
std::swap (arg0, arg1);
tree type = TREE_TYPE (gimple_phi_result (phi));
- result = gimple_simplify (COND_EXPR, type,
- cond,
- arg0, arg1,
- &seq, NULL);
+ result = gimple_simplify_phiopt (early_p, type, stmt,
+ arg0, arg1,
+ &seq);
if (!result)
return false;
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/5] Try inverted comparison for match_simplify in phiopt
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
2021-07-04 18:37 ` [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt apinski
@ 2021-07-04 18:37 ` apinski
2021-07-05 11:18 ` Richard Biener
2021-07-04 18:37 ` [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match apinski
2021-07-05 11:25 ` [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT Richard Biener
4 siblings, 1 reply; 11+ messages in thread
From: apinski @ 2021-07-04 18:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
From: Andrew Pinski <apinski@marvell.com>
Since match and simplify does not have all of the inverted
comparison patterns, it make sense to just have
phi-opt try to do the inversion and try match and simplify again.
OK? Bootstrapped and tested on x86_64-linux-gnu.
Thanks,
Andrew Pinski
gcc/ChangeLog:
* tree-ssa-phiopt.c (gimple_simplify_phiopt):
If "A ? B : C" fails to simplify, try "(!A) ? C : B".
---
gcc/tree-ssa-phiopt.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index d4449afcdca..fec8c02c062 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -844,7 +844,8 @@ phiopt_early_allow (enum tree_code code)
with parts pushed if EARLY_P was true. Also rejects non allowed tree code
if EARLY_P is set.
Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
- to simplify CMP ? ARG0 : ARG1. */
+ to simplify CMP ? ARG0 : ARG1.
+ Also try to simplify (!CMP) ? ARG1 : ARG0 if the non-inverse failed. */
static tree
gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
tree arg0, tree arg1,
@@ -877,6 +878,30 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
return result;
}
}
+ /* Try the inverted comparison, that is !COMP ? ARG1 : ARG0. */
+ comp_code = invert_tree_comparison (comp_code, HONOR_NANS (cmp0));
+
+ if (comp_code == ERROR_MARK)
+ return NULL;
+
+ cond = build2_loc (loc,
+ comp_code, boolean_type_node,
+ cmp0, cmp1);
+ gimple_match_op op1 (gimple_match_cond::UNCOND,
+ COND_EXPR, type, cond, arg1, arg0);
+
+ if (op1.resimplify (early_p ? NULL : seq, follow_all_ssa_edges))
+ {
+ /* Early we want only to allow some generated tree codes. */
+ if (!early_p
+ || op1.code.is_tree_code ()
+ || phiopt_early_allow ((tree_code)op1.code))
+ {
+ result = maybe_push_res_to_seq (&op1, seq);
+ if (result)
+ return result;
+ }
+ }
return NULL;
}
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
` (2 preceding siblings ...)
2021-07-04 18:37 ` [PATCH 4/5] Try inverted comparison for match_simplify in phiopt apinski
@ 2021-07-04 18:37 ` apinski
2021-07-05 11:26 ` Richard Biener
2021-07-05 11:25 ` [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT Richard Biener
4 siblings, 1 reply; 11+ messages in thread
From: apinski @ 2021-07-04 18:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
From: Andrew Pinski <apinski@marvell.com>
To improve phiopt and be able to remove abs_replacement, this ports
most of "A CMP 0 ? A : -A" from fold_cond_expr_with_comparison to
match.pd. There is a few extra changes that are needed to remove
the "A CMP 0 ? A : -A" part from fold_cond_expr_with_comparison:
* Need to handle (A - B) case
* Need to handle UN* comparisons.
I will handle those in a different patch.
Note phi-opt-15.c test needed to be updated as we get ABSU now
instead of not getting ABS. When ABSU was added phiopt was not
updated even to use ABSU instead of not creating ABS.
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
gcc/ChangeLog:
PR tree-optimization/101039
* match.pd (A CMP 0 ? A : -A): New patterns.
* tree-ssa-phiopt.c (abs_replacement): Delete function.
(tree_ssa_phiopt_worker): Don't call abs_replacement.
Update comment about abs_replacement.
gcc/testsuite/ChangeLog:
PR tree-optimization/101039
* gcc.dg/tree-ssa/phi-opt-15.c: Update test to expect
ABSU and still not expect ABS_EXPR.
* gcc.dg/tree-ssa/phi-opt-23.c: New test.
* gcc.dg/tree-ssa/phi-opt-24.c: New test.
---
gcc/match.pd | 60 +++++++++
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c | 4 +-
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c | 44 +++++++
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c | 44 +++++++
gcc/tree-ssa-phiopt.c | 134 +--------------------
5 files changed, 152 insertions(+), 134 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
diff --git a/gcc/match.pd b/gcc/match.pd
index 4e10d54383c..72860fbd448 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3976,6 +3976,66 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(cnd (logical_inverted_value truth_valued_p@0) @1 @2)
(cnd @0 @2 @1)))
+/* abs/negative simplifications moved from fold_cond_expr_with_comparison,
+ Need to handle (A - B) case as fold_cond_expr_with_comparison does.
+ Need to handle UN* comparisons.
+
+ None of these transformations work for modes with signed
+ zeros. If A is +/-0, the first two transformations will
+ change the sign of the result (from +0 to -0, or vice
+ versa). The last four will fix the sign of the result,
+ even though the original expressions could be positive or
+ negative, depending on the sign of A.
+
+ Note that all these transformations are correct if A is
+ NaN, since the two alternatives (A and -A) are also NaNs. */
+
+(for cnd (cond vec_cond)
+ /* A == 0 ? A : -A same as -A */
+ (for cmp (eq uneq)
+ (simplify
+ (cnd (cmp @0 zerop) @0 (negate@1 @0))
+ (if (!HONOR_SIGNED_ZEROS (type))
+ @1))
+ (simplify
+ (cnd (cmp @0 zerop) integer_zerop (negate@1 @0))
+ (if (!HONOR_SIGNED_ZEROS (type))
+ @1))
+ )
+ /* A != 0 ? A : -A same as A */
+ (for cmp (ne ltgt)
+ (simplify
+ (cnd (cmp @0 zerop) @0 (negate @0))
+ (if (!HONOR_SIGNED_ZEROS (type))
+ @0))
+ (simplify
+ (cnd (cmp @0 zerop) @0 integer_zerop)
+ (if (!HONOR_SIGNED_ZEROS (type))
+ @0))
+ )
+ /* A >=/> 0 ? A : -A same as abs (A) */
+ (for cmp (ge gt)
+ (simplify
+ (cnd (cmp @0 zerop) @0 (negate @0))
+ (if (!HONOR_SIGNED_ZEROS (type)
+ && !TYPE_UNSIGNED (type))
+ (abs @0))))
+ /* A <=/< 0 ? A : -A same as -abs (A) */
+ (for cmp (le lt)
+ (simplify
+ (cnd (cmp @0 zerop) @0 (negate @0))
+ (if (!HONOR_SIGNED_ZEROS (type)
+ && !TYPE_UNSIGNED (type))
+ (if (ANY_INTEGRAL_TYPE_P (type)
+ && !TYPE_OVERFLOW_WRAPS (type))
+ (with {
+ tree utype = unsigned_type_for (type);
+ }
+ (convert (negate (absu:utype @0))))
+ (negate (abs @0)))))
+ )
+)
+
/* -(type)!A -> (type)A - 1. */
(simplify
(negate (convert?:s (logical_inverted_value:s @0)))
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
index ac3018ef533..6aec68961cf 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
@@ -9,4 +9,6 @@ foo (int i)
return i;
}
-/* { dg-final { scan-tree-dump-not "ABS" "optimized" } } */
+/* We should not have ABS_EXPR but ABSU_EXPR instead. */
+/* { dg-final { scan-tree-dump-not "ABS_EXPR" "optimized" } } */
+/* { dg-final { scan-tree-dump "ABSU" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
new file mode 100644
index 00000000000..ff658cd16a7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
@@ -0,0 +1,44 @@
+/* { dg-options "-O2 -fdump-tree-phiopt" } */
+
+int f0(int A)
+{
+// A == 0? A : -A same as -A
+ if (A == 0) return A;
+ return -A;
+}
+
+int f1(int A)
+{
+// A != 0? A : -A same as A
+ if (A != 0) return A;
+ return -A;
+}
+int f2(int A)
+{
+// A >= 0? A : -A same as abs (A)
+ if (A >= 0) return A;
+ return -A;
+}
+int f3(int A)
+{
+// A > 0? A : -A same as abs (A)
+ if (A > 0) return A;
+ return -A;
+}
+int f4(int A)
+{
+// A <= 0? A : -A same as -abs (A)
+ if (A <= 0) return A;
+ return -A;
+}
+int f5(int A)
+{
+// A < 0? A : -A same as -abs (A)
+ if (A < 0) return A;
+ return -A;
+}
+
+/* These should be optimized in phiopt1 but is confused by predicts. */
+/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
+/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
+
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
new file mode 100644
index 00000000000..eb89decb4bf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
@@ -0,0 +1,44 @@
+/* { dg-options "-O2 -fno-signed-zeros -fdump-tree-phiopt" } */
+
+float f0(float A)
+{
+// A == 0? A : -A same as -A
+ if (A == 0) return A;
+ return -A;
+}
+
+float f1(float A)
+{
+// A != 0? A : -A same as A
+ if (A != 0) return A;
+ return -A;
+}
+float f2(float A)
+{
+// A >= 0? A : -A same as abs (A)
+ if (A >= 0) return A;
+ return -A;
+}
+float f3(float A)
+{
+// A > 0? A : -A same as abs (A)
+ if (A > 0) return A;
+ return -A;
+}
+float f4(float A)
+{
+// A <= 0? A : -A same as -abs (A)
+ if (A <= 0) return A;
+ return -A;
+}
+float f5(float A)
+{
+// A < 0? A : -A same as -abs (A)
+ if (A < 0) return A;
+ return -A;
+}
+
+/* These should be optimized in phiopt1 but is confused by predicts. */
+/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
+/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
+
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index fec8c02c062..a1664c1f914 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -64,8 +64,6 @@ static int value_replacement (basic_block, basic_block,
edge, edge, gphi *, tree, tree);
static bool minmax_replacement (basic_block, basic_block,
edge, edge, gphi *, tree, tree);
-static bool abs_replacement (basic_block, basic_block,
- edge, edge, gphi *, tree, tree);
static bool spaceship_replacement (basic_block, basic_block,
edge, edge, gphi *, tree, tree);
static bool cond_removal_in_popcount_clz_ctz_pattern (basic_block, basic_block,
@@ -352,8 +350,6 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
arg0, arg1,
early_p))
cfgchanged = true;
- else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
- cfgchanged = true;
else if (!early_p
&& cond_removal_in_popcount_clz_ctz_pattern (bb, bb1, e1,
e2, phi, arg0,
@@ -2614,134 +2610,6 @@ cond_removal_in_popcount_clz_ctz_pattern (basic_block cond_bb,
return true;
}
-/* The function absolute_replacement does the main work of doing the absolute
- replacement. Return true if the replacement is done. Otherwise return
- false.
- bb is the basic block where the replacement is going to be done on. arg0
- is argument 0 from the phi. Likewise for arg1. */
-
-static bool
-abs_replacement (basic_block cond_bb, basic_block middle_bb,
- edge e0 ATTRIBUTE_UNUSED, edge e1,
- gphi *phi, tree arg0, tree arg1)
-{
- tree result;
- gassign *new_stmt;
- gimple *cond;
- gimple_stmt_iterator gsi;
- edge true_edge, false_edge;
- gimple *assign;
- edge e;
- tree rhs, lhs;
- bool negate;
- enum tree_code cond_code;
-
- /* If the type says honor signed zeros we cannot do this
- optimization. */
- if (HONOR_SIGNED_ZEROS (arg1))
- return false;
-
- /* OTHER_BLOCK must have only one executable statement which must have the
- form arg0 = -arg1 or arg1 = -arg0. */
-
- assign = last_and_only_stmt (middle_bb);
- /* If we did not find the proper negation assignment, then we cannot
- optimize. */
- if (assign == NULL)
- return false;
-
- /* If we got here, then we have found the only executable statement
- in OTHER_BLOCK. If it is anything other than arg = -arg1 or
- arg1 = -arg0, then we cannot optimize. */
- if (gimple_code (assign) != GIMPLE_ASSIGN)
- return false;
-
- lhs = gimple_assign_lhs (assign);
-
- if (gimple_assign_rhs_code (assign) != NEGATE_EXPR)
- return false;
-
- rhs = gimple_assign_rhs1 (assign);
-
- /* The assignment has to be arg0 = -arg1 or arg1 = -arg0. */
- if (!(lhs == arg0 && rhs == arg1)
- && !(lhs == arg1 && rhs == arg0))
- return false;
-
- cond = last_stmt (cond_bb);
- result = PHI_RESULT (phi);
-
- /* Only relationals comparing arg[01] against zero are interesting. */
- cond_code = gimple_cond_code (cond);
- if (cond_code != GT_EXPR && cond_code != GE_EXPR
- && cond_code != LT_EXPR && cond_code != LE_EXPR)
- return false;
-
- /* Make sure the conditional is arg[01] OP y. */
- if (gimple_cond_lhs (cond) != rhs)
- return false;
-
- if (FLOAT_TYPE_P (TREE_TYPE (gimple_cond_rhs (cond)))
- ? real_zerop (gimple_cond_rhs (cond))
- : integer_zerop (gimple_cond_rhs (cond)))
- ;
- else
- return false;
-
- /* We need to know which is the true edge and which is the false
- edge so that we know if have abs or negative abs. */
- extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
-
- /* For GT_EXPR/GE_EXPR, if the true edge goes to OTHER_BLOCK, then we
- will need to negate the result. Similarly for LT_EXPR/LE_EXPR if
- the false edge goes to OTHER_BLOCK. */
- if (cond_code == GT_EXPR || cond_code == GE_EXPR)
- e = true_edge;
- else
- e = false_edge;
-
- if (e->dest == middle_bb)
- negate = true;
- else
- negate = false;
-
- /* If the code negates only iff positive then make sure to not
- introduce undefined behavior when negating or computing the absolute.
- ??? We could use range info if present to check for arg1 == INT_MIN. */
- if (negate
- && (ANY_INTEGRAL_TYPE_P (TREE_TYPE (arg1))
- && ! TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg1))))
- return false;
-
- result = duplicate_ssa_name (result, NULL);
-
- if (negate)
- lhs = make_ssa_name (TREE_TYPE (result));
- else
- lhs = result;
-
- /* Build the modify expression with abs expression. */
- new_stmt = gimple_build_assign (lhs, ABS_EXPR, rhs);
-
- gsi = gsi_last_bb (cond_bb);
- gsi_insert_before (&gsi, new_stmt, GSI_NEW_STMT);
-
- if (negate)
- {
- /* Get the right GSI. We want to insert after the recently
- added ABS_EXPR statement (which we know is the first statement
- in the block. */
- new_stmt = gimple_build_assign (result, NEGATE_EXPR, lhs);
-
- gsi_insert_after (&gsi, new_stmt, GSI_NEW_STMT);
- }
-
- replace_phi_edge_with_variable (cond_bb, e1, phi, result);
-
- /* Note that we optimized this PHI. */
- return true;
-}
-
/* Auxiliary functions to determine the set of memory accesses which
can't trap because they are preceded by accesses to the same memory
portion. We do that for MEM_REFs, so we only need to track
@@ -3678,7 +3546,7 @@ gate_hoist_loads (void)
ABS Replacement
---------------
- This transformation, implemented in abs_replacement, replaces
+ This transformation, implemented in match_simplify_replacement, replaces
bb0:
if (a >= 0) goto bb2; else goto bb1;
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/5] Try inverted comparison for match_simplify in phiopt
2021-07-04 18:37 ` [PATCH 4/5] Try inverted comparison for match_simplify in phiopt apinski
@ 2021-07-05 11:18 ` Richard Biener
0 siblings, 0 replies; 11+ messages in thread
From: Richard Biener @ 2021-07-05 11:18 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sun, Jul 4, 2021 at 8:38 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> Since match and simplify does not have all of the inverted
> comparison patterns, it make sense to just have
> phi-opt try to do the inversion and try match and simplify again.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu.
OK.
> Thanks,
> Andrew Pinski
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.c (gimple_simplify_phiopt):
> If "A ? B : C" fails to simplify, try "(!A) ? C : B".
> ---
> gcc/tree-ssa-phiopt.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index d4449afcdca..fec8c02c062 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -844,7 +844,8 @@ phiopt_early_allow (enum tree_code code)
> with parts pushed if EARLY_P was true. Also rejects non allowed tree code
> if EARLY_P is set.
> Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
> - to simplify CMP ? ARG0 : ARG1. */
> + to simplify CMP ? ARG0 : ARG1.
> + Also try to simplify (!CMP) ? ARG1 : ARG0 if the non-inverse failed. */
> static tree
> gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
> tree arg0, tree arg1,
> @@ -877,6 +878,30 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
> return result;
> }
> }
> + /* Try the inverted comparison, that is !COMP ? ARG1 : ARG0. */
> + comp_code = invert_tree_comparison (comp_code, HONOR_NANS (cmp0));
> +
> + if (comp_code == ERROR_MARK)
> + return NULL;
> +
> + cond = build2_loc (loc,
> + comp_code, boolean_type_node,
> + cmp0, cmp1);
> + gimple_match_op op1 (gimple_match_cond::UNCOND,
> + COND_EXPR, type, cond, arg1, arg0);
> +
> + if (op1.resimplify (early_p ? NULL : seq, follow_all_ssa_edges))
> + {
> + /* Early we want only to allow some generated tree codes. */
> + if (!early_p
> + || op1.code.is_tree_code ()
> + || phiopt_early_allow ((tree_code)op1.code))
> + {
> + result = maybe_push_res_to_seq (&op1, seq);
> + if (result)
> + return result;
> + }
> + }
>
> return NULL;
> }
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real
2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
@ 2021-07-05 11:18 ` Richard Biener
0 siblings, 0 replies; 11+ messages in thread
From: Richard Biener @ 2021-07-05 11:18 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sun, Jul 4, 2021 at 8:39 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> HONOR_SIGNED_ZEROS, HONOR_SIGN_DEPENDENT_ROUNDING, and HONOR_SNANS all
> have an overload for taking a tree type now, so we should do that instead.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu.
OK.
Thanks,
Richard.
> gcc/ChangeLog:
>
> PR middle-end/101237
> * fold-const.c (negate_expr_p): Remove call to element_mode
> and TREE_MODE/TREE_TYPE when calling HONOR_SIGNED_ZEROS,
> HONOR_SIGN_DEPENDENT_ROUNDING, and HONOR_SNANS.
> (fold_negate_expr_1): Likewise.
> (const_unop): Likewise.
> (fold_cond_expr_with_comparison): Likewise.
> (fold_binary_loc): Likewise.
> (fold_ternary_loc): Likewise.
> (tree_call_nonnegative_warnv_p): Likewise.
> * match.pd (-(A + B) -> (-B) - A): Likewise.
> ---
> gcc/fold-const.c | 46 +++++++++++++++++++++++-----------------------
> gcc/match.pd | 4 ++--
> 2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index dfccbaec683..e0cdb75fb26 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -432,8 +432,8 @@ negate_expr_p (tree t)
> return negate_expr_p (TREE_OPERAND (t, 0));
>
> case PLUS_EXPR:
> - if (HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> - || HONOR_SIGNED_ZEROS (element_mode (type))
> + if (HONOR_SIGN_DEPENDENT_ROUNDING (type)
> + || HONOR_SIGNED_ZEROS (type)
> || (ANY_INTEGRAL_TYPE_P (type)
> && ! TYPE_OVERFLOW_WRAPS (type)))
> return false;
> @@ -445,8 +445,8 @@ negate_expr_p (tree t)
>
> case MINUS_EXPR:
> /* We can't turn -(A-B) into B-A when we honor signed zeros. */
> - return !HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> - && !HONOR_SIGNED_ZEROS (element_mode (type))
> + return !HONOR_SIGN_DEPENDENT_ROUNDING (type)
> + && !HONOR_SIGNED_ZEROS (type)
> && (! ANY_INTEGRAL_TYPE_P (type)
> || TYPE_OVERFLOW_WRAPS (type));
>
> @@ -468,7 +468,7 @@ negate_expr_p (tree t)
> /* Fall through. */
>
> case RDIV_EXPR:
> - if (! HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (TREE_TYPE (t))))
> + if (! HONOR_SIGN_DEPENDENT_ROUNDING (t))
> return negate_expr_p (TREE_OPERAND (t, 1))
> || negate_expr_p (TREE_OPERAND (t, 0));
> break;
> @@ -605,8 +605,8 @@ fold_negate_expr_1 (location_t loc, tree t)
> break;
>
> case PLUS_EXPR:
> - if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> - && !HONOR_SIGNED_ZEROS (element_mode (type)))
> + if (!HONOR_SIGN_DEPENDENT_ROUNDING (type)
> + && !HONOR_SIGNED_ZEROS (type))
> {
> /* -(A + B) -> (-B) - A. */
> if (negate_expr_p (TREE_OPERAND (t, 1)))
> @@ -628,8 +628,8 @@ fold_negate_expr_1 (location_t loc, tree t)
>
> case MINUS_EXPR:
> /* - (A - B) -> B - A */
> - if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> - && !HONOR_SIGNED_ZEROS (element_mode (type)))
> + if (!HONOR_SIGN_DEPENDENT_ROUNDING (type)
> + && !HONOR_SIGNED_ZEROS (type))
> return fold_build2_loc (loc, MINUS_EXPR, type,
> TREE_OPERAND (t, 1), TREE_OPERAND (t, 0));
> break;
> @@ -641,7 +641,7 @@ fold_negate_expr_1 (location_t loc, tree t)
> /* Fall through. */
>
> case RDIV_EXPR:
> - if (! HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type)))
> + if (! HONOR_SIGN_DEPENDENT_ROUNDING (type))
> {
> tem = TREE_OPERAND (t, 1);
> if (negate_expr_p (tem))
> @@ -1725,7 +1725,7 @@ const_unop (enum tree_code code, tree type, tree arg0)
> /* Don't perform the operation, other than NEGATE and ABS, if
> flag_signaling_nans is on and the operand is a signaling NaN. */
> if (TREE_CODE (arg0) == REAL_CST
> - && HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg0)))
> + && HONOR_SNANS (arg0)
> && REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg0))
> && code != NEGATE_EXPR
> && code != ABS_EXPR
> @@ -2135,7 +2135,7 @@ fold_convert_const_real_from_real (tree type, const_tree arg1)
>
> /* Don't perform the operation if flag_signaling_nans is on
> and the operand is a signaling NaN. */
> - if (HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg1)))
> + if (HONOR_SNANS (arg1)
> && REAL_VALUE_ISSIGNALING_NAN (TREE_REAL_CST (arg1)))
> return NULL_TREE;
>
> @@ -5773,7 +5773,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type,
>
> Note that all these transformations are correct if A is
> NaN, since the two alternatives (A and -A) are also NaNs. */
> - if (!HONOR_SIGNED_ZEROS (element_mode (type))
> + if (!HONOR_SIGNED_ZEROS (type)
> && (FLOAT_TYPE_P (TREE_TYPE (arg01))
> ? real_zerop (arg01)
> : integer_zerop (arg01))
> @@ -5842,7 +5842,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type,
> both transformations are correct when A is NaN: A != 0
> is then true, and A == 0 is false. */
>
> - if (!HONOR_SIGNED_ZEROS (element_mode (type))
> + if (!HONOR_SIGNED_ZEROS (type)
> && integer_zerop (arg01) && integer_zerop (arg2))
> {
> if (comp_code == NE_EXPR)
> @@ -5877,7 +5877,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type,
> a number and A is not. The conditions in the original
> expressions will be false, so all four give B. The min()
> and max() versions would give a NaN instead. */
> - if (!HONOR_SIGNED_ZEROS (element_mode (type))
> + if (!HONOR_SIGNED_ZEROS (type)
> && operand_equal_for_comparison_p (arg01, arg2)
> /* Avoid these transformations if the COND_EXPR may be used
> as an lvalue in the C++ front-end. PR c++/19199. */
> @@ -11005,8 +11005,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
> /* Fold __complex__ ( x, 0 ) + __complex__ ( 0, y )
> to __complex__ ( x, y ). This is not the same for SNaNs or
> if signed zeros are involved. */
> - if (!HONOR_SNANS (element_mode (arg0))
> - && !HONOR_SIGNED_ZEROS (element_mode (arg0))
> + if (!HONOR_SNANS (arg0)
> + && !HONOR_SIGNED_ZEROS (arg0)
> && COMPLEX_FLOAT_TYPE_P (TREE_TYPE (arg0)))
> {
> tree rtype = TREE_TYPE (TREE_TYPE (arg0));
> @@ -11404,8 +11404,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
> /* Fold __complex__ ( x, 0 ) - __complex__ ( 0, y ) to
> __complex__ ( x, -y ). This is not the same for SNaNs or if
> signed zeros are involved. */
> - if (!HONOR_SNANS (element_mode (arg0))
> - && !HONOR_SIGNED_ZEROS (element_mode (arg0))
> + if (!HONOR_SNANS (arg0)
> + && !HONOR_SIGNED_ZEROS (arg0)
> && COMPLEX_FLOAT_TYPE_P (TREE_TYPE (arg0)))
> {
> tree rtype = TREE_TYPE (TREE_TYPE (arg0));
> @@ -11509,7 +11509,7 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type,
> This is not the same for NaNs or if signed zeros are
> involved. */
> if (!HONOR_NANS (arg0)
> - && !HONOR_SIGNED_ZEROS (element_mode (arg0))
> + && !HONOR_SIGNED_ZEROS (arg0)
> && COMPLEX_FLOAT_TYPE_P (TREE_TYPE (arg0))
> && TREE_CODE (arg1) == COMPLEX_CST
> && real_zerop (TREE_REALPART (arg1)))
> @@ -12819,7 +12819,7 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type,
> Also try swapping the arguments and inverting the conditional. */
> if (COMPARISON_CLASS_P (arg0)
> && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op1)
> - && !HONOR_SIGNED_ZEROS (element_mode (op1)))
> + && !HONOR_SIGNED_ZEROS (op1))
> {
> tem = fold_cond_expr_with_comparison (loc, type, TREE_CODE (arg0),
> TREE_OPERAND (arg0, 0),
> @@ -12831,7 +12831,7 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type,
>
> if (COMPARISON_CLASS_P (arg0)
> && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op2)
> - && !HONOR_SIGNED_ZEROS (element_mode (op2)))
> + && !HONOR_SIGNED_ZEROS (op2))
> {
> enum tree_code comp_code = TREE_CODE (arg0);
> tree arg00 = TREE_OPERAND (arg0, 0);
> @@ -14713,7 +14713,7 @@ tree_call_nonnegative_warnv_p (tree type, combined_fn fn, tree arg0, tree arg1,
> CASE_CFN_SQRT:
> CASE_CFN_SQRT_FN:
> /* sqrt(-0.0) is -0.0. */
> - if (!HONOR_SIGNED_ZEROS (element_mode (type)))
> + if (!HONOR_SIGNED_ZEROS (type))
> return true;
> return RECURSE (arg0);
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 82052714e1c..4e10d54383c 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -1458,8 +1458,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> /* -(A + B) -> (-B) - A. */
> (simplify
> (negate (plus:c @0 negate_expr_p@1))
> - (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> - && !HONOR_SIGNED_ZEROS (element_mode (type)))
> + (if (!HONOR_SIGN_DEPENDENT_ROUNDING (type)
> + && !HONOR_SIGNED_ZEROS (type))
> (minus (negate @1) @0)))
>
> /* -(A - B) -> B - A. */
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
` (3 preceding siblings ...)
2021-07-04 18:37 ` [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match apinski
@ 2021-07-05 11:25 ` Richard Biener
2021-07-05 22:54 ` Andrew Pinski
4 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2021-07-05 11:25 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sun, Jul 4, 2021 at 8:40 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> So the problem here is that replace_phi_edge_with_variable
> will copy range information to a already (not newly) defined
> ssa name. This causes wrong code later on.
That's a bit too conservative I guess? Shouldn't it work for at least
all defs defined in the same block as the original conditional (and
thus then applying to the seq inserted there by the callers)?
I realize it's wrong for, say
_1 = ..
if (_1 != 0)
{
...
if (..)
;
# _2 = PHI <_1, 1>
...
}
with _2 having range [1, +INF] but clearly not _1 at the point of its
definition.
Richard.
> This patch fixes the problem by requiring there to be statements
> that are to be placed before the conditional to be able to
> copy the range info; this assumes the statements will define
> the ssa name.
>
> gcc/ChangeLog:
>
> PR tree-optimization/101256
> * dbgcnt.def (phiopt_edge_range): New counter.
> * tree-ssa-phiopt.c (replace_phi_edge_with_variable):
> Add optional sequence which will be added before the old
> conditional. Check sequence for non-null if we want to
> update the range.
> (two_value_replacement): Instead of inserting the sequence,
> update the call to replace_phi_edge_with_variable.
> (match_simplify_replacement): Likewise.
> (minmax_replacement): Likewise.
> (value_replacement): Create a sequence of statements
> which would have defined the ssa name. Update call
> to replace_phi_edge_with_variable.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/101256
> * g++.dg/torture/pr101256.C: New test.
> ---
> gcc/dbgcnt.def | 1 +
> gcc/testsuite/g++.dg/torture/pr101256.C | 28 +++++++++++++
> gcc/tree-ssa-phiopt.c | 52 ++++++++++++++-----------
> 3 files changed, 59 insertions(+), 22 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/pr101256.C
>
> diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
> index 93e7b4fd30e..2345899ba68 100644
> --- a/gcc/dbgcnt.def
> +++ b/gcc/dbgcnt.def
> @@ -183,6 +183,7 @@ DEBUG_COUNTER (lim)
> DEBUG_COUNTER (local_alloc_for_sched)
> DEBUG_COUNTER (match)
> DEBUG_COUNTER (merged_ipa_icf)
> +DEBUG_COUNTER (phiopt_edge_range)
> DEBUG_COUNTER (postreload_cse)
> DEBUG_COUNTER (pre)
> DEBUG_COUNTER (pre_insn)
> diff --git a/gcc/testsuite/g++.dg/torture/pr101256.C b/gcc/testsuite/g++.dg/torture/pr101256.C
> new file mode 100644
> index 00000000000..973a8b4caf3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr101256.C
> @@ -0,0 +1,28 @@
> +// { dg-do run }
> +
> +template<class T>
> +const T& max(const T& a, const T& b)
> +{
> + return (a < b) ? b : a;
> +}
> +
> +signed char var_5 = -128;
> +unsigned int var_11 = 2144479212U;
> +unsigned long long int arr [22];
> +
> +void
> +__attribute__((noipa))
> +test(signed char var_5, unsigned var_11) {
> + for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
> + arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
> +}
> +
> +int main() {
> + for (int i_0 = 0; i_0 < 22; ++i_0)
> + arr [i_0] = 11834725929543695741ULL;
> +
> + test(var_5, var_11);
> + if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
> + __builtin_abort ();
> + return 0;
> +}
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index ab12e85569d..71f0019d877 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3. If not see
> #include "gimple-fold.h"
> #include "internal-fn.h"
> #include "gimple-range.h"
> +#include "dbgcnt.h"
>
> static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
> static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
> @@ -73,7 +74,8 @@ static bool cond_store_replacement (basic_block, basic_block, edge, edge,
> hash_set<tree> *);
> static bool cond_if_else_store_replacement (basic_block, basic_block, basic_block);
> static hash_set<tree> * get_non_trapping ();
> -static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree);
> +static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree,
> + gimple_seq = NULL);
> static void hoist_adjacent_loads (basic_block, basic_block,
> basic_block, basic_block);
> static bool gate_hoist_loads (void);
> @@ -382,18 +384,20 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>
> /* Replace PHI node element whose edge is E in block BB with variable NEW.
> Remove the edge from COND_BLOCK which does not lead to BB (COND_BLOCK
> - is known to have two edges, one of which must reach BB). */
> + is known to have two edges, one of which must reach BB).
> + Optionally insert stmts before the old condition. */
>
> static void
> replace_phi_edge_with_variable (basic_block cond_block,
> - edge e, gphi *phi, tree new_tree)
> + edge e, gphi *phi, tree new_tree,
> + gimple_seq stmts)
> {
> basic_block bb = gimple_bb (phi);
> basic_block block_to_remove;
> gimple_stmt_iterator gsi;
> tree phi_result = PHI_RESULT (phi);
>
> - /* Duplicate range info if we're the only things setting the target PHI.
> + /* Duplicate range info if they are the only things setting the target PHI.
> This is needed as later on, the new_tree will be replacing
> The assignement of the PHI.
> For an example:
> @@ -401,19 +405,23 @@ replace_phi_edge_with_variable (basic_block cond_block,
> _4 = min<a_1, 255>
> goto bb2
>
> - range<-INF,255>
> + # RANGE [-INF, 255]
> a_3 = PHI<_4(1)>
> bb3:
>
> use(a_3)
> - And _4 gets prograted into the use of a_3 and losing the range info.
> - This can't be done for more than 2 incoming edges as the progration
> - won't happen. */
> + And _4 gets propagated into the use of a_3 and losing the range info.
> + This can't be done for more than 2 incoming edges as the propagation
> + won't happen.
> + This also cannot be done if the name is not defined by statements to
> + be placed before the conditional. */
> if (TREE_CODE (new_tree) == SSA_NAME
> && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> && INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
> && !SSA_NAME_RANGE_INFO (new_tree)
> - && SSA_NAME_RANGE_INFO (phi_result))
> + && SSA_NAME_RANGE_INFO (phi_result)
> + && stmts != NULL
> + && dbg_cnt (phiopt_edge_range))
> duplicate_ssa_name_range_info (new_tree,
> SSA_NAME_RANGE_TYPE (phi_result),
> SSA_NAME_RANGE_INFO (phi_result));
> @@ -443,6 +451,8 @@ replace_phi_edge_with_variable (basic_block cond_block,
>
> /* Eliminate the COND_EXPR at the end of COND_BLOCK. */
> gsi = gsi_last_bb (cond_block);
> + if (stmts)
> + gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
> gsi_remove (&gsi, true);
>
> statistics_counter_event (cfun, "Replace PHI with variable", 1);
> @@ -802,10 +812,8 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> else
> new_rhs = gimple_build (&stmts, MINUS_EXPR, type, arg, lhs);
> new_rhs = gimple_convert (&stmts, TREE_TYPE (arg0), new_rhs);
> - gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> - gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
>
> - replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs);
> + replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs, stmts);
>
> /* Note that we optimized this PHI. */
> return true;
> @@ -920,10 +928,8 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> gsi_move_before (&gsi1, &gsi);
> reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
> }
> - if (seq)
> - gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
>
> - replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> + replace_phi_edge_with_variable (cond_bb, e1, phi, result, seq);
>
> /* Add Statistic here even though replace_phi_edge_with_variable already
> does it as we want to be able to count when match-simplify happens vs
> @@ -1396,6 +1402,7 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> && absorbing_element_p (code_def,
> cond_rhs, false, rhs2))))))
> {
> + gimple_seq stmts = NULL;
> gsi = gsi_for_stmt (cond);
> /* Moving ASSIGN might change VR of lhs, e.g. when moving u_6
> def-stmt in:
> @@ -1417,11 +1424,15 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> tree plhs = gimple_assign_lhs (prep_stmt[i]);
> reset_flow_sensitive_info (plhs);
> gsi_from = gsi_for_stmt (prep_stmt[i]);
> - gsi_move_before (&gsi_from, &gsi);
> + gsi_remove (&gsi_from, false);
> +
> + gimple_seq_add_stmt (&stmts, prep_stmt[i]);
> }
> gsi_from = gsi_for_stmt (assign);
> - gsi_move_before (&gsi_from, &gsi);
> - replace_phi_edge_with_variable (cond_bb, e1, phi, lhs);
> + gsi_remove (&gsi_from, false);
> + gimple_seq_add_stmt (&stmts, assign);
> +
> + replace_phi_edge_with_variable (cond_bb, e1, phi, lhs, stmts);
> return 2;
> }
>
> @@ -1811,10 +1822,7 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
> tree phi_result = PHI_RESULT (phi);
> result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
>
> - gsi = gsi_last_bb (cond_bb);
> - gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
> -
> - replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> + replace_phi_edge_with_variable (cond_bb, e1, phi, result, stmts);
>
> return true;
> }
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt
2021-07-04 18:37 ` [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt apinski
@ 2021-07-05 11:26 ` Richard Biener
0 siblings, 0 replies; 11+ messages in thread
From: Richard Biener @ 2021-07-05 11:26 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sun, Jul 4, 2021 at 8:41 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> To move a few things more to match-and-simplify from phiopt,
> we need to allow match_simplify_replacement to run in early
> phiopt. To do this we add a replacement for gimple_simplify
> that is explictly for phiopt.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no
> regressions.
OK.
Richard.
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.c (match_simplify_replacement):
> Add early_p argument. Call gimple_simplify_phiopt
> instead of gimple_simplify.
> (tree_ssa_phiopt_worker): Update call to
> match_simplify_replacement and allow unconditionally.
> (phiopt_early_allow): New function.
> (gimple_simplify_phiopt): New function.
> ---
> gcc/tree-ssa-phiopt.c | 89 ++++++++++++++++++++++++++++++++++---------
> 1 file changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 71f0019d877..d4449afcdca 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -50,13 +50,14 @@ along with GCC; see the file COPYING3. If not see
> #include "gimple-fold.h"
> #include "internal-fn.h"
> #include "gimple-range.h"
> +#include "gimple-match.h"
> #include "dbgcnt.h"
>
> static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
> static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
> tree, tree);
> static bool match_simplify_replacement (basic_block, basic_block,
> - edge, edge, gphi *, tree, tree);
> + edge, edge, gphi *, tree, tree, bool);
> static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
> gimple *);
> static int value_replacement (basic_block, basic_block,
> @@ -347,9 +348,9 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
> /* Do the replacement of conditional if it can be done. */
> if (!early_p && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
> cfgchanged = true;
> - else if (!early_p
> - && match_simplify_replacement (bb, bb1, e1, e2, phi,
> - arg0, arg1))
> + else if (match_simplify_replacement (bb, bb1, e1, e2, phi,
> + arg0, arg1,
> + early_p))
> cfgchanged = true;
> else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> cfgchanged = true;
> @@ -819,6 +820,67 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> return true;
> }
>
> +/* Return TRUE if CODE should be allowed during early phiopt.
> + Currently this is to allow MIN/MAX and ABS/NEGATE. */
> +static bool
> +phiopt_early_allow (enum tree_code code)
> +{
> + switch (code)
> + {
> + case MIN_EXPR:
> + case MAX_EXPR:
> + case ABS_EXPR:
> + case ABSU_EXPR:
> + case NEGATE_EXPR:
> + case SSA_NAME:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +/* gimple_simplify_phiopt is like gimple_simplify but designed for PHIOPT.
> + Return NULL if nothing can be simplified or the resulting simplified value
> + with parts pushed if EARLY_P was true. Also rejects non allowed tree code
> + if EARLY_P is set.
> + Takes the comparison from COMP_STMT and two args, ARG0 and ARG1 and tries
> + to simplify CMP ? ARG0 : ARG1. */
> +static tree
> +gimple_simplify_phiopt (bool early_p, tree type, gimple *comp_stmt,
> + tree arg0, tree arg1,
> + gimple_seq *seq)
> +{
> + tree result;
> + enum tree_code comp_code = gimple_cond_code (comp_stmt);
> + location_t loc = gimple_location (comp_stmt);
> + tree cmp0 = gimple_cond_lhs (comp_stmt);
> + tree cmp1 = gimple_cond_rhs (comp_stmt);
> + /* To handle special cases like floating point comparison, it is easier and
> + less error-prone to build a tree and gimplify it on the fly though it is
> + less efficient.
> + Don't use fold_build2 here as that might create (bool)a instead of just
> + "a != 0". */
> + tree cond = build2_loc (loc, comp_code, boolean_type_node,
> + cmp0, cmp1);
> + gimple_match_op op (gimple_match_cond::UNCOND,
> + COND_EXPR, type, cond, arg0, arg1);
> +
> + if (op.resimplify (early_p ? NULL : seq, follow_all_ssa_edges))
> + {
> + /* Early we want only to allow some generated tree codes. */
> + if (!early_p
> + || op.code.is_tree_code ()
> + || phiopt_early_allow ((tree_code)op.code))
> + {
> + result = maybe_push_res_to_seq (&op, seq);
> + if (result)
> + return result;
> + }
> + }
> +
> + return NULL;
> +}
> +
> /* The function match_simplify_replacement does the main work of doing the
> replacement using match and simplify. Return true if the replacement is done.
> Otherwise return false.
> @@ -828,10 +890,9 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> static bool
> match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> edge e0, edge e1, gphi *phi,
> - tree arg0, tree arg1)
> + tree arg0, tree arg1, bool early_p)
> {
> gimple *stmt;
> - tree cond;
> gimple_stmt_iterator gsi;
> edge true_edge, false_edge;
> gimple_seq seq = NULL;
> @@ -892,15 +953,6 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
>
> stmt = last_stmt (cond_bb);
>
> - /* To handle special cases like floating point comparison, it is easier and
> - less error-prone to build a tree and gimplify it on the fly though it is
> - less efficient.
> - Don't use fold_build2 here as that might create (bool)a instead of just
> - "a != 0". */
> - cond = build2_loc (gimple_location (stmt),
> - gimple_cond_code (stmt), boolean_type_node,
> - gimple_cond_lhs (stmt), gimple_cond_rhs (stmt));
> -
> /* We need to know which is the true edge and which is the false
> edge so that we know when to invert the condition below. */
> extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
> @@ -908,10 +960,9 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> std::swap (arg0, arg1);
>
> tree type = TREE_TYPE (gimple_phi_result (phi));
> - result = gimple_simplify (COND_EXPR, type,
> - cond,
> - arg0, arg1,
> - &seq, NULL);
> + result = gimple_simplify_phiopt (early_p, type, stmt,
> + arg0, arg1,
> + &seq);
> if (!result)
> return false;
>
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match
2021-07-04 18:37 ` [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match apinski
@ 2021-07-05 11:26 ` Richard Biener
0 siblings, 0 replies; 11+ messages in thread
From: Richard Biener @ 2021-07-05 11:26 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sun, Jul 4, 2021 at 8:42 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> To improve phiopt and be able to remove abs_replacement, this ports
> most of "A CMP 0 ? A : -A" from fold_cond_expr_with_comparison to
> match.pd. There is a few extra changes that are needed to remove
> the "A CMP 0 ? A : -A" part from fold_cond_expr_with_comparison:
> * Need to handle (A - B) case
> * Need to handle UN* comparisons.
>
> I will handle those in a different patch.
>
> Note phi-opt-15.c test needed to be updated as we get ABSU now
> instead of not getting ABS. When ABSU was added phiopt was not
> updated even to use ABSU instead of not creating ABS.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
OK
> gcc/ChangeLog:
>
> PR tree-optimization/101039
> * match.pd (A CMP 0 ? A : -A): New patterns.
> * tree-ssa-phiopt.c (abs_replacement): Delete function.
> (tree_ssa_phiopt_worker): Don't call abs_replacement.
> Update comment about abs_replacement.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/101039
> * gcc.dg/tree-ssa/phi-opt-15.c: Update test to expect
> ABSU and still not expect ABS_EXPR.
> * gcc.dg/tree-ssa/phi-opt-23.c: New test.
> * gcc.dg/tree-ssa/phi-opt-24.c: New test.
> ---
> gcc/match.pd | 60 +++++++++
> gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c | 4 +-
> gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c | 44 +++++++
> gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c | 44 +++++++
> gcc/tree-ssa-phiopt.c | 134 +--------------------
> 5 files changed, 152 insertions(+), 134 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 4e10d54383c..72860fbd448 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3976,6 +3976,66 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> (cnd (logical_inverted_value truth_valued_p@0) @1 @2)
> (cnd @0 @2 @1)))
>
> +/* abs/negative simplifications moved from fold_cond_expr_with_comparison,
> + Need to handle (A - B) case as fold_cond_expr_with_comparison does.
> + Need to handle UN* comparisons.
> +
> + None of these transformations work for modes with signed
> + zeros. If A is +/-0, the first two transformations will
> + change the sign of the result (from +0 to -0, or vice
> + versa). The last four will fix the sign of the result,
> + even though the original expressions could be positive or
> + negative, depending on the sign of A.
> +
> + Note that all these transformations are correct if A is
> + NaN, since the two alternatives (A and -A) are also NaNs. */
> +
> +(for cnd (cond vec_cond)
> + /* A == 0 ? A : -A same as -A */
> + (for cmp (eq uneq)
> + (simplify
> + (cnd (cmp @0 zerop) @0 (negate@1 @0))
> + (if (!HONOR_SIGNED_ZEROS (type))
> + @1))
> + (simplify
> + (cnd (cmp @0 zerop) integer_zerop (negate@1 @0))
> + (if (!HONOR_SIGNED_ZEROS (type))
> + @1))
> + )
> + /* A != 0 ? A : -A same as A */
> + (for cmp (ne ltgt)
> + (simplify
> + (cnd (cmp @0 zerop) @0 (negate @0))
> + (if (!HONOR_SIGNED_ZEROS (type))
> + @0))
> + (simplify
> + (cnd (cmp @0 zerop) @0 integer_zerop)
> + (if (!HONOR_SIGNED_ZEROS (type))
> + @0))
> + )
> + /* A >=/> 0 ? A : -A same as abs (A) */
> + (for cmp (ge gt)
> + (simplify
> + (cnd (cmp @0 zerop) @0 (negate @0))
> + (if (!HONOR_SIGNED_ZEROS (type)
> + && !TYPE_UNSIGNED (type))
> + (abs @0))))
> + /* A <=/< 0 ? A : -A same as -abs (A) */
> + (for cmp (le lt)
> + (simplify
> + (cnd (cmp @0 zerop) @0 (negate @0))
> + (if (!HONOR_SIGNED_ZEROS (type)
> + && !TYPE_UNSIGNED (type))
> + (if (ANY_INTEGRAL_TYPE_P (type)
> + && !TYPE_OVERFLOW_WRAPS (type))
> + (with {
> + tree utype = unsigned_type_for (type);
> + }
> + (convert (negate (absu:utype @0))))
> + (negate (abs @0)))))
> + )
> +)
> +
> /* -(type)!A -> (type)A - 1. */
> (simplify
> (negate (convert?:s (logical_inverted_value:s @0)))
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
> index ac3018ef533..6aec68961cf 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c
> @@ -9,4 +9,6 @@ foo (int i)
> return i;
> }
>
> -/* { dg-final { scan-tree-dump-not "ABS" "optimized" } } */
> +/* We should not have ABS_EXPR but ABSU_EXPR instead. */
> +/* { dg-final { scan-tree-dump-not "ABS_EXPR" "optimized" } } */
> +/* { dg-final { scan-tree-dump "ABSU" "optimized" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> new file mode 100644
> index 00000000000..ff658cd16a7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c
> @@ -0,0 +1,44 @@
> +/* { dg-options "-O2 -fdump-tree-phiopt" } */
> +
> +int f0(int A)
> +{
> +// A == 0? A : -A same as -A
> + if (A == 0) return A;
> + return -A;
> +}
> +
> +int f1(int A)
> +{
> +// A != 0? A : -A same as A
> + if (A != 0) return A;
> + return -A;
> +}
> +int f2(int A)
> +{
> +// A >= 0? A : -A same as abs (A)
> + if (A >= 0) return A;
> + return -A;
> +}
> +int f3(int A)
> +{
> +// A > 0? A : -A same as abs (A)
> + if (A > 0) return A;
> + return -A;
> +}
> +int f4(int A)
> +{
> +// A <= 0? A : -A same as -abs (A)
> + if (A <= 0) return A;
> + return -A;
> +}
> +int f5(int A)
> +{
> +// A < 0? A : -A same as -abs (A)
> + if (A < 0) return A;
> + return -A;
> +}
> +
> +/* These should be optimized in phiopt1 but is confused by predicts. */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
> +
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> new file mode 100644
> index 00000000000..eb89decb4bf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c
> @@ -0,0 +1,44 @@
> +/* { dg-options "-O2 -fno-signed-zeros -fdump-tree-phiopt" } */
> +
> +float f0(float A)
> +{
> +// A == 0? A : -A same as -A
> + if (A == 0) return A;
> + return -A;
> +}
> +
> +float f1(float A)
> +{
> +// A != 0? A : -A same as A
> + if (A != 0) return A;
> + return -A;
> +}
> +float f2(float A)
> +{
> +// A >= 0? A : -A same as abs (A)
> + if (A >= 0) return A;
> + return -A;
> +}
> +float f3(float A)
> +{
> +// A > 0? A : -A same as abs (A)
> + if (A > 0) return A;
> + return -A;
> +}
> +float f4(float A)
> +{
> +// A <= 0? A : -A same as -abs (A)
> + if (A <= 0) return A;
> + return -A;
> +}
> +float f5(float A)
> +{
> +// A < 0? A : -A same as -abs (A)
> + if (A < 0) return A;
> + return -A;
> +}
> +
> +/* These should be optimized in phiopt1 but is confused by predicts. */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */
> +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */
> +
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index fec8c02c062..a1664c1f914 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -64,8 +64,6 @@ static int value_replacement (basic_block, basic_block,
> edge, edge, gphi *, tree, tree);
> static bool minmax_replacement (basic_block, basic_block,
> edge, edge, gphi *, tree, tree);
> -static bool abs_replacement (basic_block, basic_block,
> - edge, edge, gphi *, tree, tree);
> static bool spaceship_replacement (basic_block, basic_block,
> edge, edge, gphi *, tree, tree);
> static bool cond_removal_in_popcount_clz_ctz_pattern (basic_block, basic_block,
> @@ -352,8 +350,6 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
> arg0, arg1,
> early_p))
> cfgchanged = true;
> - else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> - cfgchanged = true;
> else if (!early_p
> && cond_removal_in_popcount_clz_ctz_pattern (bb, bb1, e1,
> e2, phi, arg0,
> @@ -2614,134 +2610,6 @@ cond_removal_in_popcount_clz_ctz_pattern (basic_block cond_bb,
> return true;
> }
>
> -/* The function absolute_replacement does the main work of doing the absolute
> - replacement. Return true if the replacement is done. Otherwise return
> - false.
> - bb is the basic block where the replacement is going to be done on. arg0
> - is argument 0 from the phi. Likewise for arg1. */
> -
> -static bool
> -abs_replacement (basic_block cond_bb, basic_block middle_bb,
> - edge e0 ATTRIBUTE_UNUSED, edge e1,
> - gphi *phi, tree arg0, tree arg1)
> -{
> - tree result;
> - gassign *new_stmt;
> - gimple *cond;
> - gimple_stmt_iterator gsi;
> - edge true_edge, false_edge;
> - gimple *assign;
> - edge e;
> - tree rhs, lhs;
> - bool negate;
> - enum tree_code cond_code;
> -
> - /* If the type says honor signed zeros we cannot do this
> - optimization. */
> - if (HONOR_SIGNED_ZEROS (arg1))
> - return false;
> -
> - /* OTHER_BLOCK must have only one executable statement which must have the
> - form arg0 = -arg1 or arg1 = -arg0. */
> -
> - assign = last_and_only_stmt (middle_bb);
> - /* If we did not find the proper negation assignment, then we cannot
> - optimize. */
> - if (assign == NULL)
> - return false;
> -
> - /* If we got here, then we have found the only executable statement
> - in OTHER_BLOCK. If it is anything other than arg = -arg1 or
> - arg1 = -arg0, then we cannot optimize. */
> - if (gimple_code (assign) != GIMPLE_ASSIGN)
> - return false;
> -
> - lhs = gimple_assign_lhs (assign);
> -
> - if (gimple_assign_rhs_code (assign) != NEGATE_EXPR)
> - return false;
> -
> - rhs = gimple_assign_rhs1 (assign);
> -
> - /* The assignment has to be arg0 = -arg1 or arg1 = -arg0. */
> - if (!(lhs == arg0 && rhs == arg1)
> - && !(lhs == arg1 && rhs == arg0))
> - return false;
> -
> - cond = last_stmt (cond_bb);
> - result = PHI_RESULT (phi);
> -
> - /* Only relationals comparing arg[01] against zero are interesting. */
> - cond_code = gimple_cond_code (cond);
> - if (cond_code != GT_EXPR && cond_code != GE_EXPR
> - && cond_code != LT_EXPR && cond_code != LE_EXPR)
> - return false;
> -
> - /* Make sure the conditional is arg[01] OP y. */
> - if (gimple_cond_lhs (cond) != rhs)
> - return false;
> -
> - if (FLOAT_TYPE_P (TREE_TYPE (gimple_cond_rhs (cond)))
> - ? real_zerop (gimple_cond_rhs (cond))
> - : integer_zerop (gimple_cond_rhs (cond)))
> - ;
> - else
> - return false;
> -
> - /* We need to know which is the true edge and which is the false
> - edge so that we know if have abs or negative abs. */
> - extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
> -
> - /* For GT_EXPR/GE_EXPR, if the true edge goes to OTHER_BLOCK, then we
> - will need to negate the result. Similarly for LT_EXPR/LE_EXPR if
> - the false edge goes to OTHER_BLOCK. */
> - if (cond_code == GT_EXPR || cond_code == GE_EXPR)
> - e = true_edge;
> - else
> - e = false_edge;
> -
> - if (e->dest == middle_bb)
> - negate = true;
> - else
> - negate = false;
> -
> - /* If the code negates only iff positive then make sure to not
> - introduce undefined behavior when negating or computing the absolute.
> - ??? We could use range info if present to check for arg1 == INT_MIN. */
> - if (negate
> - && (ANY_INTEGRAL_TYPE_P (TREE_TYPE (arg1))
> - && ! TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg1))))
> - return false;
> -
> - result = duplicate_ssa_name (result, NULL);
> -
> - if (negate)
> - lhs = make_ssa_name (TREE_TYPE (result));
> - else
> - lhs = result;
> -
> - /* Build the modify expression with abs expression. */
> - new_stmt = gimple_build_assign (lhs, ABS_EXPR, rhs);
> -
> - gsi = gsi_last_bb (cond_bb);
> - gsi_insert_before (&gsi, new_stmt, GSI_NEW_STMT);
> -
> - if (negate)
> - {
> - /* Get the right GSI. We want to insert after the recently
> - added ABS_EXPR statement (which we know is the first statement
> - in the block. */
> - new_stmt = gimple_build_assign (result, NEGATE_EXPR, lhs);
> -
> - gsi_insert_after (&gsi, new_stmt, GSI_NEW_STMT);
> - }
> -
> - replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> -
> - /* Note that we optimized this PHI. */
> - return true;
> -}
> -
> /* Auxiliary functions to determine the set of memory accesses which
> can't trap because they are preceded by accesses to the same memory
> portion. We do that for MEM_REFs, so we only need to track
> @@ -3678,7 +3546,7 @@ gate_hoist_loads (void)
> ABS Replacement
> ---------------
>
> - This transformation, implemented in abs_replacement, replaces
> + This transformation, implemented in match_simplify_replacement, replaces
>
> bb0:
> if (a >= 0) goto bb2; else goto bb1;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT
2021-07-05 11:25 ` [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT Richard Biener
@ 2021-07-05 22:54 ` Andrew Pinski
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Pinski @ 2021-07-05 22:54 UTC (permalink / raw)
To: Richard Biener; +Cc: Andrew Pinski, GCC Patches
On Mon, Jul 5, 2021 at 4:26 AM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Sun, Jul 4, 2021 at 8:40 PM apinski--- via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > So the problem here is that replace_phi_edge_with_variable
> > will copy range information to a already (not newly) defined
> > ssa name. This causes wrong code later on.
>
> That's a bit too conservative I guess? Shouldn't it work for at least
> all defs defined in the same block as the original conditional (and
> thus then applying to the seq inserted there by the callers)?
Yes that even simplifies the change even further and still provide the
needed ranges
I should have a patch to submit later today.
Thanks,
Andrew Pinski
>
> I realize it's wrong for, say
>
> _1 = ..
> if (_1 != 0)
> {
> ...
> if (..)
> ;
> # _2 = PHI <_1, 1>
> ...
> }
>
> with _2 having range [1, +INF] but clearly not _1 at the point of its
> definition.
>
> Richard.
>
> > This patch fixes the problem by requiring there to be statements
> > that are to be placed before the conditional to be able to
> > copy the range info; this assumes the statements will define
> > the ssa name.
> >
> > gcc/ChangeLog:
> >
> > PR tree-optimization/101256
> > * dbgcnt.def (phiopt_edge_range): New counter.
> > * tree-ssa-phiopt.c (replace_phi_edge_with_variable):
> > Add optional sequence which will be added before the old
> > conditional. Check sequence for non-null if we want to
> > update the range.
> > (two_value_replacement): Instead of inserting the sequence,
> > update the call to replace_phi_edge_with_variable.
> > (match_simplify_replacement): Likewise.
> > (minmax_replacement): Likewise.
> > (value_replacement): Create a sequence of statements
> > which would have defined the ssa name. Update call
> > to replace_phi_edge_with_variable.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR tree-optimization/101256
> > * g++.dg/torture/pr101256.C: New test.
> > ---
> > gcc/dbgcnt.def | 1 +
> > gcc/testsuite/g++.dg/torture/pr101256.C | 28 +++++++++++++
> > gcc/tree-ssa-phiopt.c | 52 ++++++++++++++-----------
> > 3 files changed, 59 insertions(+), 22 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/torture/pr101256.C
> >
> > diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
> > index 93e7b4fd30e..2345899ba68 100644
> > --- a/gcc/dbgcnt.def
> > +++ b/gcc/dbgcnt.def
> > @@ -183,6 +183,7 @@ DEBUG_COUNTER (lim)
> > DEBUG_COUNTER (local_alloc_for_sched)
> > DEBUG_COUNTER (match)
> > DEBUG_COUNTER (merged_ipa_icf)
> > +DEBUG_COUNTER (phiopt_edge_range)
> > DEBUG_COUNTER (postreload_cse)
> > DEBUG_COUNTER (pre)
> > DEBUG_COUNTER (pre_insn)
> > diff --git a/gcc/testsuite/g++.dg/torture/pr101256.C b/gcc/testsuite/g++.dg/torture/pr101256.C
> > new file mode 100644
> > index 00000000000..973a8b4caf3
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/torture/pr101256.C
> > @@ -0,0 +1,28 @@
> > +// { dg-do run }
> > +
> > +template<class T>
> > +const T& max(const T& a, const T& b)
> > +{
> > + return (a < b) ? b : a;
> > +}
> > +
> > +signed char var_5 = -128;
> > +unsigned int var_11 = 2144479212U;
> > +unsigned long long int arr [22];
> > +
> > +void
> > +__attribute__((noipa))
> > +test(signed char var_5, unsigned var_11) {
> > + for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
> > + arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
> > +}
> > +
> > +int main() {
> > + for (int i_0 = 0; i_0 < 22; ++i_0)
> > + arr [i_0] = 11834725929543695741ULL;
> > +
> > + test(var_5, var_11);
> > + if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
> > + __builtin_abort ();
> > + return 0;
> > +}
> > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> > index ab12e85569d..71f0019d877 100644
> > --- a/gcc/tree-ssa-phiopt.c
> > +++ b/gcc/tree-ssa-phiopt.c
> > @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3. If not see
> > #include "gimple-fold.h"
> > #include "internal-fn.h"
> > #include "gimple-range.h"
> > +#include "dbgcnt.h"
> >
> > static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
> > static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
> > @@ -73,7 +74,8 @@ static bool cond_store_replacement (basic_block, basic_block, edge, edge,
> > hash_set<tree> *);
> > static bool cond_if_else_store_replacement (basic_block, basic_block, basic_block);
> > static hash_set<tree> * get_non_trapping ();
> > -static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree);
> > +static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree,
> > + gimple_seq = NULL);
> > static void hoist_adjacent_loads (basic_block, basic_block,
> > basic_block, basic_block);
> > static bool gate_hoist_loads (void);
> > @@ -382,18 +384,20 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
> >
> > /* Replace PHI node element whose edge is E in block BB with variable NEW.
> > Remove the edge from COND_BLOCK which does not lead to BB (COND_BLOCK
> > - is known to have two edges, one of which must reach BB). */
> > + is known to have two edges, one of which must reach BB).
> > + Optionally insert stmts before the old condition. */
> >
> > static void
> > replace_phi_edge_with_variable (basic_block cond_block,
> > - edge e, gphi *phi, tree new_tree)
> > + edge e, gphi *phi, tree new_tree,
> > + gimple_seq stmts)
> > {
> > basic_block bb = gimple_bb (phi);
> > basic_block block_to_remove;
> > gimple_stmt_iterator gsi;
> > tree phi_result = PHI_RESULT (phi);
> >
> > - /* Duplicate range info if we're the only things setting the target PHI.
> > + /* Duplicate range info if they are the only things setting the target PHI.
> > This is needed as later on, the new_tree will be replacing
> > The assignement of the PHI.
> > For an example:
> > @@ -401,19 +405,23 @@ replace_phi_edge_with_variable (basic_block cond_block,
> > _4 = min<a_1, 255>
> > goto bb2
> >
> > - range<-INF,255>
> > + # RANGE [-INF, 255]
> > a_3 = PHI<_4(1)>
> > bb3:
> >
> > use(a_3)
> > - And _4 gets prograted into the use of a_3 and losing the range info.
> > - This can't be done for more than 2 incoming edges as the progration
> > - won't happen. */
> > + And _4 gets propagated into the use of a_3 and losing the range info.
> > + This can't be done for more than 2 incoming edges as the propagation
> > + won't happen.
> > + This also cannot be done if the name is not defined by statements to
> > + be placed before the conditional. */
> > if (TREE_CODE (new_tree) == SSA_NAME
> > && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> > && INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
> > && !SSA_NAME_RANGE_INFO (new_tree)
> > - && SSA_NAME_RANGE_INFO (phi_result))
> > + && SSA_NAME_RANGE_INFO (phi_result)
> > + && stmts != NULL
> > + && dbg_cnt (phiopt_edge_range))
> > duplicate_ssa_name_range_info (new_tree,
> > SSA_NAME_RANGE_TYPE (phi_result),
> > SSA_NAME_RANGE_INFO (phi_result));
> > @@ -443,6 +451,8 @@ replace_phi_edge_with_variable (basic_block cond_block,
> >
> > /* Eliminate the COND_EXPR at the end of COND_BLOCK. */
> > gsi = gsi_last_bb (cond_block);
> > + if (stmts)
> > + gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
> > gsi_remove (&gsi, true);
> >
> > statistics_counter_event (cfun, "Replace PHI with variable", 1);
> > @@ -802,10 +812,8 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> > else
> > new_rhs = gimple_build (&stmts, MINUS_EXPR, type, arg, lhs);
> > new_rhs = gimple_convert (&stmts, TREE_TYPE (arg0), new_rhs);
> > - gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> > - gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
> >
> > - replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs);
> > + replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs, stmts);
> >
> > /* Note that we optimized this PHI. */
> > return true;
> > @@ -920,10 +928,8 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> > gsi_move_before (&gsi1, &gsi);
> > reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
> > }
> > - if (seq)
> > - gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
> >
> > - replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> > + replace_phi_edge_with_variable (cond_bb, e1, phi, result, seq);
> >
> > /* Add Statistic here even though replace_phi_edge_with_variable already
> > does it as we want to be able to count when match-simplify happens vs
> > @@ -1396,6 +1402,7 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> > && absorbing_element_p (code_def,
> > cond_rhs, false, rhs2))))))
> > {
> > + gimple_seq stmts = NULL;
> > gsi = gsi_for_stmt (cond);
> > /* Moving ASSIGN might change VR of lhs, e.g. when moving u_6
> > def-stmt in:
> > @@ -1417,11 +1424,15 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> > tree plhs = gimple_assign_lhs (prep_stmt[i]);
> > reset_flow_sensitive_info (plhs);
> > gsi_from = gsi_for_stmt (prep_stmt[i]);
> > - gsi_move_before (&gsi_from, &gsi);
> > + gsi_remove (&gsi_from, false);
> > +
> > + gimple_seq_add_stmt (&stmts, prep_stmt[i]);
> > }
> > gsi_from = gsi_for_stmt (assign);
> > - gsi_move_before (&gsi_from, &gsi);
> > - replace_phi_edge_with_variable (cond_bb, e1, phi, lhs);
> > + gsi_remove (&gsi_from, false);
> > + gimple_seq_add_stmt (&stmts, assign);
> > +
> > + replace_phi_edge_with_variable (cond_bb, e1, phi, lhs, stmts);
> > return 2;
> > }
> >
> > @@ -1811,10 +1822,7 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
> > tree phi_result = PHI_RESULT (phi);
> > result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
> >
> > - gsi = gsi_last_bb (cond_bb);
> > - gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
> > -
> > - replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> > + replace_phi_edge_with_variable (cond_bb, e1, phi, result, stmts);
> >
> > return true;
> > }
> > --
> > 2.27.0
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-05 22:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-04 18:37 [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT apinski
2021-07-04 18:37 ` [PATCH 2/5] Fix PR 101237: Remove element_type call when used with the functions from real apinski
2021-07-05 11:18 ` Richard Biener
2021-07-04 18:37 ` [PATCH 3/5] Allow match-and-simplified phiopt to run in early phiopt apinski
2021-07-05 11:26 ` Richard Biener
2021-07-04 18:37 ` [PATCH 4/5] Try inverted comparison for match_simplify in phiopt apinski
2021-07-05 11:18 ` Richard Biener
2021-07-04 18:37 ` [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match apinski
2021-07-05 11:26 ` Richard Biener
2021-07-05 11:25 ` [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT Richard Biener
2021-07-05 22:54 ` Andrew Pinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).