On Wed, 2 Oct 2019 at 12:28, Richard Biener <rguent...@suse.de> wrote:
>
> On Wed, 2 Oct 2019, Prathamesh Kulkarni wrote:
>
> > On Wed, 2 Oct 2019 at 01:08, Jeff Law <l...@redhat.com> wrote:
> > >
> > > On 10/1/19 12:40 AM, Richard Biener wrote:
> > > > On Mon, 30 Sep 2019, Prathamesh Kulkarni wrote:
> > > >
> > > >> On Wed, 25 Sep 2019 at 23:44, Richard Biener <rguent...@suse.de> wrote:
> > > >>>
> > > >>> On Wed, 25 Sep 2019, Prathamesh Kulkarni wrote:
> > > >>>
> > > >>>> On Fri, 20 Sep 2019 at 15:20, Jeff Law <l...@redhat.com> wrote:
> > > >>>>>
> > > >>>>> On 9/19/19 10:19 AM, Prathamesh Kulkarni wrote:
> > > >>>>>> Hi,
> > > >>>>>> For PR91532, the dead store is trivially deleted if we place dse
> > > >>>>>> pass
> > > >>>>>> between ifcvt and vect. Would it be OK to add another instance of
> > > >>>>>> dse there ?
> > > >>>>>> Or should we add an ad-hoc "basic-block dse" sub-pass to ifcvt that
> > > >>>>>> will clean up the dead store ?
> > > >>>>> I'd hesitate to add another DSE pass. If there's one nearby could
> > > >>>>> we
> > > >>>>> move the existing pass?
> > > >>>> Well I think the nearest one is just after pass_warn_restrict. Not
> > > >>>> sure if it's a good
> > > >>>> idea to move it up from there ?
> > > >>>
> > > >>> You'll need it inbetween ifcvt and vect so it would be disabled
> > > >>> w/o vectorization, so no, that doesn't work.
> > > >>>
> > > >>> ifcvt already invokes SEME region value-numbering so if we had
> > > >>> MESE region DSE it could use that. Not sure if you feel like
> > > >>> refactoring DSE to work on regions - it currently uses a DOM
> > > >>> walk which isn't suited for that.
> > > >>>
> > > >>> if-conversion has a little "local" dead predicate compute removal
> > > >>> thingy (not that I like that), eventually it can be enhanced to
> > > >>> do the DSE you want? Eventually it should be moved after the local
> > > >>> CSE invocation though.
> > > >> Hi,
> > > >> Thanks for the suggestions.
> > > >> For now, would it be OK to do "dse" on loop header in
> > > >> tree_if_conversion, as in the attached patch ?
> > > >> The patch does local dse in a new function ifcvt_local_dse instead of
> > > >> ifcvt_local_dce, because it needed to be done after RPO VN which
> > > >> eliminates:
> > > >> Removing dead stmt _ifc__62 = *_55;
> > > >> and makes the following store dead:
> > > >> *_55 = _ifc__61;
> > > >
> > > > I suggested trying to move ifcvt_local_dce after RPO VN, you could
> > > > try that as independent patch (pre-approved).
> > > >
> > > > I don't mind the extra walk though.
> > > >
> > > > What I see as possible issue is that dse_classify_store walks virtual
> > > > uses and I'm not sure if the loop exit is a natural boundary for
> > > > such walk (eventually the loop header virtual PHI is reached but
> > > > there may also be a loop-closed PHI for the virtual operand,
> > > > but not necessarily). So the question is whether to add a
> > > > "stop at" argument to dse_classify_store specifying the virtual
> > > > use the walk should stop at?
> > > I think we want to stop at the block boundary -- aren't the cases we
> > > care about here local to a block?
> > This version restricts walking in dse_classify_store to basic-block if
> > bb_only is true,
> > and removes dead stores in ifcvt_local_dce instead of separate walk.
> > Does it look OK ?
>
> As relied to Jeff please make it trivially work for SESE region walks
> by specifying the exit virtual operand to stop on.
Thanks for the suggestions, does the attached patch look OK ?
Um, sorry, I don't really understand why we need to specify virtual
phi arg from back edge to stop the walk.
Would it achieve the same effect if we pass index of loop->latch to
stop the walk ?
Thanks,
Prathamesh
>
> Richard.
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index 822aae5b83f..1422e8a3374 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -120,6 +120,7 @@ along with GCC; see the file COPYING3. If not see
#include "fold-const.h"
#include "tree-ssa-sccvn.h"
#include "tree-cfgcleanup.h"
+#include "tree-ssa-dse.h"
/* Only handle PHIs with no more arguments unless we are asked to by
simd pragma. */
@@ -2960,6 +2961,39 @@ ifcvt_local_dce (basic_block bb)
while (!gsi_end_p (gsi))
{
stmt = gsi_stmt (gsi);
+ if (gimple_store_p (stmt))
+ {
+ tree lhs = gimple_get_lhs (stmt);
+ ao_ref write;
+ ao_ref_init (&write, lhs);
+
+ tree stop_at_vuse = NULL_TREE;
+ basic_block header = bb->loop_father->header;
+ for (gphi_iterator h_gsi = gsi_start_phis (header);
+ !gsi_end_p (h_gsi);
+ gsi_next (&h_gsi))
+ {
+ gphi *phi = h_gsi.phi ();
+ if (virtual_operand_p (gimple_phi_result (phi)))
+ for (int i = 0; i < gimple_phi_num_args (phi); ++i)
+ {
+ edge e = gimple_phi_arg_edge (phi, i);
+ if (e->src == bb->loop_father->latch)
+ {
+ stop_at_vuse = gimple_phi_arg_def (phi, i);
+ goto dse_classify_store_call;
+ }
+ }
+ }
+
+dse_classify_store_call:
+ if (dse_classify_store (&write, stmt, false, NULL, NULL, stop_at_vuse)
+ == DSE_STORE_DEAD)
+ delete_dead_or_redundant_assignment (&gsi, "dead");
+ gsi_next (&gsi);
+ continue;
+ }
+
if (gimple_plf (stmt, GF_PLF_2))
{
gsi_next (&gsi);
diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
index ba67884a825..123d2f61e44 100644
--- a/gcc/tree-ssa-dse.c
+++ b/gcc/tree-ssa-dse.c
@@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see
#include "params.h"
#include "alias.h"
#include "tree-ssa-loop.h"
+#include "tree-ssa-dse.h"
/* This file implements dead store elimination.
@@ -76,21 +77,13 @@ along with GCC; see the file COPYING3. If not see
fact, they are the same transformation applied to different views of
the CFG. */
-static void delete_dead_or_redundant_assignment (gimple_stmt_iterator *, const char *);
+void delete_dead_or_redundant_assignment (gimple_stmt_iterator *, const char *);
static void delete_dead_or_redundant_call (gimple_stmt_iterator *, const char *);
/* Bitmap of blocks that have had EH statements cleaned. We should
remove their dead edges eventually. */
static bitmap need_eh_cleanup;
-/* Return value from dse_classify_store */
-enum dse_store_status
-{
- DSE_STORE_LIVE,
- DSE_STORE_MAYBE_PARTIAL_DEAD,
- DSE_STORE_DEAD
-};
-
/* STMT is a statement that may write into memory. Analyze it and
initialize WRITE to describe how STMT affects memory.
@@ -662,10 +655,10 @@ dse_optimize_redundant_stores (gimple *stmt)
if only clobber statements influenced the classification result.
Returns the classification. */
-static dse_store_status
+dse_store_status
dse_classify_store (ao_ref *ref, gimple *stmt,
bool byte_tracking_enabled, sbitmap live_bytes,
- bool *by_clobber_p = NULL)
+ bool *by_clobber_p, tree stop_at_vuse)
{
gimple *temp;
int cnt = 0;
@@ -706,7 +699,9 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
FOR_EACH_IMM_USE_STMT (use_stmt, ui, defvar)
{
/* Limit stmt walking. */
- if (++cnt > PARAM_VALUE (PARAM_DSE_MAX_ALIAS_QUERIES_PER_STORE))
+ if (++cnt > PARAM_VALUE (PARAM_DSE_MAX_ALIAS_QUERIES_PER_STORE)
+ || (stop_at_vuse
+ && gimple_vuse (use_stmt) == stop_at_vuse))
{
fail = true;
BREAK_FROM_IMM_USE_STMT (ui);
@@ -901,7 +896,7 @@ delete_dead_or_redundant_call (gimple_stmt_iterator *gsi, const char *type)
/* Delete a dead store at GSI, which is a gimple assignment. */
-static void
+void
delete_dead_or_redundant_assignment (gimple_stmt_iterator *gsi, const char *type)
{
gimple *stmt = gsi_stmt (*gsi);
diff --git a/gcc/tree-ssa-dse.h b/gcc/tree-ssa-dse.h
new file mode 100644
index 00000000000..65be5667cd5
--- /dev/null
+++ b/gcc/tree-ssa-dse.h
@@ -0,0 +1,17 @@
+#ifndef TREE_SSA_DSE_H
+#define TREE_SSA_DSE_H
+
+/* Return value from dse_classify_store */
+enum dse_store_status
+{
+ DSE_STORE_LIVE,
+ DSE_STORE_MAYBE_PARTIAL_DEAD,
+ DSE_STORE_DEAD
+};
+
+dse_store_status dse_classify_store (ao_ref *, gimple *, bool, sbitmap,
+ bool * = NULL, tree = NULL);
+
+void delete_dead_or_redundant_assignment (gimple_stmt_iterator *, const char *);
+
+#endif
