Re: Cleaning up expand optabs code

2011-04-01 Thread Georg-Johann Lay 
Richard Sandiford schrieb:
> Georg-Johann Lay  writes:
>> Richard Henderson schrieb:
>>> On 03/25/2011 05:41 AM, Georg-Johann Lay wrote:
> On 03/22/2011 06:48 PM, Richard Henderson wrote:
>
>> Ok.  Watch out for other target problems this week.
 libgcc fails to build for avr (SVN 171446)

 ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c: In function
 '__negdi2':
 ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c:68:17:
 internal compiler error: in maybe_gen_insn, at  optabs.c:7123
>>> This is due to a miscommunication between the middle-end and the backend
>>> about how many arguments the setmemhi pattern takes.
>>>
>>> (define_expand "setmemhi"
>>>   [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>>>(match_operand 2 "const_int_operand" ""))
>>>   (use (match_operand:HI 1 "const_int_operand" ""))
>>>   (use (match_operand:HI 3 "const_int_operand" "n"))
>>>   (clobber (match_scratch:HI 4 ""))
>>>   (clobber (match_dup 5))])]
>>>
>>> The match_scratch is counted in .n_operands, which makes the count of
>>> operands not equal 4, so we assume 6 operands are necessary.  We can
>>> fix this for the special case of avr by only assuming 6 operands when
>>> there are in fact 6 operands, but of course this could fail just as
>>> easily if there were two scratches.
>>>
>>> All of which suggests that optional arguments to a named optab is a
>>> mistake that ought to be rectified.
>>>
>>> I plan to commit the following after bootstrap and check.
>>>
>>>
>> Hi, there is still trouble with "setmemhi" on avr, again for the
>> negdi2 from libgcc:
>>
>> #1  0x0839ccd7 in maybe_legitimize_operand (icode=CODE_FOR_setmemhi,
>> opno=4, op=0xbfffd22c) at ../../../gcc.gnu.org/trunk/gcc/optabs.c:7024
>> (gdb) p *op
>> $11 = {type = EXPAND_OUTPUT, unsigned_p = 0, unused = 0, mode =
>> VOIDmode, value = 0xb7d63360}
>> (gdb) p op->value
>> $12 = (rtx) 0xb7d63360
>> (gdb) pr
>> (use:CC (nil))
>>
>>
>> i.e. there is garbage in op->value
>>
>> (gdb)
>> (gdb) frame 0
>> #0  fancy_abort (file=0x88099f0
>> "../../../gcc.gnu.org/trunk/gcc/optabs.c", line=7024,
>> function=0x880a280 "maybe_legitimize_operand") at
>> ../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
>> (gdb)
> 
> Yeah, as things stand, we need to set nops to 4 if was originally 5.
> 
> I'm testing a series of patches to make the number of generator
> arguments available in insn_data.  I'll post them once they pass
> (hopefully later today).
> 
> Richard

Thanks, I can build avr-gcc again.

Johann

Re: Cleaning up expand optabs code

2011-03-31 Thread Richard Sandiford 
Georg-Johann Lay  writes:
> Richard Henderson schrieb:
>> On 03/25/2011 05:41 AM, Georg-Johann Lay wrote:
 On 03/22/2011 06:48 PM, Richard Henderson wrote:

> Ok.  Watch out for other target problems this week.
>>> libgcc fails to build for avr (SVN 171446)
>>>
>>> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c: In function
>>> '__negdi2':
>>> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c:68:17:
>>> internal compiler error: in maybe_gen_insn, at  optabs.c:7123
>> 
>> This is due to a miscommunication between the middle-end and the backend
>> about how many arguments the setmemhi pattern takes.
>> 
>> (define_expand "setmemhi"
>>   [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>>(match_operand 2 "const_int_operand" ""))
>>   (use (match_operand:HI 1 "const_int_operand" ""))
>>   (use (match_operand:HI 3 "const_int_operand" "n"))
>>   (clobber (match_scratch:HI 4 ""))
>>   (clobber (match_dup 5))])]
>> 
>> The match_scratch is counted in .n_operands, which makes the count of
>> operands not equal 4, so we assume 6 operands are necessary.  We can
>> fix this for the special case of avr by only assuming 6 operands when
>> there are in fact 6 operands, but of course this could fail just as
>> easily if there were two scratches.
>> 
>> All of which suggests that optional arguments to a named optab is a
>> mistake that ought to be rectified.
>> 
>> I plan to commit the following after bootstrap and check.
>> 
>> 
>
> Hi, there is still trouble with "setmemhi" on avr, again for the
> negdi2 from libgcc:
>
> #1  0x0839ccd7 in maybe_legitimize_operand (icode=CODE_FOR_setmemhi,
> opno=4, op=0xbfffd22c) at ../../../gcc.gnu.org/trunk/gcc/optabs.c:7024
> (gdb) p *op
> $11 = {type = EXPAND_OUTPUT, unsigned_p = 0, unused = 0, mode =
> VOIDmode, value = 0xb7d63360}
> (gdb) p op->value
> $12 = (rtx) 0xb7d63360
> (gdb) pr
> (use:CC (nil))
>
>
> i.e. there is garbage in op->value
>
> (gdb)
> (gdb) frame 0
> #0  fancy_abort (file=0x88099f0
> "../../../gcc.gnu.org/trunk/gcc/optabs.c", line=7024,
> function=0x880a280 "maybe_legitimize_operand") at
> ../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
> (gdb)

Yeah, as things stand, we need to set nops to 4 if was originally 5.

I'm testing a series of patches to make the number of generator
arguments available in insn_data.  I'll post them once they pass
(hopefully later today).

Richard

Re: Cleaning up expand optabs code

2011-03-31 Thread Georg-Johann Lay 
Richard Henderson schrieb:
> On 03/25/2011 05:41 AM, Georg-Johann Lay wrote:
>>> On 03/22/2011 06:48 PM, Richard Henderson wrote:
>>>
 Ok.  Watch out for other target problems this week.
>> libgcc fails to build for avr (SVN 171446)
>>
>> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c: In function
>> '__negdi2':
>> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c:68:17:
>> internal compiler error: in maybe_gen_insn, at  optabs.c:7123
> 
> This is due to a miscommunication between the middle-end and the backend
> about how many arguments the setmemhi pattern takes.
> 
> (define_expand "setmemhi"
>   [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>(match_operand 2 "const_int_operand" ""))
>   (use (match_operand:HI 1 "const_int_operand" ""))
>   (use (match_operand:HI 3 "const_int_operand" "n"))
>   (clobber (match_scratch:HI 4 ""))
>   (clobber (match_dup 5))])]
> 
> The match_scratch is counted in .n_operands, which makes the count of
> operands not equal 4, so we assume 6 operands are necessary.  We can
> fix this for the special case of avr by only assuming 6 operands when
> there are in fact 6 operands, but of course this could fail just as
> easily if there were two scratches.
> 
> All of which suggests that optional arguments to a named optab is a
> mistake that ought to be rectified.
> 
> I plan to commit the following after bootstrap and check.
> 
> 

Hi, there is still trouble with "setmemhi" on avr, again for the
negdi2 from libgcc:

#1  0x0839ccd7 in maybe_legitimize_operand (icode=CODE_FOR_setmemhi,
opno=4, op=0xbfffd22c) at ../../../gcc.gnu.org/trunk/gcc/optabs.c:7024
(gdb) p *op
$11 = {type = EXPAND_OUTPUT, unsigned_p = 0, unused = 0, mode =
VOIDmode, value = 0xb7d63360}
(gdb) p op->value
$12 = (rtx) 0xb7d63360
(gdb) pr
(use:CC (nil))


i.e. there is garbage in op->value

(gdb)
(gdb) frame 0
#0  fancy_abort (file=0x88099f0
"../../../gcc.gnu.org/trunk/gcc/optabs.c", line=7024,
function=0x880a280 "maybe_legitimize_operand") at
../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
(gdb)

Sources are as of svn trunk 4.7 (SVN 171773)

Reading specs from /mnt/nfs/home/georg/gnu/build/gcc-4.6-avr/gcc/specs
COLLECT_GCC=/mnt/nfs/home/georg/gnu/build/gcc-4.6-avr/gcc/xgcc
COLLECT_LTO_WRAPPER=/mnt/nfs/home/georg/gnu/build/gcc-4.6-avr/gcc/lto-wrapper
Target: avr
Configured with: ../../gcc.gnu.org/trunk/configure --target=avr
--prefix=/local/gnu/install/gcc-4.6 --enable-languages=c,c++
--disable-libssp --disable-libada --disable-nls --disable-shared

