Re: [PATCH] improve ifcvt optimization (PR rtl-optimization/89430)

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Jul 8, 2019 at 12:39 AM JiangNing OS
<jiangning@os.amperecomputing.com> 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!

  /* Prove that we can move the store down.  We could also check
     TREE_THIS_NOTRAP here, but in that case we also could move stores,
     whose value is not available readily, which we want to avoid.  */

I think the comment above the change needs to be changed or extended slightly.

Thanks,
Andrew Pinski

>
> Thanks a lot!
> -Jiangning
>
> > -----Original Message-----
> > From: Jeff Law <law@redhat.com>
> > Sent: Saturday, June 22, 2019 12:10 AM
> > To: Richard Biener <richard.guenther@gmail.com>
> > Cc: JiangNing OS <jiangning@os.amperecomputing.com>; gcc-
> > patches@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