On 7/8/19 1:39 AM, JiangNing OS wrote: > Hi Jeff and Richard B., > > Following your tips, I've found a much simpler solution in tree-ssa-phiopt.c. > Attached is the new patch. Review again, please! > > Thanks a lot! > -Jiangning > >> -----Original Message----- >> From: Jeff Law <l...@redhat.com> >> Sent: Saturday, June 22, 2019 12:10 AM >> To: Richard Biener <richard.guent...@gmail.com> >> Cc: JiangNing OS <jiangn...@os.amperecomputing.com>; gcc- >> patc...@gcc.gnu.org >> Subject: Re: [PATCH] improve ifcvt optimization (PR rtl-optimization/89430) >> >> On 6/21/19 6:23 AM, Richard Biener wrote: >>> That looks like a pretty easy form to analyze. I'd suggest looking >>> through tree-ssa-phiopt.c closely. There's several transformations in >>> there that share similarities with yours. >>> >>> >>> More specifically there is the conditional store elimination (cselim) >>> pass inside this file. >> That's the one I was thinking of. It looks reasonably close to the cases >> JiangNing is trying to capture. >> >> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>>> 2) My current solution fits into current back-end if-conversion >>> pass very well. I don't want to invent >>>> a new framework to solve this relatively small issue. Besides, >>> this back-end patch doesn't only >>>> enhance store speculation detection, but also fix a bug in the >>> original code. Understood, but I still wonder if we're better off >>> addressing this in gimple. >>> >>> >>> Traditionally if-conversions profitability heavily depends on the >>> target and esp. if memory is involved costing on GIMPLE is hard. >>> That's also one reason why we turned off loop if-conversion on GIMPLE >>> for non-vectorized code. >> Yea. That's always the concern for transformations that aren't trivial to >> show >> are better. >> >> Conditional store elimination as implemented right now in phiopt requires a >> single store in the then/else clauses. So we're really just sinking the >> stores >> through a PHI. We're really not changing the number of loads or stores on >> any path. >> >> In the cases I think JiangNing is trying to handle we are adding a store on a >> path where it didn't previously exist because there is no else clause. So we >> likely need to check the edge probabilities to avoid doing something dumb. I >> don't have a good sense where the cutoff should be. tree-ssa-sink might >> have some nuggets of information to help guide. >> >>> phiopt is really about followup optimization opportunities in passes >>> that do not handle conditionals well. >>> >>> cselim is on the border... >> ACK. In fact, looking at it it I wonder if it'd be better in >> tree-ssa-sink.c :-) >> >> >> jeff > > csel5.patch > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > index 9f83f75b0bf..1a7acf7f1ba 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,9 @@ > +2019-07-08 Jiangning Liu <jiangning....@amperecomputing.com> > + > + PR tree-optimization/89430 > + * tree-ssa-phiopt.c (cond_store_replacement): Support conditional > + store elimination for local variable without address escape. > + > 2019-07-07 Jeff Law <l...@redhat.com> > > PR tree-optimization/91090 > diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog > index 12e5bc167e0..65a34b43fb5 100644 > --- a/gcc/testsuite/ChangeLog > +++ b/gcc/testsuite/ChangeLog > @@ -1,3 +1,13 @@ > +2019-07-08 Jiangning Liu <jiangning....@amperecomputing.com> > + > + PR tree-optimization/89430 > + * gcc.dg/tree-ssa/pr89430-1.c: New test. > + * gcc.dg/tree-ssa/pr89430-2.c: New test. > + * gcc.dg/tree-ssa/pr89430-3.c: New test. > + * gcc.dg/tree-ssa/pr89430-4.c: New test. > + * gcc.dg/tree-ssa/pr89430-5.c: New test. > + * gcc.dg/tree-ssa/pr89430-6.c: New test. THanks!
I made a trivial update to the comment before cond_store_replacement and installed this patch on the trunk. jeff
