On Wed, 2012-04-04 at 15:08 +0200, Richard Guenther wrote: > 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).
Unfortunately this seems to be necessary if I name the two passes "reassoc1" and "reassoc2". If I try to name both of them "reassoc" I get failures in other tests like gfortran.dg/reassoc_4, where -fdump-tree-reassoc1 doesn't work. Unless I'm missing something obvious, I think I need to keep that change. Frankly I was surprised and relieved that there weren't more tests that used the generic -fdump-tree-reassoc. Thanks, Bill > > Thanks, > Richard. > > > Thanks, > > Bill > >> > >> Thanks, > >> Richard. > >> > > > > >
