RE: [PATCH PR95855]A missing ifcvt optimization to generate fcsel
> On Tue, Jun 30, 2020 at 1:31 PM yangyang (ET) > wrote: > > > > > On Tue, Jun 30, 2020 at 4:29 AM yangyang (ET) > > > > > > wrote: > > > > > > > > Hi, > > > > > > > > > > Hi, > > > > > > > > > > > > This is a simple fix for pr95855. > > > > > > > > > > > > With this fix, pass_split_paths can recognize the > > > > > > if-conversion > > > > > opportunity of the testcase and doesn't duplicate the corresponding > block. > > > > > > > > > > > > Added one testcase for this. Bootstrap and tested on both > > > > > > aarch64 and > > > > > x86 Linux platform, no new regression witnessed. > > > > > > > > > > > > Ok for trunk? > > > > > > > > > > Can you try using the num_stmts_in_pred[12] counts instead of > > > > > using empty_block_p? > > > > > > > > It' ok to using num_stmts_in_pred[12] to judge whether the > > > > pred[12] is empty since bb's immediate dominator can't meet the > > > > constraints "single_pred_p (pred[12]) && single_pred (pred[12]) == > pred[21]". > > > > > > > > > > > > > > Your matching doesn't allow for FP constants like > > > > > > > > > > dmax[0] = d1[i] < 1.0 ? 1.0 : d1[i]; > > > > > > > > > > since FP constants are not shared. You likely want to use > > > > > operand_equal_p to do the PHI argument comparison. > > > > > > > > That's right, after using operand_equal_p instead of == to do the > > > > PHI argument Comparison, the mentioned case can be covered as well. > > > > > > > > > > > > > > Thanks, > > > > > Richard. > > > > > > > > Thanks for your suggestions. We have revised our patch based on > > > > your > > > suggestions. > > > > > > > > Bootstrap and tested on both aarch64 and x86 Linux platform. Does > > > > the v1 > > > patch looks better? > > > > > > Yes. This variant is OK. > > > > > > Thanks, > > > Richard. > > > > Thanks for reviewing this. Could you please help install it? > > Installed. Please double-check your ChangeLog with gcc-verify, it had a typo > in > the testcase filename. > > Richard. Thanks and Sorry for the typo, we will double-check it next time. Yang Yang
Re: [PATCH PR95855]A missing ifcvt optimization to generate fcsel
On Tue, Jun 30, 2020 at 1:31 PM yangyang (ET) wrote: > > > On Tue, Jun 30, 2020 at 4:29 AM yangyang (ET) > > wrote: > > > > > > Hi, > > > > > > > > Hi, > > > > > > > > > > This is a simple fix for pr95855. > > > > > > > > > > With this fix, pass_split_paths can recognize the > > > > > if-conversion > > > > opportunity of the testcase and doesn't duplicate the corresponding > > > > block. > > > > > > > > > > Added one testcase for this. Bootstrap and tested on both > > > > > aarch64 and > > > > x86 Linux platform, no new regression witnessed. > > > > > > > > > > Ok for trunk? > > > > > > > > Can you try using the num_stmts_in_pred[12] counts instead of using > > > > empty_block_p? > > > > > > It' ok to using num_stmts_in_pred[12] to judge whether the pred[12] is > > > empty since bb's immediate dominator can't meet the constraints > > > "single_pred_p (pred[12]) && single_pred (pred[12]) == pred[21]". > > > > > > > > > > > Your matching doesn't allow for FP constants like > > > > > > > > dmax[0] = d1[i] < 1.0 ? 1.0 : d1[i]; > > > > > > > > since FP constants are not shared. You likely want to use > > > > operand_equal_p to do the PHI argument comparison. > > > > > > That's right, after using operand_equal_p instead of == to do the PHI > > > argument Comparison, the mentioned case can be covered as well. > > > > > > > > > > > Thanks, > > > > Richard. > > > > > > Thanks for your suggestions. We have revised our patch based on your > > suggestions. > > > > > > Bootstrap and tested on both aarch64 and x86 Linux platform. Does the v1 > > patch looks better? > > > > Yes. This variant is OK. > > > > Thanks, > > Richard. > > Thanks for reviewing this. Could you please help install it? Installed. Please double-check your ChangeLog with gcc-verify, it had a typo in the testcase filename. Richard. > Yang Yang
RE: [PATCH PR95855]A missing ifcvt optimization to generate fcsel
> On Tue, Jun 30, 2020 at 4:29 AM yangyang (ET) > wrote: > > > > Hi, > > > > > > Hi, > > > > > > > > This is a simple fix for pr95855. > > > > > > > > With this fix, pass_split_paths can recognize the > > > > if-conversion > > > opportunity of the testcase and doesn't duplicate the corresponding block. > > > > > > > > Added one testcase for this. Bootstrap and tested on both > > > > aarch64 and > > > x86 Linux platform, no new regression witnessed. > > > > > > > > Ok for trunk? > > > > > > Can you try using the num_stmts_in_pred[12] counts instead of using > > > empty_block_p? > > > > It' ok to using num_stmts_in_pred[12] to judge whether the pred[12] is > > empty since bb's immediate dominator can't meet the constraints > > "single_pred_p (pred[12]) && single_pred (pred[12]) == pred[21]". > > > > > > > > Your matching doesn't allow for FP constants like > > > > > > dmax[0] = d1[i] < 1.0 ? 1.0 : d1[i]; > > > > > > since FP constants are not shared. You likely want to use > > > operand_equal_p to do the PHI argument comparison. > > > > That's right, after using operand_equal_p instead of == to do the PHI > > argument Comparison, the mentioned case can be covered as well. > > > > > > > > Thanks, > > > Richard. > > > > Thanks for your suggestions. We have revised our patch based on your > suggestions. > > > > Bootstrap and tested on both aarch64 and x86 Linux platform. Does the v1 > patch looks better? > > Yes. This variant is OK. > > Thanks, > Richard. Thanks for reviewing this. Could you please help install it? Yang Yang
Re: [PATCH PR95855]A missing ifcvt optimization to generate fcsel
On Tue, Jun 30, 2020 at 4:29 AM yangyang (ET) wrote: > > Hi, > > > > Hi, > > > > > > This is a simple fix for pr95855. > > > > > > With this fix, pass_split_paths can recognize the if-conversion > > opportunity of the testcase and doesn't duplicate the corresponding block. > > > > > > Added one testcase for this. Bootstrap and tested on both aarch64 and > > x86 Linux platform, no new regression witnessed. > > > > > > Ok for trunk? > > > > Can you try using the num_stmts_in_pred[12] counts instead of using > > empty_block_p? > > It' ok to using num_stmts_in_pred[12] to judge whether the pred[12] is > empty since bb's immediate dominator can't meet the constraints > "single_pred_p (pred[12]) && single_pred (pred[12]) == pred[21]". > > > > > Your matching doesn't allow for FP constants like > > > > dmax[0] = d1[i] < 1.0 ? 1.0 : d1[i]; > > > > since FP constants are not shared. You likely want to use operand_equal_p > > to > > do the PHI argument comparison. > > That's right, after using operand_equal_p instead of == to do the PHI argument > Comparison, the mentioned case can be covered as well. > > > > > Thanks, > > Richard. > > Thanks for your suggestions. We have revised our patch based on your > suggestions. > > Bootstrap and tested on both aarch64 and x86 Linux platform. Does the v1 > patch looks better? Yes. This variant is OK. Thanks, Richard. > Yang Yang > > +2020-06-30 Yang Yang > + > + PR tree-optimization/95855 > + * gimple-ssa-split-paths.c (is_feasible_trace): Add extra > + checks to recognize a missed if-conversion opportunity when > + judging whether to duplicate a block. > + > > +2020-06-30 Yang Yang > + > + PR tree-optimization/95855 > + * gcc.dg/tree-ssa/split-paths-12.c: New testcase. > +
RE: [PATCH PR95855]A missing ifcvt optimization to generate fcsel
Hi, > > Hi, > > > > This is a simple fix for pr95855. > > > > With this fix, pass_split_paths can recognize the if-conversion > opportunity of the testcase and doesn't duplicate the corresponding block. > > > > Added one testcase for this. Bootstrap and tested on both aarch64 and > x86 Linux platform, no new regression witnessed. > > > > Ok for trunk? > > Can you try using the num_stmts_in_pred[12] counts instead of using > empty_block_p? It' ok to using num_stmts_in_pred[12] to judge whether the pred[12] is empty since bb's immediate dominator can't meet the constraints "single_pred_p (pred[12]) && single_pred (pred[12]) == pred[21]". > > Your matching doesn't allow for FP constants like > > dmax[0] = d1[i] < 1.0 ? 1.0 : d1[i]; > > since FP constants are not shared. You likely want to use operand_equal_p to > do the PHI argument comparison. That's right, after using operand_equal_p instead of == to do the PHI argument Comparison, the mentioned case can be covered as well. > > Thanks, > Richard. Thanks for your suggestions. We have revised our patch based on your suggestions. Bootstrap and tested on both aarch64 and x86 Linux platform. Does the v1 patch looks better? Yang Yang +2020-06-30 Yang Yang + + PR tree-optimization/95855 + * gimple-ssa-split-paths.c (is_feasible_trace): Add extra + checks to recognize a missed if-conversion opportunity when + judging whether to duplicate a block. + +2020-06-30 Yang Yang + + PR tree-optimization/95855 + * gcc.dg/tree-ssa/split-paths-12.c: New testcase. + PR95855-v1.patch Description: PR95855-v1.patch
Re: [PATCH PR95855]A missing ifcvt optimization to generate fcsel
On Sun, Jun 28, 2020 at 2:32 PM yangyang (ET) wrote: > > Hi, > > This is a simple fix for pr95855. > > With this fix, pass_split_paths can recognize the if-conversion > opportunity of the testcase and doesn't duplicate the corresponding block. > > Added one testcase for this. Bootstrap and tested on both aarch64 and x86 > Linux platform, no new regression witnessed. > > Ok for trunk? Can you try using the num_stmts_in_pred[12] counts instead of using empty_block_p? Your matching doesn't allow for FP constants like dmax[0] = d1[i] < 1.0 ? 1.0 : d1[i]; since FP constants are not shared. You likely want to use operand_equal_p to do the PHI argument comparison. Thanks, Richard. > Thanks, > Yang Yang > > > +2020-06-28 Yang Yang > + > + PR tree-optimization/95855 > + * gimple-ssa-split-paths.c (is_feasible_trace): Add extra > + checks to recognize a missed if-conversion opportunity when > + judging whether to duplicate a block. > + > > +2020-06-28 Yang Yang > + > + PR tree-optimization/95855 > + * gcc.dg/tree-ssa/split-paths-12.c: New testcase. > +
