On Mon, 7 Oct 2019 at 03:11, Richard Biener <rguent...@suse.de> wrote:
>
> On Fri, 4 Oct 2019, Prathamesh Kulkarni wrote:
>
> > On Fri, 4 Oct 2019 at 12:18, Richard Biener <rguent...@suse.de> wrote:
> > >
> > > On Thu, 3 Oct 2019, Prathamesh Kulkarni wrote:
> > >
> > > > 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 ?
> > >
> > > Since DSE walks the virtual def->use chain it makes sense to stop
> > > it at a specific virtual definition. Note
> > >
> > > @@ -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))
> > > {
> > >
> > > can be done above this loop:
> > >
> > > diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> > > index ba67884a825..bc95c21e52c 100644
> > > --- a/gcc/tree-ssa-dse.c
> > > +++ b/gcc/tree-ssa-dse.c
> > > @@ -701,6 +701,9 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > }
> > > else
> > > defvar = gimple_vdef (temp);
> > > + /* If we are instructed to stop walking at region boundary, do so.
> > > */
> > > + if (defvar == stop_at_vuse)
> > > + return DSE_STORE_LIVE;
> > > auto_vec<gimple *, 10> defs;
> > > gimple *phi_def = NULL;
> > > FOR_EACH_IMM_USE_STMT (use_stmt, ui, defvar)
> > >
> > > The way you find the virtual PHI is also too complicated. I'd do
> > >
> > > @@ -2873,7 +2873,7 @@ ifcvt_split_critical_edges (class loop *loop, bool
> > > aggressive_if_conv)
> > > loop vectorization. */
> > >
> > > static void
> > > -ifcvt_local_dce (basic_block bb)
> > > +ifcvt_local_dce (class loop *loop)
> > > {
> > > gimple *stmt;
> > > gimple *stmt1;
> > > @@ -2890,6 +2890,10 @@ ifcvt_local_dce (basic_block bb)
> > > replace_uses_by (name_pair->first, name_pair->second);
> > > redundant_ssa_names.release ();
> > >
> > > + /* The loop has a single BB only. */
> > > + basic_block bb = loop->header;
> > > + tree latch_vdef = NULL_TREE;
> > > +
> > > worklist.create (64);
> > > /* Consider all phi as live statements. */
> > > for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > > @@ -2897,6 +2901,8 @@ ifcvt_local_dce (basic_block bb)
> > > phi = gsi_stmt (gsi);
> > > gimple_set_plf (phi, GF_PLF_2, true);
> > > worklist.safe_push (phi);
> > > + if (virtual_operand_p (gimple_phi_result (phi)))
> > > + latch_vdef = PHI_ARG_DEF_FROM_EDGE (phi, loop_latch_edge (loop));
> > > }
> > > /* Consider load/store statements, CALL and COND as live. */
> > > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > Hi,
> > Thanks a lot for the suggestions.
> > Does the attached version look OK ?
>
> Yes. Please put in the usual copyright header in the new tree-ssa-dse.h
> file though.
>
> OK with that change.
Thanks, committed the patch in r276681 after adding copyright header
and bootstrap+test on
x86_64-unknown-linux-gnu and aarch64-linux-gnu.
Thanks,
Prathamesh
>
> Thanks,
> Richard.
>
> > Thanks,
> > Prathamesh
> > >
> > >
> >
> > > > Thanks,
> > > > Prathamesh
> > > > >
> > > > > Richard.
> > > >
> > >
> > > --
> > > Richard Biener <rguent...@suse.de>
> > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> > > Germany; GF: Felix Imendörffer; HRB 247165 (AG München)
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 247165 (AG München)
