On Wed, Jul 20, 2016 at 01:41:39PM +0200, Bernd Schmidt wrote:
> On 07/20/2016 11:51 AM, James Greenhalgh wrote:
>
> >
> >2016-07-20 James Greenhalgh <james.greenha...@arm.com>
> >
> > * target.def (max_noce_ifcvt_seq_cost): New.
> > * doc/tm.texi.in (TARGET_MAX_NOCE_IFCVT_SEQ_COST): Document it.
> > * doc/tm.texi: Regenerate.
> > * targhooks.h (default_max_noce_ifcvt_seq_cost): New.
> > * targhooks.c (default_max_noce_ifcvt_seq_cost): New.
> > * params.def (PARAM_MAX_RTL_IF_CONVERSION_PREDICTABLE_COST): New.
> > (PARAM_MAX_RTL_IF_CONVERSION_UNPREDICTABLE_COST): Likewise.
> > * doc/invoke.texi: Document new params.
>
> I think this is starting to look like a clear improvement, so I'll
> ack patches 1-3 with a few minor comments, and with the expectation
> that you'll address performance regressions on other targets if they
> occur.
I'll gladly take a look if I've caused anyone any trouble.
> Number 4 I still need to figure out.
>
> Minor details:
>
> >+ if (!speed_p)
> >+ {
> >+ return cost <= if_info->original_cost;
> >+ }
>
> No braces around single statements in ifs. There's an instance of
> this in patch 4 as well.
>
> >+ if (global_options_set.x_param_values[param])
> >+ return PARAM_VALUE (param);
>
> How about wrapping the param value into COSTS_N_INSNS, to make the
> value of the param less dependent on compiler internals?
I did consider this, but found it hard to word for the user documentation.
I found it easier to understand when it was in the same units as
rtx_cost, particularly as the AArch64 backend prints RTX costs to most
dump files (including ce1, ce2, ce3) so comparing directly was easy for me
to grok. I think going in either direction has the potential to confuse
users, the cost metrics of the RTL passes are very tightly coupled to
compiler internals.
I don't have a strong feeling either way, just a slight preference to keep
everything in the same units as rtx_cost where I can.
Let me know if you'd rather I follow this comment. There's some precedent
to wrapping it in COSTS_N_INSNS in GCSE_UNRESTRICTED_COST, but I find this
less clear than what I've done (well, I would say that :-) ).
> In patch 4:
>
> >+ /* Check that our new insn isn't going to clobber ORIG_OTHER_DEST. */
> >+ bool modified_in_x = (set_tmp != NULL_RTX)
> >+ && modified_in_p (orig_other_dest, set_tmp);
>
> Watch line wrapping. No parens around the first subexpression (there
> are other examples of unnecessary ones in invocations of
> noce_arith_helper), but around the full one.
I'll catch these and others on commit, thanks for pointing them out.
Thanks,
James
