| Message ID | g41v1qqcy5.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
Richard Sandiford writes: > Mikael Pettersson <mikpe@it.uu.se> writes: > > Richard Sandiford writes: > > > Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes: > > > > On 03/22/2011 06:48 PM, Richard Henderson wrote: > > > > > > > >> Ok. Watch out for other target problems this week. > > > > > > > > This unfortunately broke bootstrap on s390. > > > > > > This is PR 48263. Since it seems to be affecting several targets, > > > and since my bootstrap seems to be taking a looong time, I'll post > > > the patch here before testing has finished. > > > > > > > Just copying the pre-patch behaviour fixes the problem for me: > > > > > > I think we need to undo more of the patch, and leave the conversion > > > outside of the new interface. > > > > > > Sorry for the breakage. > > > > > > Richard > > > > > > > > > gcc/ > > > PR rtl-optimization/48263 > > > * optabs.c (expand_binop_directly): Reinstate convert_modes code > > > and original commutative_p handling. Use maybe_gen_insn. > > > > Unfortunately this fix (r171418) broke m68k-linux, see > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48332 > > Please try the attached > > Richard > > > gcc/ > PR rtl-optimization/48332 > * optabs.c (expand_binop_directly): Set xmodeN to the target-mandated > mode of input operand N and modeN to its actual mode. Thanks, this allowed me to build gcc-4.7 as a cross to m68k-linux again. I'm starting a native bootstrap of gcc-4.7 head + this fix on m68k, but that will take about 5 days to complete... /Mikael > > Index: gcc/optabs.c > =================================================================== > --- gcc/optabs.c 2011-03-24 17:23:05.000000000 +0000 > +++ gcc/optabs.c 2011-03-29 14:18:02.000000000 +0100 > @@ -1243,9 +1243,9 @@ expand_binop_directly (enum machine_mode > rtx last) > { > enum insn_code icode = optab_handler (binoptab, mode); > - 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; > struct expand_operand ops[3]; > bool commutative_p; > rtx pat; > @@ -1256,8 +1256,8 @@ expand_binop_directly (enum machine_mode > if we would swap the operands, we can save the conversions. */ > commutative_p = commutative_optab_p (binoptab); > if (commutative_p > - && GET_MODE (xop0) != mode0 && GET_MODE (xop1) != mode1 > - && GET_MODE (xop0) == mode1 && GET_MODE (xop1) == mode1) > + && GET_MODE (xop0) != xmode0 && GET_MODE (xop1) != xmode1 > + && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode1) > { > swap = xop0; > xop0 = xop1; > @@ -1265,9 +1265,9 @@ expand_binop_directly (enum machine_mode > } > > /* If we are optimizing, force expensive constants into a register. */ > - xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp); > + xop0 = avoid_expensive_constant (xmode0, binoptab, xop0, unsignedp); > if (!shift_optab_p (binoptab)) > - xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp); > + xop1 = avoid_expensive_constant (xmode1, binoptab, xop1, unsignedp); > > /* In case the insn wants input operands in modes different from > those of the actual operands, convert the operands. It would > @@ -1275,19 +1275,19 @@ expand_binop_directly (enum machine_mode > that they're properly zero-extended, sign-extended or truncated > for their 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; > + } > > /* If operation is commutative, > try to make the first operand a register. >
Mikael Pettersson <mikpe@it.uu.se> writes: > > gcc/ > > PR rtl-optimization/48332 > > * optabs.c (expand_binop_directly): Set xmodeN to the target-mandated > > mode of input operand N and modeN to its actual mode. > > Thanks, this allowed me to build gcc-4.7 as a cross to m68k-linux again. Good to hear, thanks for testing. > I'm starting a native bootstrap of gcc-4.7 head + this fix on m68k, but > that will take about 5 days to complete... Ouch. Hopefully this time I've finally got it right. Bootstrapped & regression-tested on x86_64-linux-gnu. OK to install? Richard > > > > Index: gcc/optabs.c > > =================================================================== > > --- gcc/optabs.c 2011-03-24 17:23:05.000000000 +0000 > > +++ gcc/optabs.c 2011-03-29 14:18:02.000000000 +0100 > > @@ -1243,9 +1243,9 @@ expand_binop_directly (enum machine_mode > > rtx last) > > { > > enum insn_code icode = optab_handler (binoptab, mode); > > - 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; > > struct expand_operand ops[3]; > > bool commutative_p; > > rtx pat; > > @@ -1256,8 +1256,8 @@ expand_binop_directly (enum machine_mode > > if we would swap the operands, we can save the conversions. */ > > commutative_p = commutative_optab_p (binoptab); > > if (commutative_p > > - && GET_MODE (xop0) != mode0 && GET_MODE (xop1) != mode1 > > - && GET_MODE (xop0) == mode1 && GET_MODE (xop1) == mode1) > > + && GET_MODE (xop0) != xmode0 && GET_MODE (xop1) != xmode1 > > + && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode1) > > { > > swap = xop0; > > xop0 = xop1; > > @@ -1265,9 +1265,9 @@ expand_binop_directly (enum machine_mode > > } > > > > /* If we are optimizing, force expensive constants into a register. */ > > - xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp); > > + xop0 = avoid_expensive_constant (xmode0, binoptab, xop0, unsignedp); > > if (!shift_optab_p (binoptab)) > > - xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp); > > + xop1 = avoid_expensive_constant (xmode1, binoptab, xop1, unsignedp); > > > > /* In case the insn wants input operands in modes different from > > those of the actual operands, convert the operands. It would > > @@ -1275,19 +1275,19 @@ expand_binop_directly (enum machine_mode > > that they're properly zero-extended, sign-extended or truncated > > for their 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; > > + } > > > > /* If operation is commutative, > > try to make the first operand a register. > >
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. 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? 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. r~
Richard Henderson <rth@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
On 03/30/2011 01:53 AM, Richard Sandiford wrote: > 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 Ok, I get it. The patch is ok as-is. r~
Index: gcc/optabs.c =================================================================== --- gcc/optabs.c 2011-03-24 17:23:05.000000000 +0000 +++ gcc/optabs.c 2011-03-29 14:18:02.000000000 +0100 @@ -1243,9 +1243,9 @@ expand_binop_directly (enum machine_mode rtx last) { enum insn_code icode = optab_handler (binoptab, mode); - 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; struct expand_operand ops[3]; bool commutative_p; rtx pat; @@ -1256,8 +1256,8 @@ expand_binop_directly (enum machine_mode if we would swap the operands, we can save the conversions. */ commutative_p = commutative_optab_p (binoptab); if (commutative_p - && GET_MODE (xop0) != mode0 && GET_MODE (xop1) != mode1 - && GET_MODE (xop0) == mode1 && GET_MODE (xop1) == mode1) + && GET_MODE (xop0) != xmode0 && GET_MODE (xop1) != xmode1 + && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode1) { swap = xop0; xop0 = xop1; @@ -1265,9 +1265,9 @@ expand_binop_directly (enum machine_mode } /* If we are optimizing, force expensive constants into a register. */ - xop0 = avoid_expensive_constant (mode0, binoptab, xop0, unsignedp); + xop0 = avoid_expensive_constant (xmode0, binoptab, xop0, unsignedp); if (!shift_optab_p (binoptab)) - xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp); + xop1 = avoid_expensive_constant (xmode1, binoptab, xop1, unsignedp); /* In case the insn wants input operands in modes different from those of the actual operands, convert the operands. It would @@ -1275,19 +1275,19 @@ expand_binop_directly (enum machine_mode that they're properly zero-extended, sign-extended or truncated for their 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; + } /* If operation is commutative, try to make the first operand a register.