Johann

Re: Cleaning up expand optabs code

2011-03-30 Thread Richard Henderson 
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~

Re: Cleaning up expand optabs code

2011-03-30 Thread Richard Sandiford 
Richard Henderson  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 (rathe

Re: Cleaning up expand optabs code

2011-03-29 Thread Richard Henderson 
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~

Re: Cleaning up expand optabs code

2011-03-29 Thread Richard Sandiford 
Mikael Pettersson  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.0 +
>  > +++ gcc/optabs.c   2011-03-29 14:18:02.0 +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.
>  >

Re: Cleaning up expand optabs code

2011-03-29 Thread Mikael Pettersson 
Richard Sandiford writes:
 > Mikael Pettersson  writes:
 > > Richard Sandiford writes:
 > >  > Andreas Krebbel  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.0 +
 > +++ gcc/optabs.c 2011-03-29 14:18:02.0 +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.
 >

Re: Cleaning up expand optabs code

2011-03-29 Thread Richard Sandiford 
Mikael Pettersson  writes:
> Richard Sandiford writes:
>  > Andreas Krebbel  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.

Index: gcc/optabs.c
===
--- gcc/optabs.c2011-03-24 17:23:05.0 +
+++ gcc/optabs.c2011-03-29 14:18:02.0 +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.

Re: Cleaning up expand optabs code

2011-03-29 Thread Mikael Pettersson 
Richard Sandiford writes:
 > Andreas Krebbel  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

/Mikael

Re: Cleaning up expand optabs code

2011-03-25 Thread Richard Henderson 
On 03/25/2011 11:49 AM, Richard Henderson wrote:
> On 03/25/2011 10:51 AM, Richard Sandiford wrote:
>> Thanks.  I think it needs to be s/!= 4/>= 6/ though, so that
>> match_scratches still work when 6 operands really are passed in.
> 
> For the record, I audited all setmem and movmem patterns.
> 
> There are is only one that uses 6 operands: i386.
> There are two that use 4 operands, but have 1 scratch: avr, pdp11.
> All the rest have exactly 4 operands.
> 
> I'll leave the test as == 6 for now.

I added a check to make sure that we don't get a 7 unexpectedly.

Committed as below.


r~
commit b52cb7196addd0e2bc281787a43dd9ce2b9b8cdc
Author: rth 
Date:   Fri Mar 25 23:17:26 2011 +

* expr.c (emit_block_move_via_movmem): Only use 6 operand variant
if there are exactly 6 operands.
(set_storage_via_setmem): Similarly.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@171532 
138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e7983a2..040a83c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2011-03-25  Richard Henderson  
+
+   * expr.c (emit_block_move_via_movmem): Only use 6 operand variant
+   if there are exactly 6 operands.
+   (set_storage_via_setmem): Similarly.
+
 2011-03-25  Kai Tietz  
 
* collect2.c (write_c_file_stat): Handle backslash
diff --git a/gcc/expr.c b/gcc/expr.c
index 4db1c77..076b8d2 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -1294,16 +1294,20 @@ emit_block_move_via_movmem (rtx x, rtx y, rtx size, 
unsigned int align,
 that it doesn't fail the expansion because it thinks
 emitting the libcall would be more efficient.  */
  nops = insn_data[(int) code].n_operands;
+ /* ??? n_operands includes match_scratches; find some other
+way to select the 6 operand variant, or force all targets
+to have exactly 6 operands.  */
+ gcc_assert (nops >= 4 && nops <= 6);
+
  create_fixed_operand (&ops[0], x);
  create_fixed_operand (&ops[1], y);
  /* The check above guarantees that this size conversion is valid.  */
  create_convert_operand_to (&ops[2], size, mode, true);
  create_integer_operand (&ops[3], align / BITS_PER_UNIT);
- if (nops != 4)
+ if (nops == 6)
{
  create_integer_operand (&ops[4], expected_align / BITS_PER_UNIT);
  create_integer_operand (&ops[5], expected_size);
- nops = 6;
}
  if (maybe_expand_insn (code, nops, ops))
{
@@ -2716,16 +2720,20 @@ set_storage_via_setmem (rtx object, rtx size, rtx val, 
unsigned int align,
  unsigned int nops;
 
  nops = insn_data[(int) code].n_operands;
+ /* ??? n_operands includes match_scratches; find some other
+way to select the 6 operand variant, or force all targets
+to have exactly 6 operands.  */
+ gcc_assert (nops >= 4 && nops <= 6);
+
  create_fixed_operand (&ops[0], object);
  /* The check above guarantees that this size conversion is valid.  */
  create_convert_operand_to (&ops[1], size, mode, true);
  create_convert_operand_from (&ops[2], val, byte_mode, true);
  create_integer_operand (&ops[3], align / BITS_PER_UNIT);
- if (nops != 4)
+ if (nops == 6)
{
  create_integer_operand (&ops[4], expected_align / BITS_PER_UNIT);
  create_integer_operand (&ops[5], expected_size);
- nops = 6;
}
  if (maybe_expand_insn (code, nops, ops))
return true;

Re: Cleaning up expand optabs code

2011-03-25 Thread Richard Henderson 
On 03/25/2011 10:51 AM, Richard Sandiford wrote:
> Thanks.  I think it needs to be s/!= 4/>= 6/ though, so that
> match_scratches still work when 6 operands really are passed in.

For the record, I audited all setmem and movmem patterns.

There are is only one that uses 6 operands: i386.
There are two that use 4 operands, but have 1 scratch: avr, pdp11.
All the rest have exactly 4 operands.

I'll leave the test as == 6 for now.

> Fully agreed on the optional args thing.  Or maybe insn_data should
> have a separate "num_args" field.

This is sounds like a good thing.

It's probably worth doing some checking in some genfoo (opinit?) that
the named patterns have exactly the number of operands expected.


r~

Re: Cleaning up expand optabs code

2011-03-25 Thread Richard Sandiford 
Richard Sandiford  writes:
> Er, >= 4 even...

Or not,  *sigh*.  Time I went home...

Richard

Re: Cleaning up expand optabs code

2011-03-25 Thread Richard Sandiford 
Richard Sandiford  writes:
> Richard Henderson  writes:
>> This is due to a miscommunication between the middle-end and the backend
>> about how many arguments the setmemhi pattern takes.
>>
>> (define_expand "setmemhi"
>>   [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>>(match_operand 2 "const_int_operand" ""))
>>   (use (match_operand:HI 1 "const_int_operand" ""))
>>   (use (match_operand:HI 3 "const_int_operand" "n"))
>>   (clobber (match_scratch:HI 4 ""))
>>   (clobber (match_dup 5))])]
>>
>> The match_scratch is counted in .n_operands, which makes the count of
>> operands not equal 4, so we assume 6 operands are necessary.  We can
>> fix this for the special case of avr by only assuming 6 operands when
>> there are in fact 6 operands, but of course this could fail just as
>> easily if there were two scratches.
>>
>> All of which suggests that optional arguments to a named optab is a
>> mistake that ought to be rectified.
>>
>> I plan to commit the following after bootstrap and check.
>
> Thanks.  I think it needs to be s/!= 4/>= 6/ though, so that
> match_scratches still work when 6 operands really are passed in.

Er, >= 4 even...

Richard

Re: Cleaning up expand optabs code

2011-03-25 Thread Richard Sandiford 
Richard Henderson  writes:
> This is due to a miscommunication between the middle-end and the backend
> about how many arguments the setmemhi pattern takes.
>
> (define_expand "setmemhi"
>   [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>(match_operand 2 "const_int_operand" ""))
>   (use (match_operand:HI 1 "const_int_operand" ""))
>   (use (match_operand:HI 3 "const_int_operand" "n"))
>   (clobber (match_scratch:HI 4 ""))
>   (clobber (match_dup 5))])]
>
> The match_scratch is counted in .n_operands, which makes the count of
> operands not equal 4, so we assume 6 operands are necessary.  We can
> fix this for the special case of avr by only assuming 6 operands when
> there are in fact 6 operands, but of course this could fail just as
> easily if there were two scratches.
>
> All of which suggests that optional arguments to a named optab is a
> mistake that ought to be rectified.
>
> I plan to commit the following after bootstrap and check.

Thanks.  I think it needs to be s/!= 4/>= 6/ though, so that
match_scratches still work when 6 operands really are passed in.

Fully agreed on the optional args thing.  Or maybe insn_data should
have a separate "num_args" field.

Richard

Re: Cleaning up expand optabs code

