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?
Andrew
PS Of course, we still fail 101912. The only way I see us being
able to do anything with that is to effectively peel the first iteration
off, either physically, or logically with the path ranger to determine
if a given use is actually reachable by the undefined value.
<bb2>
<bb 9> :
# prevcorr_7 = PHI <prevcorr_19(D)(2), corr_22(8)>
# leapcnt_8 = PHI <0(2), leapcnt_26(8)>
if (leapcnt_8 < n_16) // 0 < n_16
goto <bb 3>; [INV]
<bb 3> :
corr_22 = getint ();
if (corr_22 <= 0)
goto <bb 7>; [INV]
else
goto <bb 4>; [INV]
<bb 4> :
_1 = corr_22 == 1;
_2 = leapcnt_8 != 0; // [0, 0] = 0 != 0
_3 = _1 & _2; // [0, 0] = 0 & _2
if (_3 != 0) // 4->5 is not taken on the path starting
2->9
goto <bb 5>; [INV]
else
goto <bb 8>; [INV]
<bb 5> : // We know this path is not taken when
prevcorr_7 == prevcorr_19(D)(2)
if (prevcorr_7 != 1)
goto <bb 6>; [INV]
else
goto <bb 8>; [INV]
<bb 6> :
_5 = prevcorr_7 + -1;
if (prevcorr_7 != 2)
goto <bb 7>; [INV]
else
goto <bb 8>; [INV]
Using the path ranger (Would it even need tweaks aldy?) , before issuing
the warning the uninit code could easily start at each use, construct
the path(s) to that use from the unitialized value, and determine when
prevcorr is uninitialized, 2->9->3->4->5 will not be executed and of
course,neither will 2->9->3->4->5->6
I think threading already does something similar?
commit 79b13320cf739c965bc8c7ceb8b27903271a3f6e
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 defintion 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..8860152d3a0 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -742,7 +742,8 @@ fold_using_range::range_of_phi (vrange &r, gphi *phi, fur_source &src)
// Track if all executable arguments are the same.
tree single_arg = NULL_TREE;
- bool seen_arg = false;
+ edge single_arg_edge = NULL;
+ basic_block bb = gimple_bb (phi);
// Start with an empty range, unioning in each argument's range.
r.set_undefined ();
@@ -773,13 +774,23 @@ fold_using_range::range_of_phi (vrange &r, gphi *phi, fur_source &src)
src.gori ()->register_dependency (phi_def, arg);
// Track if all arguments are the same.
- if (!seen_arg)
+ if (!single_arg_edge)
{
- seen_arg = true;
single_arg = arg;
+ single_arg_edge = gimple_phi_arg_edge (phi, x);
}
else if (single_arg != arg)
single_arg = NULL_TREE;
+ else
+ {
+ // Check the current single edge for dominance, and if its ok,
+ // set it to the new edge. Otherwise not a single_arg case.
+ if (dom_info_available_p (CDI_DOMINATORS)
+ && !dominated_by_p (CDI_DOMINATORS, single_arg_edge->src, bb))
+ single_arg_edge = gimple_phi_arg_edge (phi, x);
+ else
+ single_arg = NULL_TREE;
+ }
}
// Once the value reaches varying, stop looking.
@@ -795,9 +806,14 @@ 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.
if (gimple_range_ssa_p (single_arg))
- src.register_relation (phi, VREL_EQ, phi_def, single_arg);
+ {
+ // Register an equivalence only if the PHI does not dominate
+ // the argmuent edge. See PR 108139/109462.
+ if (dom_info_available_p (CDI_DOMINATORS)
+ && !dominated_by_p (CDI_DOMINATORS, single_arg_edge->src, bb))
+ src.register_relation (phi, VREL_EQ, phi_def, single_arg);
+ }
else if (src.get_operand (arg_range, single_arg)
&& arg_range.singleton_p ())
{
