Patchwork Cleaning up expand optabs code

login
register
mail settings
Submitter Richard Henderson
Date March 25, 2011, 5:17 p.m.
Message ID <4D8CCE3D.7010100@redhat.com>
Download mbox | patch
Permalink /patch/88403/
State New
Headers show

Comments

Richard Henderson - March 25, 2011, 5:17 p.m.
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~
Richard Sandiford - March 25, 2011, 5:51 p.m.
Richard Henderson <rth@redhat.com> 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
Richard Sandiford - March 25, 2011, 6:29 p.m.
Richard Sandiford <richard.sandiford@linaro.org> writes:
> Richard Henderson <rth@redhat.com> 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
Richard Sandiford - March 25, 2011, 6:30 p.m.
Richard Sandiford <richard.sandiford@linaro.org> writes:
> Er, >= 4 even...

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

Richard
Richard Henderson - March 25, 2011, 6:49 p.m.
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~
Georg-Johann Lay - March 31, 2011, 9:49 a.m.
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
Richard Sandiford - March 31, 2011, 12:43 p.m.
Georg-Johann Lay <avr@gjlay.de> 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
Georg-Johann Lay - April 1, 2011, 12:54 p.m.
Richard Sandiford schrieb:
> Georg-Johann Lay <avr@gjlay.de> 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

Patch

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;