Richard Henderson <r...@redhat.com> writes:
> On 03/29/2011 06:21 AM, Richard Sandiford wrote:
>> - enum machine_mode mode0 = insn_data[(int) icode].operand[1].mode;
>> - enum machine_mode mode1 = insn_data[(int) icode].operand[2].mode;
>> - enum machine_mode tmp_mode;
>> + enum machine_mode xmode0 = insn_data[(int) icode].operand[1].mode;
>> + enum machine_mode xmode1 = insn_data[(int) icode].operand[2].mode;
>> + enum machine_mode mode0, mode1, tmp_mode;
> ...
>> - if (GET_MODE (xop0) != mode0 && mode0 != VOIDmode)
>> - xop0 = convert_modes (mode0,
>> - GET_MODE (xop0) != VOIDmode
>> - ? GET_MODE (xop0)
>> - : mode,
>> - xop0, unsignedp);
>> -
>> - if (GET_MODE (xop1) != mode1 && mode1 != VOIDmode)
>> - xop1 = convert_modes (mode1,
>> - GET_MODE (xop1) != VOIDmode
>> - ? GET_MODE (xop1)
>> - : mode,
>> - xop1, unsignedp);
>> + mode0 = GET_MODE (xop0) != VOIDmode ? GET_MODE (xop0) : mode;
>> + if (xmode0 != VOIDmode && xmode0 != mode0)
>> + {
>> + xop0 = convert_modes (xmode0, mode0, xop0, unsignedp);
>> + mode0 = xmode0;
>> + }
>> +
>> + mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode;
>> + if (xmode1 != VOIDmode && xmode1 != mode1)
>> + {
>> + xop1 = convert_modes (xmode1, mode1, xop1, unsignedp);
>> + mode1 = xmode1;
>> + }
>
> This smells like a target bug, but I can't quite put my finger
> on exactly what's wrong, and I haven't debugged the original.
>
> The backend has said xmode[01] is what it expects. The only
> reason you wouldn't write xmode[01] in the create_input_operand
> line is if xmode[01] are VOIDmode.
Right. Sorry, I should have said, but the failure was actually from:
case EXPAND_INPUT:
input:
gcc_assert (mode != VOIDmode);
expand_binop_direct passed the match_operand's mode to create_input_operand,
which was really the opposite of the intention. The caller should be
passing the mode of the rtx, which it should always "know".
> That seems like mistake number one, particularly for a binop,
> but I'll accept that for the nonce. Which pattern triggered
> the problem in the target?
It was ashrdi3:
(define_expand "ashrdi3"
[(set (match_operand:DI 0 "register_operand" "")
(ashiftrt:DI (match_operand:DI 1 "register_operand" "")
(match_operand 2 "const_int_operand" "")))]
"!TARGET_COLDFIRE"
Which seems reasonable in this context, since the shift count isn't
really DImode.
> Then we've got the conditionalization in the init of mode[01],
> which is presumably to handle CONST_INT as an input.
>
> What about something like
>
> xmode0 = insn_data...
> if (xmode0 == VOIDmode)
> xmode0 = mode;
>
> mode0 = GET_MODE (xop0);
> if (mode0 == VOIDmode)
> mode0 = mode;
>
> if (xmode0 != mode0)
> convert_modes
>
> create_input_operand(..., xmode0)
>
> ? That seems more obvious than what you have. And I *think*
> it should produce the same results. If it doesn't, then this
> whole block of code needs a lot more explanation.
The problem is that a VOIDmode match_operand doesn't necessary imply
that "mode" is the right mode. VOIDmode register-accepting operands
are only a warning, not an error, and are sometimes needed for
flag-specific variations. They've traditionally not forced a conversion
to "mode". E.g. if you have something like this on a 32-bit target:
unsigned long long foo (unsigned long long x, int y)
{
return x >>= y;
}
then op1 will be (reg:SI y). Having a VOIDmode match_operand shouldn't
force that to be converted to (reg:DI y), whereas I think the sequence
above would.
Or to put it another way: as things stand, "mode" is only trustworthy
for CONST_INT opNs. A VOIDmode match_operand doesn't imply that the
opN argument to expand_binop_directly is a CONST_INT, or even that
only CONST_INTs are acceptable to the target.
This comes back to the point that we really should know up-front what
modes op0 and op1 actually have. (The thing I left as a future clean-up.)
Until then, the process implemented by yesterday's patch was supposed to be:
- work out what mode opN actually has at this point in time
- see if the target has specifically asked for a different mode
- if so, convert the operand
This is directly equivalent to what create_convert_operand_from does:
case EXPAND_CONVERT_FROM:
if (GET_MODE (op->value) != VOIDmode)
mode = GET_MODE (op->value);
else
/* The caller must tell us what mode this value has. */
gcc_assert (mode != VOIDmode);
imode = insn_data[(int) icode].operand[opno].mode;
if (imode != VOIDmode && imode != mode)
{
op->value = convert_modes (imode, mode, op->value, op->unsigned_p);
mode = imode;
}
But we have to break the flow there (rather than go on to coerce the operands)
because of the commutative thing.
Richard
