Re: [PATCH] Fix PR18589

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Apr 4, 2012 at 2:35 PM, William J. Schmidt
<wschmidt@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
>> <wschmidt@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
>> >> <wschmidt@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.
>>
>
>