This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Bad choices by expand_mult_highpart
Sorry for the breakage, and thanks for taking so much trouble to fix it.
Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de> writes:
> However, it would appear that the problem is caused by trying to do
> a signed multiply, which synth_mult doesn't actually support. To fix
> this requires an adjustment like the one performed by
> expand_mult_highpart_adjust.
And later...
> Unfortunately it failed bootstrap on s390 in this form. There are two
> additional problems: if the mode size is smaller than HOST_BITS_PER_WIDE_INT,
> we need to clear the high bits in the integer constant before passing it
> to choose_mult_variant, [...]
And associated hunk:
> *************** expand_mult_highpart (enum machine_mode
> *** 2986,3007 ****
> abort ();
>
> op1 = gen_int_mode (cnst1, mode);
>
> /* See whether shift/add multiplication is cheap enough. */
> ! if (choose_mult_variant (mode, cnst1, &alg, &variant)
> ! && (alg.cost += shift_cost[GET_MODE_BITSIZE (mode) - 1]) < max_cost)
> {
> /* See whether the specialized multiplication optabs are
> cheaper than the shift/add version. */
> tem = expand_mult_highpart_optab (mode, op0, op1, target,
> ! unsignedp, alg.cost);
> if (tem)
> return tem;
>
> ! wider_mode = GET_MODE_WIDER_MODE (mode);
> ! op0 = convert_to_mode (wider_mode, op0, unsignedp);
> ! tem = expand_mult_const (wider_mode, op0, cnst1, 0, &alg, variant);
> ! return extract_high_half (mode, tem);
> }
> return expand_mult_highpart_optab (mode, op0, op1, target,
> unsignedp, max_cost);
> --- 2988,3032 ----
> abort ();
>
> op1 = gen_int_mode (cnst1, mode);
> + cnst1 &= GET_MODE_MASK (mode);
> +
> + /* We can't optimize modes wider than BITS_PER_WORD.
> + ??? We might be able to perform double-word arithmetic if
> + mode == word_mode, however all the cost calculations in
> + synth_mult etc. assume single-word operations. */
> + if (GET_MODE_BITSIZE (wider_mode) > BITS_PER_WORD)
> + return expand_mult_highpart_optab (mode, op0, op1, target,
> + unsignedp, max_cost);
> +
> + extra_cost = shift_cost[GET_MODE_BITSIZE (mode) - 1];
> +
> + /* Check whether we try to multiply by a negative constant. */
> + if (!unsignedp && ((cnst1 >> (GET_MODE_BITSIZE (mode) - 1)) & 1))
> + {
> + sign_adjust = true;
> + extra_cost += add_cost;
> + }
>
> /* See whether shift/add multiplication is cheap enough. */
> ! if (choose_mult_variant (wider_mode, cnst1, &alg, &variant,
> ! max_cost - extra_cost))
> {
> /* See whether the specialized multiplication optabs are
> cheaper than the shift/add version. */
> tem = expand_mult_highpart_optab (mode, op0, op1, target,
> ! unsignedp, alg.cost + extra_cost);
> if (tem)
> return tem;
>
> ! tem = convert_to_mode (wider_mode, op0, unsignedp);
> ! tem = expand_mult_const (wider_mode, tem, cnst1, 0, &alg, variant);
> ! tem = extract_high_half (mode, tem);
> !
> ! /* Adjust result for signedness. */
> ! if (sign_adjust)
> ! tem = force_operand (gen_rtx_MINUS (mode, tem, op0), tem);
> !
> ! return tem;
> }
> return expand_mult_highpart_optab (mode, op0, op1, target,
> unsignedp, max_cost);
>
I don't understand why the masking and special handling of negative
constants is necessary. What does synth_mult do wrong wrt negative
constants? I ask because (a) neg_variant involves using synth_mult
with the negative of the original constant and (b) expand_mult doesn't
do anything like this.
That said, part of the reason for (b) might be:
/* synth_mult does an `unsigned int' multiply. As long as the mode is
less than or equal in size to `unsigned int' this doesn't matter.
If the mode is larger than `unsigned int', then synth_mult works only
if the constant value exactly fits in an `unsigned int' without any
truncation. This means that multiplying by negative values does
not work; results are off by 2^32 on a 32 bit machine. */
[...]
else if (HOST_BITS_PER_INT < GET_MODE_BITSIZE (mode)
&& GET_CODE (op1) == CONST_INT
&& INTVAL (op1) < 0)
const_op1 = 0;
which seems very out of date. synth_mult seems to operate on
'unsigned HOST_WIDE_INT' and any CONST_INT by definition will
be HOST_WIDE_INT or smaller.
Richard