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. Richard.
