| Message ID | 20160222130839.GJ3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/22/2016 06:08 AM, Jakub Jelinek wrote: > Hi! > > Supposedly my recent invalid shift count expansion changes broke > m68k bootstrap, we now really require that the shift expanders > have some non-VOIDmode, so that we can convert_mode it to that > mode. But m68k didn't specify mode. For valid shift counts > the patch makes no difference beyond fixing the ICE, those are > all CONST_INTs which when converted to CONST_INTs valid for > SImode give the same values. > > Fixed thusly, Matthias has kindly tested it on m68k-linux, ok for trunk? > > 2016-02-22 Jakub Jelinek <jakub@redhat.com> > > PR target/69885 > * config/m68k/m68k.md (ashldi3, ashrdi3, lshrdi3): Use > SImode for last match_operand. > > * gcc.dg/pr69885.c: New test. OK for the trunk. Can you take a peek at other ports and see if any have the same issue? It wouldn't surprise me if some of the older ports did. If so, similar patches for other ports are pre-approved. It also seems like an update for md.texi is in order. That can be handled as a pre-approved follow-up as well. jeff
On Mon, Feb 22, 2016 at 07:09:41AM -0700, Jeff Law wrote: > On 02/22/2016 06:08 AM, Jakub Jelinek wrote: > >Hi! > > > >Supposedly my recent invalid shift count expansion changes broke > >m68k bootstrap, we now really require that the shift expanders > >have some non-VOIDmode, so that we can convert_mode it to that > >mode. But m68k didn't specify mode. For valid shift counts > >the patch makes no difference beyond fixing the ICE, those are > >all CONST_INTs which when converted to CONST_INTs valid for > >SImode give the same values. > > > >Fixed thusly, Matthias has kindly tested it on m68k-linux, ok for trunk? > > > >2016-02-22 Jakub Jelinek <jakub@redhat.com> > > > > PR target/69885 > > * config/m68k/m68k.md (ashldi3, ashrdi3, lshrdi3): Use > > SImode for last match_operand. > > > > * gcc.dg/pr69885.c: New test. > OK for the trunk. Can you take a peek at other ports and see if any have > the same issue? It wouldn't surprise me if some of the older ports did. If > so, similar patches for other ports are pre-approved. Quick grepping for 'match_operand .*const_int_operand' used in define_insn/define_expand that could possibly be any of the shift/rotate expanders didn't reveal anything in any other port. > It also seems like an update for md.texi is in order. That can be handled > as a pre-approved follow-up as well. We already document in md.texi: Here @var{m} is the mode of operand 0 and operand 1; operand 2's mode is specified by the instruction pattern, and the compiler will convert the operand to that mode before generating the instruction. Would you like to somehow stress that operand 2's mode must be specified in the instruction pattern or expander? As it says that the compiler will convert it to that mode, I'd kind of say that VOIDmode should not be used there. Jakub
Jakub Jelinek <jakub@redhat.com> writes: > Would you like to somehow stress that operand 2's mode must be specified > in the instruction pattern or expander? As it says that the compiler will > convert it to that mode, I'd kind of say that VOIDmode should not be used > there. Perhaps a warning could be emitted by genrecog to that effect? It already warns for some uses of modeless match_operand. Andreas.
On 02/22/2016 07:28 AM, Jakub Jelinek wrote: > On Mon, Feb 22, 2016 at 07:09:41AM -0700, Jeff Law wrote: >> On 02/22/2016 06:08 AM, Jakub Jelinek wrote: >>> Hi! >>> >>> Supposedly my recent invalid shift count expansion changes broke >>> m68k bootstrap, we now really require that the shift expanders >>> have some non-VOIDmode, so that we can convert_mode it to that >>> mode. But m68k didn't specify mode. For valid shift counts >>> the patch makes no difference beyond fixing the ICE, those are >>> all CONST_INTs which when converted to CONST_INTs valid for >>> SImode give the same values. >>> >>> Fixed thusly, Matthias has kindly tested it on m68k-linux, ok for trunk? >>> >>> 2016-02-22 Jakub Jelinek <jakub@redhat.com> >>> >>> PR target/69885 >>> * config/m68k/m68k.md (ashldi3, ashrdi3, lshrdi3): Use >>> SImode for last match_operand. >>> >>> * gcc.dg/pr69885.c: New test. >> OK for the trunk. Can you take a peek at other ports and see if any have >> the same issue? It wouldn't surprise me if some of the older ports did. If >> so, similar patches for other ports are pre-approved. > > Quick grepping for 'match_operand .*const_int_operand' used in > define_insn/define_expand that could possibly be any of the shift/rotate > expanders didn't reveal anything in any other port. Excellent. Thanks for verifying. > >> It also seems like an update for md.texi is in order. That can be handled >> as a pre-approved follow-up as well. > > We already document in md.texi: > Here @var{m} is the mode of > operand 0 and operand 1; operand 2's mode is specified by the > instruction pattern, and the compiler will convert the operand to that > mode before generating the instruction. > > Would you like to somehow stress that operand 2's mode must be specified > in the instruction pattern or expander? As it says that the compiler will > convert it to that mode, I'd kind of say that VOIDmode should not be used > there. Yea, as it's currently written, it's a bit vague. I think your suggesting of saying that VOIDmode should not be used is what we need. I'll leave the final word-smithing to you. jeff
--- gcc/config/m68k/m68k.md.jj 2016-01-20 10:55:16.000000000 +0100 +++ gcc/config/m68k/m68k.md 2016-02-22 11:14:01.789697673 +0100 @@ -4544,7 +4544,7 @@ (define_insn "*ashldi3" (define_expand "ashldi3" [(set (match_operand:DI 0 "register_operand" "") (ashift:DI (match_operand:DI 1 "register_operand" "") - (match_operand 2 "const_int_operand" "")))] + (match_operand:SI 2 "const_int_operand" "")))] "!TARGET_COLDFIRE" { /* ??? This is a named pattern like this is not allowed to FAIL based @@ -4813,7 +4813,7 @@ (define_insn "ashrdi_const" (define_expand "ashrdi3" [(set (match_operand:DI 0 "register_operand" "") (ashiftrt:DI (match_operand:DI 1 "register_operand" "") - (match_operand 2 "const_int_operand" "")))] + (match_operand:SI 2 "const_int_operand" "")))] "!TARGET_COLDFIRE" { /* ??? This is a named pattern like this is not allowed to FAIL based @@ -5082,7 +5082,7 @@ (define_insn "*lshrdi3_const" (define_expand "lshrdi3" [(set (match_operand:DI 0 "register_operand" "") (lshiftrt:DI (match_operand:DI 1 "register_operand" "") - (match_operand 2 "const_int_operand" "")))] + (match_operand:SI 2 "const_int_operand" "")))] "!TARGET_COLDFIRE" { /* ??? This is a named pattern like this is not allowed to FAIL based --- gcc/testsuite/gcc.dg/pr69885.c.jj 2016-02-22 11:14:50.981023120 +0100 +++ gcc/testsuite/gcc.dg/pr69885.c 2016-02-22 11:13:45.000000000 +0100 @@ -0,0 +1,13 @@ +/* PR target/69885 */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ +/* { dg-additional-options "-m68000" { target m68k*-*-* } } */ + +void bar (void); + +void +foo (long long x) +{ + if (x >> 1) + bar (); +}