This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR18589
On Wed, Apr 4, 2012 at 9:15 PM, William J. Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> On Wed, 2012-04-04 at 15:08 +0200, Richard Guenther wrote:
>> 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).
>
> 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.
Hm, naming them "reassoc1" and "reassoc2" is a hack. Naming both
"reassoc" will not trigger re-naming them to reassoc1 and reassoc2
I think. How ugly. Especially that -fdump-tree-reassoc will no longer
work. Maybe instead of using two pass structs resort to using
the existing hack with using first_pass_instance and TODO_mark_first_instance.
> 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.
>> >>
>> >
>> >
>>
>
>