2011-03-25 Thread Richard Henderson 
On 03/25/2011 05:41 AM, Georg-Johann Lay wrote:
>> On 03/22/2011 06:48 PM, Richard Henderson wrote:
>>
>>> Ok.  Watch out for other target problems this week.
> 
> libgcc fails to build for avr (SVN 171446)
> 
> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c: In function
> '__negdi2':
> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c:68:17:
> internal compiler error: in maybe_gen_insn, at  optabs.c:7123

This is due to a miscommunication between the middle-end and the backend
about how many arguments the setmemhi pattern takes.

(define_expand "setmemhi"
  [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
   (match_operand 2 "const_int_operand" ""))
  (use (match_operand:HI 1 "const_int_operand" ""))
  (use (match_operand:HI 3 "const_int_operand" "n"))
  (clobber (match_scratch:HI 4 ""))
  (clobber (match_dup 5))])]

The match_scratch is counted in .n_operands, which makes the count of
operands not equal 4, so we assume 6 operands are necessary.  We can
fix this for the special case of avr by only assuming 6 operands when
there are in fact 6 operands, but of course this could fail just as
easily if there were two scratches.

All of which suggests that optional arguments to a named optab is a
mistake that ought to be rectified.

I plan to commit the following after bootstrap and check.


r~
diff --git a/gcc/expr.c b/gcc/expr.c
index 4db1c77..0a95aa7 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -1299,11 +1299,10 @@ emit_block_move_via_movmem (rtx x, rtx y, rtx size, 
unsigned int align,
  /* The check above guarantees that this size conversion is valid.  */
  create_convert_operand_to (&ops[2], size, mode, true);
  create_integer_operand (&ops[3], align / BITS_PER_UNIT);
- if (nops != 4)
+ if (nops == 6)
{
  create_integer_operand (&ops[4], expected_align / BITS_PER_UNIT);
  create_integer_operand (&ops[5], expected_size);
- nops = 6;
}
  if (maybe_expand_insn (code, nops, ops))
{
@@ -2721,11 +2720,10 @@ set_storage_via_setmem (rtx object, rtx size, rtx val, 
unsigned int align,
  create_convert_operand_to (&ops[1], size, mode, true);
  create_convert_operand_from (&ops[2], val, byte_mode, true);
  create_integer_operand (&ops[3], align / BITS_PER_UNIT);
- if (nops != 4)
+ if (nops == 6)
{
  create_integer_operand (&ops[4], expected_align / BITS_PER_UNIT);
  create_integer_operand (&ops[5], expected_size);
- nops = 6;
}
  if (maybe_expand_insn (code, nops, ops))
return true;

Re: Cleaning up expand optabs code

2011-03-25 Thread Georg-Johann Lay 
Georg-Johann Lay schrieb:
> Andreas Krebbel schrieb:
>> On 03/22/2011 06:48 PM, Richard Henderson wrote:
>>
>>> Ok.  Watch out for other target problems this week.
> 
> libgcc fails to build for avr (SVN 171446)
> 
> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c: In function
> '__negdi2':
> ../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c:68:17:
> internal compiler error: in maybe_gen_insn, at  optabs.c:7123

p.s.: some additional info

(gdb) set args -quiet -v -iprefix
/local/gnu/build/gcc-4.6-avr/gcc/../lib/gcc/avr/4.7.0/ -isystem
/mnt/nfs/home/georg/gnu/build/gcc-4.6-avr/gcc/include -isystem
/mnt/nfs/home/georg/gnu/build/gcc-4.6-avr/gcc/include-fixed
libgcc2-negdi2.c -quiet -dumpbase libgcc2-negdi2.c -auxbase
libgcc2-negdi2 -Os -version -o libgcc2-negdi2.s
(gdb) cd ~/test
(gdb) r
GNU C (GCC) version 4.7.0 20110325 (experimental) (avr)
compiled by GNU C version 4.3.2 [gcc-4_3-branch revision 141291], GMP
version 5.0.1, MPFR version 3.0.0-p8, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096

Breakpoint 1, fancy_abort (file=0x87ee230
"../../../gcc.gnu.org/trunk/gcc/optabs.c", line=7123,
function=0x87ee585 "maybe_gen_insn") at
../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
(gdb) bt
#0  fancy_abort (file=0x87ee230
"../../../gcc.gnu.org/trunk/gcc/optabs.c", line=7123,
function=0x87ee585 "maybe_gen_insn") at
../../../gcc.gnu.org/trunk/gcc/diagnostic.c:893
#1  0x0837a86a in maybe_gen_insn (icode=CODE_FOR_setmemhi, nops=6,
ops=0xbfffdfcc) at ../../../gcc.gnu.org/trunk/gcc/optabs.c:7123
#2  0x0837a91f in maybe_expand_insn (icode=CODE_FOR_setmemhi, nops=6,
ops=0xbfffdfcc) at ../../../gcc.gnu.org/trunk/gcc/optabs.c:7155
#3  0x08241122 in set_storage_via_setmem (object=0xb7dfcc60,
size=0xb7d682f8, val=0xb7d682b8, align=,
expected_align=, expected_size=-1) at
../../../gcc.gnu.org/trunk/gcc/expr.c:2730
#4  0x082567d5 in clear_storage_hints (object=0xb7dfcc60,
size=0xb7d682f8, method=BLOCK_OP_NORMAL, expected_align=0,
expected_size=-1) at ../../../gcc.gnu.org/trunk/gcc/expr.c:2575
#5  0x0825691f in clear_storage (object=0xb7dfcc60, size=0xb7d682f8,
method=BLOCK_OP_NORMAL) at ../../../gcc.gnu.org/trunk/gcc/expr.c:2590
#6  0x0825aa83 in store_constructor (exp=0xb7dfaca8,
target=0xb7dfcc60, cleared=0, size=8) at
../../../gcc.gnu.org/trunk/gcc/expr.c:5188
#7  0x0825b6fe in expand_constructor (exp=0xb7dfaca8,
target=0xb7dfcc60, modifier=EXPAND_NORMAL, avoid_temp_mem=0 '\0') at
../../../gcc.gnu.org/trunk/gcc/expr.c:7092
#8  0x0824dade in expand_expr_real_1 (exp=0xb7dfaca8,
target=0xb7dfcc60, tmode=BLKmode, modifier=EXPAND_NORMAL,
alt_rtl=0xbfffe35c) at ../../../gcc.gnu.org/trunk/gcc/expr.c:8655
#9  0x08245d52 in store_expr (exp=0xb7dfaca8, target=0xb7dfcc60,
call_param_p=0, nontemporal=0 '\0') at
../../../gcc.gnu.org/trunk/gcc/expr.c:4645
#10 0x08257949 in expand_assignment (to=0xb7e2a1e0, from=0xb7dfaca8,
nontemporal=5 '\005') at ../../../gcc.gnu.org/trunk/gcc/expr.c:4433
#11 0x081a61aa in expand_gimple_stmt (stmt=0xb7e237e0) at
../../../gcc.gnu.org/trunk/gcc/cfgexpand.c:1971
#12 0x081a7200 in expand_gimple_basic_block (bb=0xb7ddf3c0) at
../../../gcc.gnu.org/trunk/gcc/cfgexpand.c:3645
#13 0x081a8a65 in gimple_expand_cfg () at
../../../gcc.gnu.org/trunk/gcc/cfgexpand.c:4128
#14 0x08395334 in execute_one_pass (pass=0x88742e0) at
../../../gcc.gnu.org/trunk/gcc/passes.c:1554
#15 0x0839562d in execute_pass_list (pass=0x88742e0) at
../../../gcc.gnu.org/trunk/gcc/passes.c:1609
#16 0x08475baa in tree_rest_of_compilation (fndecl=0xb7e21180) at
../../../gcc.gnu.org/trunk/gcc/tree-optimize.c:422
#17 0x08615bc6 in cgraph_expand_function (node=0xb7e2c000) at
../../../gcc.gnu.org/trunk/gcc/cgraphunit.c:1576
#18 0x08618c59 in cgraph_optimize () at
../../../gcc.gnu.org/trunk/gcc/cgraphunit.c:1635
#19 0x0861915d in cgraph_finalize_compilation_unit () at
../../../gcc.gnu.org/trunk/gcc/cgraphunit.c:1096
#20 0x080b6590 in c_write_global_declarations () at
../../../gcc.gnu.org/trunk/gcc/c-decl.c:9879
#21 0x08411fae in toplev_main (argc=19, argv=0xbfffe874) at
../../../gcc.gnu.org/trunk/gcc/toplev.c:591
#22 0x08152372 in main (argc=Cannot access memory at address 0x1
) at ../../../gcc.gnu.org/trunk/gcc/main.c:36
(gdb) frame 1
#1  0x0837a86a in maybe_gen_insn (icode=CODE_FOR_setmemhi, nops=6,
ops=0xbfffdfcc) at ../../../gcc.gnu.org/trunk/gcc/optabs.c:7123
(gdb) p nops
$1 = 6
(gdb) p insn_data[(int) icode].n_operands
$2 = 5 '\005'
(gdb) p *ops
$3 = {type = EXPAND_FIXED, unsigned_p = 0, unused = 0, mode =
VOIDmode, value = 0xb7dfcc60}
(gdb) p ops -> value
$4 = (rtx) 0xb7dfcc60
(gdb) pr
(mem/s/c:BLK (reg/f:HI 37 virtual-stack-vars) [2 w+0 S8 A8])
(gdb) p icode
$5 = CODE_FOR_setmemhi
(gdb) Quit
(gdb)

