On Wed, Apr 4, 2012 at 2:35 PM, William J. Schmidt <wschm...@linux.vnet.ibm.com> wrote: > On Wed, 2012-04-04 at 13:35 +0200, Richard Guenther wrote: >> On Tue, Apr 3, 2012 at 10:25 PM, William J. Schmidt >> <wschm...@linux.vnet.ibm.com> wrote: >> > >> > >> > On Wed, 2012-03-28 at 15:57 +0200, Richard Guenther wrote: >> >> On Tue, Mar 6, 2012 at 9:49 PM, William J. Schmidt >> >> <wschm...@linux.vnet.ibm.com> wrote: >> >> > Hi, >> >> > >> >> > This is a re-post of the patch I posted for comments in January to >> >> > address http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18589. The patch >> >> > modifies reassociation to expose repeated factors from __builtin_pow* >> >> > calls, optimally reassociate repeated factors, and possibly reconstitute >> >> > __builtin_powi calls from the results of reassociation. >> >> > >> >> > Bootstrapped and passes regression tests for powerpc64-linux-gnu. I >> >> > expect there may need to be some small changes, but I am targeting this >> >> > for trunk approval. >> >> > >> >> > Thanks very much for the review, >> >> >> >> Hmm. How much work would it be to extend the reassoc 'IL' to allow >> >> a repeat factor per op? I realize what you do is all within what reassoc >> >> already does though ideally we would not require any GIMPLE IL changes >> >> for building up / optimizing the reassoc IL but only do so when we commit >> >> changes. >> >> >> >> Thanks, >> >> Richard. >> > >> > Hi Richard, >> > >> > I've revised my patch along these lines; see the new version below. >> > While testing it I realized I could do a better job of reducing the >> > number of multiplies, so there are some changes to that logic as well, >> > and a couple of additional test cases. Regstrapped successfully on >> > powerpc64-linux. >> > >> > Hope this looks better! >> >> Yes indeed. A few observations though. You didn't integrate >> attempt_builtin_powi >> with optimize_ops_list - presumably because it's result does not really fit >> the single-operation assumption? But note that undistribute_ops_list and >> optimize_range_tests have the same issue. Thus, I'd have prefered if >> attempt_builtin_powi worked in the same way, remove the parts of the >> ops list it consumed and stick an operand for its result there instead. >> That should simplify things (not having that special powi_result) and >> allow for multiple "powi results" in a single op list? > > Multiple powi results are already handled, but yes, what you're > suggesting would simplify things by eliminating the need to create > explicit multiplies to join them and the cached-multiply results > together. Sounds reasonable on the surface; it just hadn't occurred to > me to do it this way. I'll have a look. > > Any other major concerns while I'm reworking this?
No, the rest looks fine (you should not need to repace -fdump-tree-reassoc-details with -fdump-tree-reassoc1-details -fdump-tree-reassoc2-details in the first testcase). Thanks, Richard. > Thanks, > Bill >> >> Thanks, >> Richard. >> > >
