The PR shows that when using std::clamp from the C++ standard library
and there is surrounding code using exceptions then phiprop can fail
to simplify the code so phiopt can turn the clamping into efficient
min/max operations.
The validation code is needlessly complicated, steming from the
time we had memory-SSA with multiple virtual operands. The following
simplifies this, thereby fixing this issue.
Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
PR tree-optimization/117965
* tree-ssa-phiprop.cc (phivn_valid_p): Remove.
(propagate_with_phi): Pass in virtual PHI node from BB,
rewrite load motion validity check to require the same
virtual use along all paths.
* g++.dg/tree-ssa/pr117965-1.C: New testcase.
* g++.dg/tree-ssa/pr117965-2.C: Likewise.
---
gcc/testsuite/g++.dg/tree-ssa/pr117965-1.C | 28 +++++++
gcc/testsuite/g++.dg/tree-ssa/pr117965-2.C | 19 +++++
gcc/tree-ssa-phiprop.cc | 96 +++++++++++-----------
3 files changed, 97 insertions(+), 46 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr117965-1.C
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr117965-2.C
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr117965-1.C
b/gcc/testsuite/g++.dg/tree-ssa/pr117965-1.C
new file mode 100644
index 00000000000..84f0f2b75df
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/pr117965-1.C
@@ -0,0 +1,28 @@
+// { dg-do compile }
+// { dg-options "-O2 -fdump-tree-phiopt1" }
+
+void f();
+void f(int&);
+
+static inline const int &
+clamp(const int &v, const int &min, const int &max)
+{
+ const int &t = (v > min) ? v : min;
+ return t > max ? max : t;
+}
+
+void clamp2(int num1) {
+ try {
+ int low = -12, high = 12;
+ f();
+ num1 = clamp(num1, low, high);
+ f(num1);
+ } catch(...)
+ {
+ __builtin_printf("caught.\n");
+ return;
+ }
+}
+
+// { dg-final { scan-tree-dump-times "MAX" 1 "phiopt1" } } */
+// { dg-final { scan-tree-dump-times "MIN" 1 "phiopt1" } } */
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr117965-2.C
b/gcc/testsuite/g++.dg/tree-ssa/pr117965-2.C
new file mode 100644
index 00000000000..3e94fb3c05c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/pr117965-2.C
@@ -0,0 +1,19 @@
+// { dg-do compile { target c++17 } }
+// { dg-options "-O2 -fdump-tree-phiopt1" }
+
+#include <iostream>
+#include <algorithm>
+
+void clamp2 ()
+{
+ float low = -1.0f, high = 1.0f;
+ float num1, num2, num3;
+ std::cin >> num1;
+ num1 = std::clamp(num1, low, high);
+ std::cout << num1;
+}
+
+// { dg-final { scan-tree-dump-times " < -1.0" 1 "phiopt1" } }
+// { dg-final { scan-tree-dump-times " \\\? -1.0e\\\+0 : " 1 "phiopt1" } }
+// { dg-final { scan-tree-dump-times " > 1.0" 1 "phiopt1" } }
+// { dg-final { scan-tree-dump-times " \\\? 1.0e\\\+0 : " 1 "phiopt1" } }
diff --git a/gcc/tree-ssa-phiprop.cc b/gcc/tree-ssa-phiprop.cc
index a2e1fb16a30..897bd583ea7 100644
--- a/gcc/tree-ssa-phiprop.cc
+++ b/gcc/tree-ssa-phiprop.cc
@@ -99,35 +99,6 @@ struct phiprop_d
tree vuse;
};
-/* Verify if the value recorded for NAME in PHIVN is still valid at
- the start of basic block BB. */
-
-static bool
-phivn_valid_p (struct phiprop_d *phivn, tree name, basic_block bb)
-{
- tree vuse = phivn[SSA_NAME_VERSION (name)].vuse;
- gimple *use_stmt;
- imm_use_iterator ui2;
- bool ok = true;
-
- /* The def stmts of the virtual uses need to be dominated by bb. */
- gcc_assert (vuse != NULL_TREE);
-
- FOR_EACH_IMM_USE_STMT (use_stmt, ui2, vuse)
- {
- /* If BB does not dominate a VDEF, the value is invalid. */
- if ((gimple_vdef (use_stmt) != NULL_TREE
- || gimple_code (use_stmt) == GIMPLE_PHI)
- && !dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), bb))
- {
- ok = false;
- break;
- }
- }
-
- return ok;
-}
-
/* Insert a new phi node for the dereference of PHI at basic_block
BB with the virtual operands from USE_STMT. */
@@ -275,12 +246,13 @@ chk_uses (tree, tree *idx, void *data)
<Lx>:;
Returns true if a transformation was done and edge insertions
need to be committed. Global data PHIVN and N is used to track
- past transformation results. We need to be especially careful here
+ past transformation results. VPHI is the virtual PHI node in BB
+ if there is one. We need to be especially careful here
with aliasing issues as we are moving memory reads. */
static bool
-propagate_with_phi (basic_block bb, gphi *phi, struct phiprop_d *phivn,
- size_t n, bitmap dce_ssa_names)
+propagate_with_phi (basic_block bb, gphi *vphi, gphi *phi,
+ struct phiprop_d *phivn, size_t n, bitmap dce_ssa_names)
{
tree ptr = PHI_RESULT (phi);
gimple *use_stmt;
@@ -298,6 +270,7 @@ propagate_with_phi (basic_block bb, gphi *phi, struct
phiprop_d *phivn,
&& TYPE_MODE (TREE_TYPE (TREE_TYPE (ptr))) == BLKmode))
return false;
+ tree up_vuse = NULL_TREE;
/* Check if we can "cheaply" dereference all phi arguments. */
FOR_EACH_PHI_ARG (arg_p, phi, i, SSA_OP_USE)
{
@@ -315,14 +288,28 @@ propagate_with_phi (basic_block bb, gphi *phi, struct
phiprop_d *phivn,
return false;
arg = gimple_assign_rhs1 (def_stmt);
}
- if (TREE_CODE (arg) != ADDR_EXPR
- && !(TREE_CODE (arg) == SSA_NAME
+ if (TREE_CODE (arg) == ADDR_EXPR)
+ ;
+ /* When we have an SSA name see if we previously encountered a
+ dereference of it. */
+ else if (TREE_CODE (arg) == SSA_NAME
&& SSA_NAME_VERSION (arg) < n
&& phivn[SSA_NAME_VERSION (arg)].value != NULL_TREE
&& (!type
|| types_compatible_p
- (type, TREE_TYPE (phivn[SSA_NAME_VERSION (arg)].value)))
- && phivn_valid_p (phivn, arg, bb)))
+ (type, TREE_TYPE (phivn[SSA_NAME_VERSION (arg)].value))))
+ {
+ /* The dereference should be under the VUSE that's active in BB.
+ If the BB has no virtual PHI then record the common "incoming"
+ vuse. */
+ if (vphi)
+ up_vuse = gimple_phi_arg_def (vphi, phi_arg_index_from_use (arg_p));
+ if (!up_vuse)
+ up_vuse = phivn[SSA_NAME_VERSION (arg)].vuse;
+ else if (up_vuse != phivn[SSA_NAME_VERSION (arg)].vuse)
+ return false;
+ }
+ else
return false;
if (!type
&& TREE_CODE (arg) == SSA_NAME)
@@ -372,17 +359,32 @@ propagate_with_phi (basic_block bb, gphi *phi, struct
phiprop_d *phivn,
&& !gimple_has_volatile_ops (use_stmt)))
continue;
- /* Check if we can move the loads. The def stmt of the virtual use
- needs to be in a different basic block dominating bb. When the
+ /* Check if we can move the loads. This is when the virtual use
+ is the same as the one active at the start of BB which we know
+ either from its virtual PHI def or from the common incoming
+ VUSE. If neither is present make sure the def stmt of the virtual
+ use is in a different basic block dominating BB. When the
def is an edge-inserted one we know it dominates us. */
vuse = gimple_vuse (use_stmt);
- def_stmt = SSA_NAME_DEF_STMT (vuse);
- if (!SSA_NAME_IS_DEFAULT_DEF (vuse)
- && (gimple_bb (def_stmt) == bb
- || (gimple_bb (def_stmt)
- && !dominated_by_p (CDI_DOMINATORS,
- bb, gimple_bb (def_stmt)))))
- goto next;
+ if (vphi)
+ {
+ if (vuse != gimple_phi_result (vphi))
+ goto next;
+ }
+ else if (up_vuse)
+ {
+ if (vuse != up_vuse)
+ goto next;
+ }
+ else
+ {
+ def_stmt = SSA_NAME_DEF_STMT (vuse);
+ if (!SSA_NAME_IS_DEFAULT_DEF (vuse)
+ && (gimple_bb (def_stmt) == bb
+ || !dominated_by_p (CDI_DOMINATORS,
+ bb, gimple_bb (def_stmt))))
+ goto next;
+ }
/* Found a proper dereference with an aggregate copy. Just
insert aggregate copies on the edges instead. */
@@ -535,8 +537,10 @@ pass_phiprop::execute (function *fun)
edges avoid blocks with abnormal predecessors. */
if (bb_has_abnormal_pred (bb))
continue;
+ gphi *vphi = get_virtual_phi (bb);
for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
- did_something |= propagate_with_phi (bb, gsi.phi (), phivn, n,
dce_ssa_names);
+ did_something |= propagate_with_phi (bb, vphi, gsi.phi (),
+ phivn, n, dce_ssa_names);
}
if (did_something)
--
2.43.0