Re: Cleaning up expand optabs code

2011-03-25 Thread Georg-Johann Lay 
Andreas Krebbel schrieb:
> On 03/22/2011 06:48 PM, Richard Henderson wrote:
> 
>> Ok.  Watch out for other target problems this week.

libgcc fails to build for avr (SVN 171446)

../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c: In function
'__negdi2':
../../../../../gcc.gnu.org/trunk/libgcc/../gcc/libgcc2.c:68:17:
internal compiler error: in maybe_gen_insn, at  optabs.c:7123

Target: avr
Configured with: ../../gcc.gnu.org/trunk/configure --target=avr
--prefix=/local/gnu/install/gcc-4.6 --enable-languages=c,c++
--disable-libssp --disable-libada --disable-nls --disable-shared

Attached the source where it crashes; compile with -S -Os

Building last 4.7 snapshot + avr backend from HEAD works fine.

Johann






















typedef int ptrdiff_t;

typedef unsigned int size_t;

typedef int wchar_t;







extern void *malloc (size_t);



extern void free (void *);



extern int atexit (void (*)(void));



extern void abort (void) __attribute__ ((__noreturn__));



extern size_t strlen (const char *);



extern void *memcpy (void *, const void *, size_t);



extern void *memset (void *, int, size_t);



extern int filename_cmp (const char *s1, const char *s2);


extern int filename_ncmp (const char *s1, const char *s2,
 size_t n);




struct _dont_use_rtx_here_;
struct _dont_use_rtvec_here_;
union _dont_use_tree_here_;











enum debug_info_type
{
  NO_DEBUG,
  DBX_DEBUG,
  SDB_DEBUG,
  DWARF2_DEBUG,
  XCOFF_DEBUG,
  VMS_DEBUG,
  VMS_AND_DWARF2_DEBUG

};

enum debug_info_levels
{
  DINFO_LEVEL_NONE,
  DINFO_LEVEL_TERSE,
  DINFO_LEVEL_NORMAL,
  DINFO_LEVEL_VERBOSE
};

enum debug_info_usage
{
  DINFO_USAGE_DFN,
  DINFO_USAGE_DIR_USE,
  DINFO_USAGE_IND_USE,
  DINFO_USAGE_NUM_ENUMS
};

enum debug_struct_file
{
  DINFO_STRUCT_FILE_NONE,
  DINFO_STRUCT_FILE_BASE,

  DINFO_STRUCT_FILE_SYS,

  DINFO_STRUCT_FILE_ANY
};





enum symbol_visibility
{
  VISIBILITY_DEFAULT,
  VISIBILITY_PROTECTED,
  VISIBILITY_HIDDEN,
  VISIBILITY_INTERNAL
};



enum ira_algorithm
{
  IRA_ALGORITHM_CB,
  IRA_ALGORITHM_PRIORITY
};


enum ira_region
{
  IRA_REGION_ONE,
  IRA_REGION_ALL,
  IRA_REGION_MIXED
};


enum excess_precision
{
  EXCESS_PRECISION_DEFAULT,
  EXCESS_PRECISION_FAST,
  EXCESS_PRECISION_STANDARD
};


enum graph_dump_types
{
  no_graph = 0,
  vcg
};


enum stack_check_type
{

  NO_STACK_CHECK = 0,



  GENERIC_STACK_CHECK,




  STATIC_BUILTIN_STACK_CHECK,



  FULL_BUILTIN_STACK_CHECK
};




enum warn_strict_overflow_code
{



  WARN_STRICT_OVERFLOW_ALL = 1,




  WARN_STRICT_OVERFLOW_CONDITIONAL = 2,


  WARN_STRICT_OVERFLOW_COMPARISON = 3,


  WARN_STRICT_OVERFLOW_MISC = 4,


  WARN_STRICT_OVERFLOW_MAGNITUDE = 5
};


enum fp_contract_mode {
  FP_CONTRACT_OFF = 0,
  FP_CONTRACT_ON = 1,
  FP_CONTRACT_FAST = 2
};


enum vect_verbosity_levels {
  REPORT_NONE,
  REPORT_VECTORIZED_LOCATIONS,
  REPORT_UNVECTORIZED_LOCATIONS,
  REPORT_COST,
  REPORT_ALIGNMENT,
  REPORT_DR_DETAILS,
  REPORT_BAD_FORM_LOOPS,
  REPORT_OUTER_LOOPS,
  REPORT_SLP,
  REPORT_DETAILS,

  MAX_VERBOSITY_LEVEL
};


enum opt_code
{
  OPT = 0,

  OPT__help = 27,
  OPT__help_ = 28,

  OPT__output_pch_ = 58,

  OPT__param = 60,

  OPT__sysroot_ = 90,
  OPT__target_help = 91,

  OPT__version = 101,


  OPT_A = 104,
  OPT_B = 105,
  OPT_C = 106,
  OPT_CC = 107,
  OPT_D = 108,
  OPT_E = 109,
  OPT_F = 110,
  OPT_H = 111,
  OPT_I = 112,
  OPT_J = 113,
  OPT_L = 114,
  OPT_M = 115,
  OPT_MD = 116,
  OPT_MF = 117,
  OPT_MG = 118,
  OPT_MM = 119,
  OPT_MMD = 120,
  OPT_MP = 121,
  OPT_MQ = 122,
  OPT_MT = 123,
  OPT_N = 124,
  OPT_O = 125,
  OPT_Ofast = 126,
  OPT_Os = 127,
  OPT_P = 128,
  OPT_Q = 129,
  OPT_Qn = 130,
  OPT_Qy = 131,
  OPT_R = 132,
  OPT_S = 133,
  OPT_T = 134,
  OPT_Tbss = 135,
  OPT_Tdata = 136,
  OPT_Ttext = 137,
  OPT_U = 138,

  OPT_Wa_ = 140,
  OPT_Wabi = 141,
  OPT_Waddress = 142,
  OPT_Waggregate_return = 143,
  OPT_Waliasing = 144,
  OPT_Walign_commons = 145,
  OPT_Wall = 146,
  OPT_Wampersand = 147,
  OPT_Warray_bounds = 148,
  OPT_Warray_temporaries = 149,
  OPT_Wassign_intercept = 150,
  OPT_Wattributes = 151,
  OPT_Wbad_function_cast = 152,
  OPT_Wbuiltin_macro_redefined = 153,
  OPT_Wc___compat = 154,
  OPT_Wc__0x_compat = 155,
  OPT_Wcast_align = 156,
  OPT_Wcast_qual = 157,
  OPT_Wchar_subscripts = 158,
  OPT_Wcharacter_truncation = 159,
  OPT_Wclobbered = 160,
  OPT_Wcomment = 161,

  OPT_Wconversion = 163,
  OPT_Wconversion_extra = 164,
  OPT_Wconversion_null = 165,
  OPT_Wcoverage_mismatch = 166,
  OPT_Wcpp = 167,
  OPT_Wctor_dtor_privacy = 168,
  OPT_Wdeclaration_after_statement = 169,
  OPT_Wdeprecated = 170,
  OPT_Wdeprecated_declarations = 171,
  OPT_Wdisabled_optimization = 172,
  OPT_Wdiv_by_zero = 173,
  OPT_Wdouble_promotion = 174,
  OPT_Weffc__ = 175,
  OPT_Wempty_body = 176,
  OPT_Wendif_labels = 177,
  OPT_Wenum_compare = 178,
  OPT_Werror = 179,

