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?
jeff
