On 4/13/23 09:56, Richard Biener wrote:
On Wed, Apr 12, 2023 at 10:55 PM Andrew MacLeod <amacl...@redhat.com> wrote:
On 4/12/23 07:01, Richard Biener wrote:
On Wed, Apr 12, 2023 at 12:59 PM Jakub Jelinek <ja...@redhat.com> wrote:
Would be nice.
Though, I'm afraid it still wouldn't fix the PR101912 testcase, because
it has exactly what happens in this PR, undefined phi arg from the
pre-header and uses of the previous iteration's value (i.e. across
backedge).
Well yes, that's what's not allowed. So when the PHI dominates the
to-be-equivalenced argument edge src then the equivalence isn't
valid because there's a place (that very source block for example) a use of the
PHI lhs could appear and where we'd then mixup iterations.
If we want to implement this cleaner, then as you say, we don't create
the equivalence if the PHI node dominates the argument edge. The
attached patch does just that, removing the both the "fix" for 108139
and the just committed one for 109462, replacing them with catching this
at the time of equivalence registering.
It bootstraps and passes all regressions tests.
Do you want me to check this into trunk?
Uh, it looks a bit convoluted. Wouldn't the following be enough? OK
if that works
(or fixed if I messed up trivially)
diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index e81f6b3699e..9c29012e160 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -776,7 +776,11 @@ fold_using_range::range_of_phi (vrange &r, gphi
*phi, fur_source &src)
if (!seen_arg)
{
seen_arg = true;
- single_arg = arg;
+ // Avoid registering an equivalence if the PHI dominates the
+ // argument edge. See PR 108139/109462.
+ if (dom_info_available_p (CDI_DOMINATORS)
+ && !dominated_by_p (CDI_DOMINATORS, e->src, gimple_bb (phi)))
+ single_arg = arg;
}
else if (single_arg != arg)
single_arg = NULL_TREE;
It would exposes a slight hole.. in cases where there is more than one
copy of the name, ie:
for a_2 = PHI <c_3, c_3> we currently will create an equivalence
between a_2 and c_3 because its considered a single argument. Not a
big deal for this case since all arguments are c_3, but the hole would
be when we have something like:
a_2 = PHI<c_3, c_3, d_4(D)> if d_4 is undefined, then with the above
patch we would only check the dominance of the first edge with c_3. we'd
need to check all of them.
The patch is slightly convoluted because we always defer checking the
edge/processing single arguments until we think there is a reason to
(for performance). My patch simple does the deferred check on the
previous edge and sets the new one so that we would check both edges are
valid before setting the equivalence. Even as it is with this deferred
check we're about 0.4% slower in VRP. IF we didnt do this deferring,
then every PHI is going to have a check.
And along the way, remove the boolean seen_arg because having
single_arg_edge set produces the same information.
Perhaps it would be cleaner to simply defer the entire thing to the end,
like so.
Performance is pretty much identical in the end.
Bootstraps on x86_64-pc-linux-gnu, regressions are running. Assuming no
regressions pop up, OK for trunk?
Andrew
commit 9e16ef8e4de26bdc6e570bd327bbe15845491169
Author: Andrew MacLeod <amacl...@redhat.com>
Date: Wed Apr 12 13:10:55 2023 -0400
Ensure PHI equivalencies do not dominate the argument edge.
When we create an equivalency between a PHI definition and an argument,
ensure the definition does not dominate the incoming argument edge.
PR tree-optimziation/108139
PR tree-optimziation/109462
* gimple-range-cache.cc (ranger_cache::fill_block_cache): Remove
equivalency check for PHI nodes.
* gimple-range-fold.cc (fold_using_range::range_of_phi): Ensure def
does not dominate single-arg equivalency edges.
diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
index 3b52f1e734c..2314478d558 100644
--- a/gcc/gimple-range-cache.cc
+++ b/gcc/gimple-range-cache.cc
@@ -1220,7 +1220,7 @@ ranger_cache::fill_block_cache (tree name, basic_block bb, basic_block def_bb)
// See if any equivalences can refine it.
// PR 109462, like 108139 below, a one way equivalence introduced
// by a PHI node can also be through the definition side. Disallow it.
- if (m_oracle && !is_a<gphi *> (SSA_NAME_DEF_STMT (name)))
+ if (m_oracle)
{
tree equiv_name;
relation_kind rel;
@@ -1237,13 +1237,6 @@ ranger_cache::fill_block_cache (tree name, basic_block bb, basic_block def_bb)
if (!m_gori.has_edge_range_p (equiv_name))
continue;
- // PR 108139. It is hazardous to assume an equivalence with
- // a PHI is the same value. The PHI may be an equivalence
- // via UNDEFINED arguments which is really a one way equivalence.
- // PHIDEF == name, but name may not be == PHIDEF.
- if (is_a<gphi *> (SSA_NAME_DEF_STMT (equiv_name)))
- continue;
-
// Check if the equiv definition dominates this block
if (equiv_bb == bb ||
(equiv_bb && !dominated_by_p (CDI_DOMINATORS, bb, equiv_bb)))
diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index e81f6b3699e..429734f954a 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -795,9 +795,28 @@ fold_using_range::range_of_phi (vrange &r, gphi *phi, fur_source &src)
// If the PHI boils down to a single effective argument, look at it.
if (single_arg)
{
- // Symbolic arguments are equivalences.
+ // Symbolic arguments can be equivalences.
if (gimple_range_ssa_p (single_arg))
- src.register_relation (phi, VREL_EQ, phi_def, single_arg);
+ {
+ // Only allow the equivalence if the PHI definition does not
+ // dominate any incoming edge for SINGLE_ARG.
+ // See PR 108139 and 109462.
+ basic_block bb = gimple_bb (phi);
+ if (!dom_info_available_p (CDI_DOMINATORS))
+ single_arg = NULL;
+ else
+ for (x = 0; x < gimple_phi_num_args (phi); x++)
+ if (gimple_phi_arg_def (phi, x) == single_arg
+ && dominated_by_p (CDI_DOMINATORS,
+ gimple_phi_arg_edge (phi, x)->src,
+ bb))
+ {
+ single_arg = NULL;
+ break;
+ }
+ if (single_arg)
+ src.register_relation (phi, VREL_EQ, phi_def, single_arg);
+ }
else if (src.get_operand (arg_range, single_arg)
&& arg_range.singleton_p ())
{
