| Message ID | 0cee3370-f035-7e8c-6c6f-f77153a40c0e@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix regression introduced by 88069 | expand |
On Wed, Nov 21, 2018 at 3:28 AM Jeff Law <law@redhat.com> wrote: > > Richi's recent change to fix 88069 is causing various targets to fail > tree-ssa/20030711-2.c. That test is verifying a variety of > optimizations occur during the first DOM pass. > > Prior to Richi's change FRE1 would do some significant cleanups of the > IL and as a result DOM was fully able to optimize the resultant code. Hum... I obviously missed the FAIL during testing somehow. FRE1 behavior shouldn't change so I'll fixup. Richard. > After Richi's change we've got a redundant load in the IL. After > analyzing the CFG and IL it was clear that DOM *should* be able to > remove the redundant load, but simply wasn't. > > DOM would discover that it could statically determine the result of a > branch condition. This resulted in one arm of the branch becoming > unreachable. That in turn caused some PHI nodes to become degenerates. > > Normally when a PHI node becomes a degenerate we record it as a copy in > the const_and_copies table and *most* of the time we'll propagate the > src value into uses of the dest. But propagation is not guaranteed > (there's a BZ around that issue you can find if you dig into the history > of some of this code). > > Anyway, exposing the degenerate PHI *should* have exposed the redundant > load, but we didn't record anything into the const/copies table for the > virtual phi. That's a conscious decision to avoid issues with > overlapping lifetimes of virtual SSA_NAMEs. > > While investigating the history here I noticed Richi's little trick > which allows propagation of virtuals if we propagate to all the uses. > Twiddling DOM to use that same trick results in the virtual operand > propagating. That in turn allows DOM to see and remove the redundant load. > > Bootstrapped and regression tested on x86_64 where is fixes > 20030711-2.c. Also verified that it fixed various other targets where > that test had started failing. > > Installing on the trunk. > > jeff
On 11/21/18 7:21 AM, Richard Biener wrote: > On Wed, Nov 21, 2018 at 3:28 AM Jeff Law <law@redhat.com> wrote: >> >> Richi's recent change to fix 88069 is causing various targets to fail >> tree-ssa/20030711-2.c. That test is verifying a variety of >> optimizations occur during the first DOM pass. >> >> Prior to Richi's change FRE1 would do some significant cleanups of the >> IL and as a result DOM was fully able to optimize the resultant code. > > Hum... I obviously missed the FAIL during testing somehow. FRE1 > behavior shouldn't change so I'll fixup. NP. The change to DOM is probably a good thing independently. I may go back and factor that little hunk into a reusable function as we've got a nearly identical copy elsewhere. jeff
On Wed, Nov 21, 2018 at 3:23 PM Jeff Law <law@redhat.com> wrote: > > On 11/21/18 7:21 AM, Richard Biener wrote: > > On Wed, Nov 21, 2018 at 3:28 AM Jeff Law <law@redhat.com> wrote: > >> > >> Richi's recent change to fix 88069 is causing various targets to fail > >> tree-ssa/20030711-2.c. That test is verifying a variety of > >> optimizations occur during the first DOM pass. > >> > >> Prior to Richi's change FRE1 would do some significant cleanups of the > >> IL and as a result DOM was fully able to optimize the resultant code. > > > > Hum... I obviously missed the FAIL during testing somehow. FRE1 > > behavior shouldn't change so I'll fixup. > NP. The change to DOM is probably a good thing independently. I may go > back and factor that little hunk into a reusable function as we've got a > nearly identical copy elsewhere. I am testing the following. Richard. 2018-11-21 Richard Biener <rguenther@suse.de> PR tree-optimization/88069 * tree-ssa-sccvn.c (visit_phi): Tweak previous fix to not apply to default defs. Index: gcc/tree-ssa-sccvn.c =================================================================== --- gcc/tree-ssa-sccvn.c (revision 266345) +++ gcc/tree-ssa-sccvn.c (working copy) @@ -4205,6 +4205,7 @@ visit_phi (gimple *phi, bool *inserted, given that allows us to escape a region in alias walking. */ || (sameval && TREE_CODE (sameval) == SSA_NAME + && !SSA_NAME_IS_DEFAULT_DEF (sameval) && SSA_NAME_IS_VIRTUAL_OPERAND (sameval) && (SSA_VAL (sameval, &visited_p), !visited_p))) /* Note this just drops to VARYING without inserting the PHI into > jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index aece55980f0..5c7a9509b35 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2018-11-20 Jeff Law <law@redhat.com> + + PR tree-optimization/88069 + * tree-ssa-dom.c (record_equivalences_from_phis): Propagate away + degenerate virtual PHIs. + 2018-11-20 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/87895 diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c index 7787da8b237..ce840488403 100644 --- a/gcc/tree-ssa-dom.c +++ b/gcc/tree-ssa-dom.c @@ -1106,10 +1106,13 @@ record_equivalences_from_phis (basic_block bb) { gphi_iterator gsi; - for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); ) { gphi *phi = gsi.phi (); + /* We might eliminate the PHI, so advance GSI now. */ + gsi_next (&gsi); + tree lhs = gimple_phi_result (phi); tree rhs = NULL; size_t i; @@ -1159,9 +1162,26 @@ record_equivalences_from_phis (basic_block bb) this, since this is a true assignment and not an equivalence inferred from a comparison. All uses of this ssa name are dominated by this assignment, so unwinding just costs time and space. */ - if (i == gimple_phi_num_args (phi) - && may_propagate_copy (lhs, rhs)) - set_ssa_name_value (lhs, rhs); + if (i == gimple_phi_num_args (phi)) + { + if (may_propagate_copy (lhs, rhs)) + set_ssa_name_value (lhs, rhs); + else if (virtual_operand_p (lhs)) + { + gimple *use_stmt; + imm_use_iterator iter; + use_operand_p use_p; + /* For virtual operands we have to propagate into all uses as + otherwise we will create overlapping life-ranges. */ + FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs) + FOR_EACH_IMM_USE_ON_STMT (use_p, iter) + SET_USE (use_p, rhs); + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs)) + SSA_NAME_OCCURS_IN_ABNORMAL_PHI (rhs) = 1; + gimple_stmt_iterator tmp_gsi = gsi_for_stmt (phi); + remove_phi_node (&tmp_gsi, true); + } + } } }
Richi's recent change to fix 88069 is causing various targets to fail tree-ssa/20030711-2.c. That test is verifying a variety of optimizations occur during the first DOM pass. Prior to Richi's change FRE1 would do some significant cleanups of the IL and as a result DOM was fully able to optimize the resultant code. After Richi's change we've got a redundant load in the IL. After analyzing the CFG and IL it was clear that DOM *should* be able to remove the redundant load, but simply wasn't. DOM would discover that it could statically determine the result of a branch condition. This resulted in one arm of the branch becoming unreachable. That in turn caused some PHI nodes to become degenerates. Normally when a PHI node becomes a degenerate we record it as a copy in the const_and_copies table and *most* of the time we'll propagate the src value into uses of the dest. But propagation is not guaranteed (there's a BZ around that issue you can find if you dig into the history of some of this code). Anyway, exposing the degenerate PHI *should* have exposed the redundant load, but we didn't record anything into the const/copies table for the virtual phi. That's a conscious decision to avoid issues with overlapping lifetimes of virtual SSA_NAMEs. While investigating the history here I noticed Richi's little trick which allows propagation of virtuals if we propagate to all the uses. Twiddling DOM to use that same trick results in the virtual operand propagating. That in turn allows DOM to see and remove the redundant load. Bootstrapped and regression tested on x86_64 where is fixes 20030711-2.c. Also verified that it fixed various other targets where that test had started failing. Installing on the trunk. jeff commit 1438e5e18df9f927990074dd32572abf924eba86 Author: Jeff Law <law@redhat.com> Date: Tue Nov 20 18:10:28 2018 -0700 2018-11-20 Jeff Law <law@redhat.com> PR tree-optimization/88069 * tree-ssa-dom.c (record_equivalences_from_phis): Propagate away degenerate virtual PHIs.