  OPT_Werror_ = 181,
  OPT_Wextra = 182,
  OPT_Wfatal_errors = 183,
  OPT_Wfloat_equal = 184,
  OPT_Wformat = 185,
  OPT_Wformat_contains_nul = 186,
  OPT

Re: Cleaning up expand optabs code

2011-03-24 Thread Richard Henderson 
On 03/24/2011 05:13 AM, Richard Sandiford wrote:
> gcc/
>   PR rtl-optimization/48263
>   * optabs.c (expand_binop_directly): Reinstate convert_modes code
>   and original commutative_p handling.  Use maybe_gen_insn.

Ok.


r~

Re: Cleaning up expand optabs code

2011-03-24 Thread Richard Sandiford 
This showed up as a warning during the h8300 build I did this morning.
I've no idea why it wasn't caught during the x86_64, ARM and MIPS testing.

Bootstrapped & regression-tested on x86_64-linux-gnu.  Installed as obvious.

Richard


gcc/
* builtins.c (expand_movstr): Fix endp == 1 adjustment after
last commit.

Index: gcc/builtins.c
===
--- gcc/builtins.c  2011-03-23 09:30:17.0 +
+++ gcc/builtins.c  2011-03-24 09:08:05.0 +
@@ -3655,7 +3655,6 @@ expand_builtin_mempcpy_args (tree dest, 
 expand_movstr (tree dest, tree src, rtx target, int endp)
 {
   struct expand_operand ops[3];
-  rtx end;
   rtx dest_mem;
   rtx src_mem;
 
@@ -3683,7 +3682,7 @@ expand_movstr (tree dest, tree src, rtx 
 adjust it.  */
   if (endp == 1)
{
- rtx tem = plus_constant (gen_lowpart (GET_MODE (target), end), 1);
+ rtx tem = plus_constant (gen_lowpart (GET_MODE (target), target), 1);
  emit_move_insn (target, force_operand (tem, NULL_RTX));
}
 }

Re: Cleaning up expand optabs code

2011-03-24 Thread Richard Sandiford 
Richard Sandiford  writes:
> Andreas Krebbel  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.

Bootstrap & regression-test on x86_64-linux-gnu now finished.  OK to install?

>> 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.
>
> Index: gcc/optabs.c
> ===
> --- gcc/optabs.c  2011-03-24 09:18:00.0 +
> +++ gcc/optabs.c  2011-03-24 09:36:46.0 +
> @@ -1269,6 +1269,38 @@ expand_binop_directly (enum machine_mode
>if (!shift_optab_p (binoptab))
>  xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
>  
> +  /* In case the insn wants input operands in modes different from
> + those of the actual operands, convert the operands.  It would
> + seem that we don't need to convert CONST_INTs, but we do, so
> + 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);
> +
> +  /* If operation is commutative,
> + try to make the first operand a register.
> + Even better, try to make it the same as the target.
> + Also try to make the last operand a constant.  */
> +  if (commutative_p
> +  && swap_commutative_operands_with_target (target, xop0, xop1))
> +{
> +  swap = xop1;
> +  xop1 = xop0;
> +  xop0 = swap;
> +}
> +
>/* Now, if insn's predicates don't allow our operands, put them into
>   pseudo regs.  */
>  
> @@ -1291,41 +1323,25 @@ expand_binop_directly (enum machine_mode
>  tmp_mode = mode;
>  
>create_output_operand (&ops[0], target, tmp_mode);
> -  create_convert_operand_from (&ops[1], xop0, mode, unsignedp);
> -  create_convert_operand_from (&ops[2], xop1, mode, unsignedp);
> -  if (maybe_legitimize_operands (icode, 0, 3, ops))
> -{
> -  /* If operation is commutative,
> -  try to make the first operand a register.
> -  Even better, try to make it the same as the target.
> -  Also try to make the last operand a constant.  */
> -  if (commutative_p
> -   && swap_commutative_operands_with_target (ops[0].value, ops[1].value,
> - ops[2].value))
> - {
> -   swap = ops[2].value;
> -   ops[2].value = ops[1].value;
> -   ops[1].value = swap;
> - }
> -
> -  pat = GEN_FCN (icode) (ops[0].value, ops[1].value, ops[2].value);
> -  if (pat)
> +  create_input_operand (&ops[1], xop0, mode0);
> +  create_input_operand (&ops[2], xop1, mode1);
> +  pat = maybe_gen_insn (icode, 3, ops);
> +  if (pat)
> +{
> +  /* If PAT is composed of more than one insn, try to add an appropriate
> +  REG_EQUAL note to it.  If we can't because TEMP conflicts with an
> +  operand, call expand_binop again, this time without a target.  */
> +  if (INSN_P (pat) && NEXT_INSN (pat) != NULL_RTX
> +   && ! add_equal_note (pat, ops[0].value, binoptab->code,
> +ops[1].value, ops[2].value))
>   {
> -   /* If PAT is composed of more than one insn, try to add an appropriate
> -  REG_EQUAL note to it.  If we can't because TEMP conflicts with an
> -  operand, call expand_binop again, this time without a target.  */
> -   if (INSN_P (pat) && NEXT_INSN (pat) != NULL_RTX
> -   && ! add_equal_note (pat, ops[0].value, binoptab->code,
> -ops[1].value, ops[2].value))
> - {
> -   delete_insns_since (last);
> -   return expand_binop (mode, binoptab, op0, op1, NULL_RTX,
> -unsignedp, methods);
> - }
> -
> -   emit_insn (pat);
> -   return ops[0].value;
> +   delete_insns_since (last);
> +   return expand_binop (mode, binoptab, op0, op1, NULL_RTX,
> +unsignedp, meth

Re: Cleaning up expand optabs code

2011-03-24 Thread Richard Sandiford 
Andreas Krebbel  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.

Index: gcc/optabs.c
===
--- gcc/optabs.c2011-03-24 09:18:00.0 +
+++ gcc/optabs.c2011-03-24 09:36:46.0 +
@@ -1269,6 +1269,38 @@ expand_binop_directly (enum machine_mode
   if (!shift_optab_p (binoptab))
 xop1 = avoid_expensive_constant (mode1, binoptab, xop1, unsignedp);
 
+  /* In case the insn wants input operands in modes different from
+ those of the actual operands, convert the operands.  It would
+ seem that we don't need to convert CONST_INTs, but we do, so
+ 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);
+
+  /* If operation is commutative,
+ try to make the first operand a register.
+ Even better, try to make it the same as the target.
+ Also try to make the last operand a constant.  */
+  if (commutative_p
+  && swap_commutative_operands_with_target (target, xop0, xop1))
+{
+  swap = xop1;
+  xop1 = xop0;
+  xop0 = swap;
+}
+
   /* Now, if insn's predicates don't allow our operands, put them into
  pseudo regs.  */
 
@@ -1291,41 +1323,25 @@ expand_binop_directly (enum machine_mode
 tmp_mode = mode;
 
   create_output_operand (&ops[0], target, tmp_mode);
-  create_convert_operand_from (&ops[1], xop0, mode, unsignedp);
-  create_convert_operand_from (&ops[2], xop1, mode, unsignedp);
-  if (maybe_legitimize_operands (icode, 0, 3, ops))
-{
-  /* If operation is commutative,
-try to make the first operand a register.
-Even better, try to make it the same as the target.
-Also try to make the last operand a constant.  */
-  if (commutative_p
- && swap_commutative_operands_with_target (ops[0].value, ops[1].value,
-   ops[2].value))
-   {
- swap = ops[2].value;
- ops[2].value = ops[1].value;
- ops[1].value = swap;
-   }
-
-  pat = GEN_FCN (icode) (ops[0].value, ops[1].value, ops[2].value);
-  if (pat)
+  create_input_operand (&ops[1], xop0, mode0);
+  create_input_operand (&ops[2], xop1, mode1);
+  pat = maybe_gen_insn (icode, 3, ops);
+  if (pat)
+{
+  /* If PAT is composed of more than one insn, try to add an appropriate
+REG_EQUAL note to it.  If we can't because TEMP conflicts with an
+operand, call expand_binop again, this time without a target.  */
+  if (INSN_P (pat) && NEXT_INSN (pat) != NULL_RTX
+ && ! add_equal_note (pat, ops[0].value, binoptab->code,
+  ops[1].value, ops[2].value))
{
- /* If PAT is composed of more than one insn, try to add an appropriate
-REG_EQUAL note to it.  If we can't because TEMP conflicts with an
-operand, call expand_binop again, this time without a target.  */
- if (INSN_P (pat) && NEXT_INSN (pat) != NULL_RTX
- && ! add_equal_note (pat, ops[0].value, binoptab->code,
-  ops[1].value, ops[2].value))
-   {
- delete_insns_since (last);
- return expand_binop (mode, binoptab, op0, op1, NULL_RTX,
-  unsignedp, methods);
-   }
-
- emit_insn (pat);
- return ops[0].value;
+ delete_insns_since (last);
+ return expand_binop (mode, binoptab, op0, op1, NULL_RTX,
+  unsignedp, methods);
}
+
+  emit_insn (pat);
+  return ops[0].value;
 }
   delete_insns_since (last);
   return NULL_RTX;

Re: Cleaning up expand optabs code

2011-03-24 Thread Andreas Krebbel 
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.

An unrecognizable insns is generated:

(insn 22 21 23 4 (set (reg/v:DI 44 [ end])
   (mult:DI (sign_extend:DI (mem/s/j:SI (plus:SI (reg/v/f:SI 47 [ foo])
(const_int 4 [0x4])) [0 foo_5(D)->incr_ull+4 S4 A32]))
(sign_extend:DI (subreg:SI (reg:DI 49) 4 t.c:12 -1
 (nil))

The problem is that expand_binop_directly swaps the operands without rechecking 
the
predicates afterwards.  The old code did:

  /* Now, if insn's predicates don't allow our operands, put them into
 pseudo regs.  */

  if (!insn_data[icode].operand[1].predicate (xop0, mode0)
  && mode0 != VOIDmode)
xop0 = copy_to_mode_reg (mode0, xop0);

  if (!insn_data[icode].operand[2].predicate (xop1, mode1)
  && mode1 != VOIDmode)
xop1 = copy_to_mode_reg (mode1, xop1);

Right after swapping the operands.

Unfortunately it cannot be simply fixed by swapping the operands in the back 
end pattern.
 Since subreg and reg have different operand precedences. A subreg usually will 
be second
after a mem while a reg (having same precedence as mem) might be first operand 
before a mem.

Just copying the pre-patch behaviour fixes the problem for me:

Index: gcc/optabs.c
===
*** gcc/optabs.c.orig   2011-03-24 12:54:31.0 +0100
--- gcc/optabs.c2011-03-24 12:54:43.0 +0100
*** expand_binop_directly (enum machine_mode
*** 1308,1313 
--- 1308,1322 
  ops[1].value = swap;
}

+   /* Now, if insn's predicates don't allow our operands, put them into
+pseudo regs.  */
+
+   if (!insn_operand_matches (icode, 1, ops[1].value))
+   ops[1].value = copy_to_mode_reg (mode0, ops[1].value);
+
+   if (!insn_operand_matches (icode, 2, ops[2].value))
+   ops[2].value = copy_to_mode_reg (mode1, ops[2].value);
+
pat = GEN_FCN (icode) (ops[0].value, ops[1].value, ops[2].value);
if (pat)
{

Ok?

Bye,

-Andreas-

Re: Cleaning up expand optabs code

2011-03-24 Thread Richard Sandiford 
"Anatoly Sokolov"  writes:
> This patch casue ICE on H8300 target:

This is due to jump_address_operand checking the wrong mode.
The predicate is:

(define_predicate "jump_address_operand"
  (match_code "reg,mem")
{
  if (GET_CODE (op) == REG)
return mode == Pmode;
  [...]
}

but "mode" is the mode passed to the predicate, not the mode of OP.
The indirect_jump pattern is:

(define_expand "indirect_jump"
  [(set (pc) (match_operand 0 "jump_address_operand" ""))]
  ""
  "")

which says that VOIDmode should be passed to jump_address_operand,
so all registers end up being rejected.

I've applied the following as the obvious fix.  You then hit bug
48263 ( http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48263 );
I'm testing a fix for that now.

Richard


gcc/
* config/h8300/predicates.md (jump_address_operand): Fix register
mode check.

Index: gcc/config/h8300/predicates.md
===
--- gcc/config/h8300/predicates.md  2011-01-05 15:12:08.0 +
+++ gcc/config/h8300/predicates.md  2011-03-24 09:20:15.0 +
@@ -259,7 +259,7 @@ (define_predicate "jump_address_operand"
   (match_code "reg,mem")
 {
   if (GET_CODE (op) == REG)
-return mode == Pmode;
+return GET_MODE (op) == Pmode;
 
   if (GET_CODE (op) == MEM)
 {

Re: Cleaning up expand optabs code

2011-03-23 Thread Anatoly Sokolov 

Hi.


From: "Richard Henderson" 
Sent: Tuesday, March 22, 2011 8:48 PM


Ok.  Watch out for other target problems this week.





This patch casue ICE on H8300 target:

make[4]: Entering directory 
`/home/aesok/h83001/build/h8300-elf/h8300h/libgcc'

# If this is the top-level multilib, build all the other
# multilibs.
/home/aesok/h83001/build/./gcc/xgcc -B/home/aesok/h83001/build/./gcc/ -nostdinc
-B/home/aesok/h83001/build/h8300-elf/newlib/ -isystem 
/home/aesok/h83001/build/h
8300-elf/newlib/targ-include -isystem 
/home/aesok/h83001/combined/newlib/libc/in

clude -B/home/aesok/cross-local/h8300-elf/h8300-elf/bin/ -B/home/aesok/cross-loc
al/h8300-elf/h8300-elf/lib/ -isystem 
/home/aesok/cross-local/h8300-elf/h8300-elf
/include -isystem 
/home/aesok/cross-local/h8300-elf/h8300-elf/sys-include -L/hom

e/aesok/h83001/build/./ld-g -O2 -mh -O2  -g -O2 -DIN_GCC -DCROSS_DIRECTORY_S
TRUCTURE  -W -Wall -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-pro
totypes -Wold-style-definition  -isystem 
./include  -DDF=SF -g  -DIN_LIBGCC2 -D_

_GCC_FLOAT_NOT_NEEDED -fno-stack-protector -Dinhibit_libc  -I. -I. -I../../.././
gcc -I../../../../combined/libgcc -I../../../../combined/libgcc/. -I../../../../
combined/libgcc/../gcc -I../../../../combined/libgcc/../include  -DHAVE_CC_TLS 
-
DUSE_EMUTLS -o unwind-sjlj.o -MT unwind-sjlj.o -MD -MP -MF 
unwind-sjlj.dep -fexc

eptions -c ../../../../combined/libgcc/../gcc/unwind-sjlj.c
../../../../combined/libgcc/../gcc/unwind-sjlj.c: In function 
'uw_install_contex

t.isra___1':
../../../../combined/libgcc/../gcc/unwind-sjlj.c:306:11: internal compiler 
error

: in expand_jump_insn, at optabs.c:7181
Please submit a full bug report,
with preprocessed source if appropriate.
See  for instructions.
make[4]: *** [unwind-sjlj.o] Error 1
make[4]: Leaving directory 
`/home/aesok/h83001/build/h8300-elf/h8300h/libgcc'

make[3]: *** [multi-do] Error 1
make[3]: Leaving directory `/home/aesok/h83001/build/h8300-elf/libgcc'
make[2]: *** [all-multi] Error 2
make[2]: Leaving directory `/home/aesok/h83001/build/h8300-elf/libgcc'
make[1]: *** [all-target-libgcc] Error 2
make[1]: Leaving directory `/home/aesok/h83001/build'

Anatoly.

Re: Cleaning up expand optabs code

2011-03-22 Thread Richard Henderson 
On 03/22/2011 08:08 AM, Richard Sandiford wrote:
>>> +  for (i = 0; i + 1 < nops; i++)
>>> +if (ops[i].commutative < MAX_EXPAND_OPERANDS
>>> +   && swap_commutative_operands_with_target 
>>> (ops[ops[i].commutative].value,
>>> + ops[i].value,
>>> + ops[i + 1].value))
>>
>> with the assumption of i & i+1 being related, to be a pretty strong
>> assumption.
> 
> OK.  I saw this as the expand equivalent of the "+" constraint, which
> makes the same assumption.  I don't think we really support gaps between
> commutative operands in any meaningful way.

You mean "%" I assume.  Yes, it does concern operands N and N+1.  But it's
documented that way, whereas you have .commutative and documented that as
the the operand number that gets swapped.  Which begs the question of what
is that third operand above, and how it would get specified.

Not that it matters now...

>   * optabs.h (emit_unop_insn, maybe_emit_unop_insn): Change insn code
>   parameter from "int" to "enum insn_code".
>   (expand_operand_type): New enum.
>   (expand_operand): New structure.
>   (create_expand_operand): New function.
>   (create_fixed_operand, create_output_operand): Likewise
>   (create_input_operand, create_convert_operand_to): Likewise.
>   (create_convert_operand_from, create_address_operand): Likewise.
>   (create_integer_operand): Likewise.
>   (create_convert_operand_from_type, maybe_legitimize_operands): Declare.
>   (maybe_gen_insn, maybe_expand_insn, maybe_expand_jump_insn): Likewise.
>   (expand_insn, expand_jump_insn): Likewise.
>   * builtins.c (expand_builtin_prefetch): Use the new interfaces.
>   (expand_builtin_interclass_mathfn, expand_builtin_strlen): Likewise.
>   (expand_movstr, expand_builtin___clear_cache): Likewise.
>   (expand_builtin_lock_release): Likewise.
>   * explow.c (allocate_dynamic_stack_space): Likewise.
>   (probe_stack_range): Likewise.  Allow check_stack to FAIL,
>   and use the default handling in that case.
>   * expmed.c (check_predicate_volatile_ok): Delete.
>   (store_bit_field_1, extract_bit_field_1): Use the new interfaces.
>   (emit_cstore): Likewise.
>   * expr.c (emit_block_move_via_movmem): Likewise.
>   (set_storage_via_setmem, expand_assignment): Likewise.
>   (emit_storent_insn, try_casesi): Likewise.
>   (emit_single_push_insn): Likewise.  Allow the expansion to fail.
>   * optabs.c (expand_widen_pattern_expr, expand_ternary_op): Likewise.
>   (expand_vec_shift_expr, expand_binop_directly): Likewise.
>   (expand_twoval_unop, expand_twoval_binop): Likewise.
>   (expand_unop_direct, emit_indirect_jump): Likewise.
>   (emit_conditional_move, vector_compare_rtx): Likewise.
>   (expand_vec_cond_expr, expand_val_compare_and_swap_1): Likewise.
>   (expand_sync_operation, expand_sync_fetch_operation): Likewise.
>   (expand_sync_lock_test_and_set): Likewise.
>   (maybe_emit_unop_insn): Likewise.  Change icode to an insn_code.
>   (emit_unop_insn): Likewise.
>   (expand_copysign_absneg): Change icode to an insn_code.
>   (create_convert_operand_from_type): New function.
>   (maybe_legitimize_operand, maybe_legitimize_operands): Likewise.
>   (maybe_gen_insn, maybe_expand_insn, maybe_expand_jump_insn): Likewise.
>   (expand_insn, expand_jump_insn): Likewise.
>   * config/i386/i386.md (setmem): Use nonmemory_operand rather
>   than const_int_operand for operand 2.

Ok.  Watch out for other target problems this week.


r~

Re: Cleaning up expand optabs code

2011-03-21 Thread Richard Sandiford 
Richard Henderson  writes:
>>  * reload.c (find_reloads_address_1): Use insn_operand_matches.
>>  * reload1.c (gen_reload): Likewise.
>
> All the bits that just use insn_operand_matches are approved.
> You can commit those first if you like to reduce the patch size.

OK, thanks, here's what I applied after testing on x86_64-linux-gnu.

Richard


gcc/
* expr.h (prepare_operand): Move to...
* optabs.h (prepare_operand): ...here and change the insn code
parameter from "int" to "enum insn_code".
(insn_operand_matches): Declare.
* expr.c (init_expr_target): Use insn_operand_matches.
(compress_float_constant): Likewise.
* function.c (safe_insn_predicate, assign_parm_setup_reg): Likewise.
* optabs.c (can_compare_p, prepare_cmp_insn): Likewise.
(emit_cmp_and_jump_insn_1, gen_add2_insn, gen_add3_insn): Likewise.
(have_add2_insn, gen_sub2_insn, gen_sub3_insn, have_sub2_insn): 
Likewise.
(gen_cond_trap): Likewise.
(prepare_operand): Likewise.  Change icode to an insn_code.
(insn_operand_matches): New function.
* reload.c (find_reloads_address_1): Use insn_operand_matches.
* reload1.c (gen_reload): Likewise.
* targhooks.c (default_secondary_reload): Likewise.

Index: gcc/expr.h
===
--- gcc/expr.h  2011-03-21 21:35:04.0 +
+++ gcc/expr.h  2011-03-21 21:36:40.0 +
@@ -173,9 +173,6 @@ extern rtx expand_simple_unop (enum mach
perform the operation described by CODE and MODE.  */
 extern int have_insn_for (enum rtx_code, enum machine_mode);
 
-extern rtx prepare_operand (int, rtx, int, enum machine_mode, enum 
machine_mode,
-   int);
-
 /* Emit code to make a call to a constant function or a library call.  */
 extern void emit_libcall_block (rtx, rtx, rtx, rtx);
 
Index: gcc/optabs.h
===
--- gcc/optabs.h2011-03-21 21:35:04.0 +
+++ gcc/optabs.h2011-03-21 21:36:40.0 +
@@ -923,4 +923,10 @@ set_direct_optab_handler (direct_optab o
 extern rtx optab_libfunc (optab optab, enum machine_mode mode);
 extern rtx convert_optab_libfunc (convert_optab optab, enum machine_mode mode1,
  enum machine_mode mode2);
+
+extern bool insn_operand_matches (enum insn_code icode, unsigned int opno,
+ rtx operand);
+extern rtx prepare_operand (enum insn_code, rtx, int, enum machine_mode,
+   enum machine_mode, int);
+
 #endif /* GCC_OPTABS_H */
Index: gcc/expr.c
===
--- gcc/expr.c  2011-03-21 21:35:04.0 +
+++ gcc/expr.c  2011-03-21 21:36:40.0 +
@@ -286,7 +286,7 @@ init_expr_target (void)
 
  PUT_MODE (mem, srcmode);
 
- if ((*insn_data[ic].operand[1].predicate) (mem, srcmode))
+ if (insn_operand_matches (ic, 1, mem))
float_extend_from_mem[mode][srcmode] = true;
}
 }
@@ -3446,7 +3446,7 @@ compress_float_constant (rtx x, rtx y)
{
  /* Skip if the target needs extra instructions to perform
 the extension.  */
- if (! (*insn_data[ic].operand[1].predicate) (trunc_y, srcmode))
+ if (!insn_operand_matches (ic, 1, trunc_y))
continue;
  /* This is valid, but may not be cheaper than the original. */
  newcost = rtx_cost (gen_rtx_FLOAT_EXTEND (dstmode, trunc_y), SET, 
speed);
Index: gcc/function.c
===
--- gcc/function.c  2011-03-21 21:35:04.0 +
+++ gcc/function.c  2011-03-21 21:36:40.0 +
@@ -1493,16 +1493,7 @@ instantiate_virtual_regs_in_rtx (rtx *lo
 static int
 safe_insn_predicate (int code, int operand, rtx x)
 {
-  const struct insn_operand_data *op_data;
-
-  if (code < 0)
-return true;
-
-  op_data = &insn_data[code].operand[operand];
-  if (op_data->predicate == NULL)
-return true;
-
-  return op_data->predicate (x, op_data->mode);
+  return code < 0 || insn_operand_matches ((enum insn_code) code, operand, x);
 }
 
 /* A subroutine of instantiate_virtual_regs.  Instantiate any virtual
@@ -3013,8 +3004,8 @@ assign_parm_setup_reg (struct assign_par
   op0 = parmreg;
   op1 = validated_mem;
   if (icode != CODE_FOR_nothing
- && insn_data[icode].operand[0].predicate (op0, promoted_nominal_mode)
- && insn_data[icode].operand[1].predicate (op1, data->passed_mode))
+ && insn_operand_matches (icode, 0, op0)
+ && insn_operand_matches (icode, 1, op1))
{
  enum rtx_code code = unsignedp ? ZERO_EXTEND : SIGN_EXTEND;
  rtx insn, insns;
Index: gcc/optabs.c
===
--- gcc/optabs.c2011-03-21 21

Re: Cleaning up expand optabs code

2011-03-21 Thread Richard Henderson 
On 03/19/2011 12:52 PM, Richard Sandiford wrote:
> Given the mode stuff above, I've tried to be quite draconian as far
> as caller-provided modes go.  I think the caller really should know
> what mode they're dealing with.  The one case where I had to hold
> back a bit was create_convert_operand_from, which replaces things like:
> 
>   if (GET_MODE (op0) != mode0 && mode0 != VOIDmode)
> xop0 = convert_modes (mode0,
>   GET_MODE (op0) != VOIDmode
>   ? GET_MODE (op0)
>   : mode,
>   xop0, unsignedp);
> 
> It seems a little suspicious that we only trust "mode" for CONST_INT
> op0s, and not for other cases, but I'd like to leave that for now.
> Maybe a future "clean up"?

Sure.

> Also, I had to change the i386 setmem pattern in order to avoid
> a regression in cold-attribute-1.c.  The current predicate for
> the character operand is "const_int_operand", but the pattern
> handles registers as well.  I'm sure there are going to be other
> things like that, so sorry in advance if this patch goes in and
> breaks a target...

Sure.

>   * reload.c (find_reloads_address_1): Use insn_operand_matches.
>   * reload1.c (gen_reload): Likewise.

All the bits that just use insn_operand_matches are approved.
You can commit those first if you like to reduce the patch size.

>  {
> -  if ((! (*insn_data[(int) CODE_FOR_prefetch].operand[0].predicate)
> -  (op0,
> -   insn_data[(int) CODE_FOR_prefetch].operand[0].mode))
> -   || (GET_MODE (op0) != Pmode))
> - {
> -   op0 = convert_memory_address (Pmode, op0);
> -   op0 = force_reg (Pmode, op0);
> - }
> -  emit_insn (gen_prefetch (op0, op1, op2));
> +  struct expand_operand ops[3];
> +
> +  create_address_operand (&ops[0], op0);
> +  create_integer_operand (&ops[1], INTVAL (op1));
> +  create_integer_operand (&ops[2], INTVAL (op2));
> +  if (maybe_expand_insn (CODE_FOR_prefetch, 3, ops))
> + return;
>  }

Yep, this interface is a definite cleanup.

> @@ -2452,10 +2443,11 @@ expand_builtin_interclass_mathfn (tree e
>if (mode != GET_MODE (op0))
>   op0 = convert_to_mode (mode, op0, 0);
>  
> -  /* Compute into TARGET.
> -  Set TARGET to wherever the result comes back.  */
> -  if (maybe_emit_unop_insn (icode, target, op0, UNKNOWN))
> - return target;
> +  create_output_operand (&ops[0], target, TYPE_MODE (TREE_TYPE (exp)));
> +  if (maybe_legitimize_operands (icode, 0, 1, ops)
> +   && maybe_emit_unop_insn (icode, ops[0].value, op0, UNKNOWN))
> + return ops[0].value;

What are you doing here that maybe_emit_unop_insn doesn't?

> +  if (maybe_expand_insn (unsignedp ? CODE_FOR_extzv : CODE_FOR_extv,
> +  4, ops))
>   {
> -   emit_insn (pat);
> +   xtarget = ops[0].value;
> if (xtarget == xspec_target)
>   return xtarget;
> -   if (xtarget == xspec_target_subreg)
> +   if (ops[0].value == xspec_target_subreg)
>   return xspec_target;

Why this last change?

>x = prepare_operand (icode, x, 2, mode, compare_mode, unsignedp);
>y = prepare_operand (icode, y, 3, mode, compare_mode, unsignedp);
>comparison = gen_rtx_fmt_ee (code, result_mode, x, y);
> -  if (!x || !y
> -  || !insn_data[icode].operand[2].predicate
> -   (x, insn_data[icode].operand[2].mode)
> -  || !insn_data[icode].operand[3].predicate
> -   (y, insn_data[icode].operand[3].mode)
> -  || !insn_data[icode].operand[1].predicate (comparison, VOIDmode))
> +  if (!x || !y)
>  {
>delete_insns_since (last);
>return NULL_RTX;

Seems like we ought to push the IF above generating COMPARISON now.


>  expand_widen_pattern_expr (sepops ops, rtx op0, rtx op1, rtx wide_op,
>  rtx target, int unsignedp)

Geezam.  I hope this one's correct -- the original code is impossible to
follow.  I suspect that it's trying to handle so many different cases at
once that it can't really validate anything at all.

> +  op = 0;
> +  create_output_operand (&eops[op++], target, TYPE_MODE (ops->type));
> +  create_convert_operand_from (&eops[op++], op0, tmode0, unsignedp);
>if (op1)
> +create_convert_operand_from (&eops[op++], op1, tmode1, unsignedp);
>if (wide_op)
> +create_convert_operand_from (&eops[op++], wide_op, wmode, unsignedp);
> +  expand_insn (icode, op, eops);
> +  return eops[0].value;

... the conversion is at least legible.

> +  if (commutative_p)
> +make_operand_commutative (&ops[1], 0);

So this is used exactly once here in expand_binop_directly?

Honestly, I found the description of make_operand_commutative to
be rather weak, and the implementation,

> +  for (i = 0; i + 1 < nops; i++)
> +if (ops[i].commutative < MAX_EXPAND_OPERANDS
> +   && swap_commutative_operands_with_target 
> (ops[ops[i].commutative].value,
> +

Re: Cleaning up expand optabs code

2011-03-17 Thread Richard Henderson 
On 03/17/2011 09:32 AM, Richard Sandiford wrote:
> This patch adds a few helper functions for dealing with optabs:
> 
> - insn_operand_matches (ICODE, OPNO, X)
> - (maybe_)legitimize_insn_target (ICODE, OPNO, TARGET, MODE)
> - (maybe_)legitimize_insn_source (ICODE, OPNO, SOURCE, MODE)

Cool.

Why pass in MODE in the later two cases?  Seems to me that your
argument for omitting it for insn_operand_matches is just as
compelling for the other functions...

> - I first tried to make insn_operand_matches an inline function,
>   but I can't think of a good header file that is guaranteed to
>   know about both recog.h and insn-codes.h.

Meh.  I can't imagine it mattering so much.  Anyway, an lto build
of gcc itself would fix that if needed, right?  ;-)

> -  char_rtx = const0_rtx;
> -  char_mode = insn_data[(int) icode].operand[2].mode;
> -  if (! (*insn_data[(int) icode].operand[2].predicate) (char_rtx,
> - char_mode))
> - char_rtx = copy_to_mode_reg (char_mode, char_rtx);
> -
> -  pat = GEN_FCN (icode) (result, gen_rtx_MEM (BLKmode, src_reg),
> -  char_rtx, GEN_INT (align));
> -  if (! pat)
> - return NULL_RTX;
> +  if (!(char_rtx = maybe_legitimize_insn_source (icode, 2, const0_rtx,
> +  VOIDmode))
> +   || !(pat = GEN_FCN (icode) (result, gen_rtx_MEM (BLKmode, src_reg),
> +   char_rtx, GEN_INT (align
> + {
> +   delete_insns_since (before_strlen);
> +   return NULL_RTX;
> + }
>emit_insn (pat);

I'm not so fond of burying assignments into conditionals.  I know we
do it a lot in gcc, and it's a tad harder to avoid here than ...

> -  if (!insn_data[icode].operand[1].predicate (val, mode))
> - val = force_reg (mode, val);
> -
> -  insn = GEN_FCN (icode) (mem, val);
> -  if (insn)
> +  start = get_last_insn ();
> +  if ((val = maybe_legitimize_insn_source (icode, 1, const0_rtx, mode))
> +   && (insn = GEN_FCN (icode) (mem, val)))
>   {
> emit_insn (insn);
> return;
>   }
> +  delete_insns_since (start);

... here, where it's just as readable to write

val = maybe_legitimize_insn_source (icode, 1, const0_rtx, mode);
if (val)
  {
insn = GEN_FCN (icode) (mem, val);
if (insn)
  {
emit_insn (insn);
return;
  }
  }
delete_insns_since (start);

> +  if ((temp = maybe_legitimize_insn_target (icode, 0, target, tmp_mode))
> +  && (xop0 = maybe_legitimize_insn_source (icode, 1, xop0, mode0))
> +  && (xop1 = maybe_legitimize_insn_source (icode, 2, xop1, mode1))
> +  && (pat = GEN_FCN (icode) (temp, xop0, xop1)))

Although, these patterns occur often enough that I wonder if there's a way
to tidy them up into a single function call.  We could either use a set of
functions like

target = maybe_legitimize_emit_insn_ts (icode, target, src1);
target = maybe_legitimize_emit_insn_tss (icode, target, src1, src2);

or have a single function with variadic source operands.  Probably the
separate functions is better for error checking within the compiler.  We
can validate the correct function was called for a given icode based on
the n_operands stored in the insn_data, and the compiler itself will make
sure that we didn't omit an argument for that function.

The interface I was considering here would validate the target and all
of the sources, emit the insn or delete_insns_since, and return the new
target or NULL if no insn was emitted.

All that said, I'm very much in favour of this cleanup.


r~