On 06/21/2016 05:50 PM, James Greenhalgh wrote:
For the default implementation, if the parameters are not set, I just
multiply BRANCH_COST through by COSTS_N_INSNS (1) for size and
COSTS_N_INSNS (3) for speed. I know this is not ideal, but I'm still short
of ideas on how best to form the default implementation.
Yeah, this does seem kind of arbitrary. It looks especialy odd
considering that BRANCH_COST is likely to already vary between the
size/speed cases. What's wrong with just multiplying through by CNI(1)?
I'm not sure we want params for this; targets should just eventually
upgrade their cost models.
The new default causes some changes in generated conditional move sequences
for x86_64. Whether these changes are for the better or not I can't say.
How about arm/aarch64? I think some benchmark results might be good to have.
Bernhard already pointed out some issues with the patch; I'll omit these.
+(max_noce_ifcvt_seq_cost,
+ "This hook should return a value in the same units as\n\
+@code{TARGET_RTX_COSTS}, giving the maximum acceptable cost for\n\
+a sequence generated by the RTL if-conversion pass when conditional\n\
+execution is not available.
There's still the issue that we're also replacing instructions when
doing if-conversion. Let's say in this case,
/* Convert "if (test) x = a; else x = b", for A and B constant.
Also allow A = y + c1, B = y + c2, with a common y between A
and B. */
we're removing two assignments for the purposes of optimizing for size,
and one assignment when considering optimization for speed. This needs
to factor into the cost calculations somehow if we want to do it
properly. I think we can leave the hook as-is, but maybe add
documentation to the effect of "The caller should increase the limit by
the cost of whatever instructions are removed in the transformation."
+/* Default implementation of TARGET_RTX_BRANCH_COST. */
Wrong name for the hook.
+
+unsigned int
+default_max_noce_ifcvt_seq_cost (bool speed_p, edge e)
+{
+ bool predictable_p = predictable_edge_p (e);
+ /* For size, some targets like to set a BRANCH_COST of zero to disable
+ ifcvt, continue to allow that. Then multiply through by
+ COSTS_N_INSNS (1) so we're in a comparable base. */
+
+ if (!speed_p)
+ return BRANCH_COST (speed_p, predictable_p) * COSTS_N_INSNS (1);
Blank line before the comment would be more readable.
+ enum compiler_param param = predictable_p
+ ? PARAM_MAX_RTL_IF_CONVERSION_PREDICTABLE_COST
+ : PARAM_MAX_RTL_IF_CONVERSION_UNPREDICTABLE_COST;
When splitting expressions across multiple lines, wrap in parens so that
emacs formats them automatically.
Bernd
