gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

I'm trying to use gcc-3.4.0 --target=m68k-uclinux, and it fails while
trhing to build printf.c from uClibc:

m68k-uclinux-gcc  -Wall -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing  -Os  -Wa,--bitwise-or -I/home/peter/work/src/ucLinux/ucTools/linux-2.4.x/include -m5200   -msoft-float -fno-builtin -nostdinc -D_LIBC -I../../include -I. -I/home/mylocal/uclinux/tools-3.4.0/lib/gcc/m68k-uclinux/3.4.0/include -DNDEBUG -DL__fpmaxtostr printf.c -c -o _fpmaxtostr.o
printf.c: In function `_fpmaxtostr':
printf.c:2454: error: insn does not satisfy its constraints:
(insn 1376 536 537 38 (set (reg:QI 8 %a0)
        (mem:QI (plus:SI (reg/f:SI 14 %a6)
                (const_int -209 [0xffffff2f])) [0 mode+0 S1 A8])) 33 {*m68k.md:826} (nil)
    (nil))
printf.c:2454: internal compiler error: in reload_cse_simplify_operands, at postreload.c:378
Please submit a full bug report,
with preprocessed source if appropriate.
See <URL:http://gcc.gnu.org/bugs.html> for instructions.
make[2]: *** [_fpmaxtostr.o] Error 1
make[2]: Leaving directory `/home/peter/work/src/ucLinux/ucTools/uClibc-0.9.26/libc/stdio'


This problem has been reported before, so I decided to try to fix it
by preventing bytes from being stored in address registers by modifying the
movqi pattern for ColdFire from :

(define_insn ""
  [(set (match_operand:QI 0 "nonimmediate_operand" "=d<Q>,dm,U,d*a")
	(match_operand:QI 1 "general_src_operand" "dmi,d<Q>,U,di*a"))]
  "TARGET_COLDFIRE"
  "* return output_move_qimode (operands);")

to:


(define_insn ""
  [(set (match_operand:QI 0 "nonimmediate_operand" "=d<Q>,dm,U,d")
	(match_operand:QI 1 "general_src_operand" "dmi,d<Q>,U,di"))]
  "TARGET_COLDFIRE"
  "* return output_move_qimode (operands);")


As well as modify HARD_REGNO_MODE_OK from:

#define HARD_REGNO_MODE_OK(REGNO, MODE) \
  (((REGNO) < 16					\
    && !((REGNO) < 8 && (REGNO) + GET_MODE_SIZE (MODE) / 4 > 8))	\
   || ((REGNO) >= 16 && (REGNO) < 24				        \
       && (GET_MODE_CLASS (MODE) == MODE_FLOAT		\
	   || GET_MODE_CLASS (MODE) == MODE_COMPLEX_FLOAT)		\
       && GET_MODE_UNIT_SIZE (MODE) <= 12))


To:

#define HARD_REGNO_MODE_OK(REGNO, MODE) \
  regno_mode_ok(REGNO, MODE)


along with the following code:

/* Value is 1 if hard register REGNO can hold a value of machine-mode MODE.
   On the 68000, the cpu registers can hold any mode except bytes in
   address registers, but the 68881 registers
   can hold only SFmode or DFmode.  */
int regno_mode_ok(int regno, enum machine_mode mode)
{
  if (regno < 8)
    {
      /* Data Registers */
      return 1;
    }
  else if (regno < 16)
    {
      /* Address Registers, can't hold bytes, can hold aggregate if
	 fits in */
      if (GET_MODE_SIZE (mode) == 1)
	return 0;
      if (!((regno) < 8 && (regno) + GET_MODE_SIZE (mode) / 4 > 8))
	return 1;
    }
  else if (regno < 24)
    {
      /* FPU registers, hold float or complex float of long double or smaller */
      if ((GET_MODE_CLASS (mode) == MODE_FLOAT
	   || GET_MODE_CLASS (mode) == MODE_COMPLEX_FLOAT)
	  && GET_MODE_UNIT_SIZE (mode) <= 12)
	return 0;
    }
  return 0;
}


Now fails with:

printf.c: In function `_fpmaxtostr':
printf.c:2454: error: unable to find a register to spill in class `ADDR_REGS'
printf.c:2454: error: this is the insn:
(insn 537 536 538 38 (set (subreg:SI (reg/v:QI 50 [ mode ]) 0)
        (plus:SI (subreg:SI (reg/v:QI 50 [ mode ]) 0)
            (const_int 32 [0x20]))) 90 {*addsi3_5200} (nil)
    (nil))

Breakpoint 1, fancy_abort (
    file=0x82582e0 "/home/peter/work/src/ucLinux/ucTools/gcc-3.4.0/gcc/reload1.c", 
    line=1874, function=0x82580f7 "spill_failure")
    at /home/peter/work/src/ucLinux/ucTools/gcc-3.4.0/gcc/diagnostic.c:584


Any suggestions where I go from here are most appreciated.

-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> Now fails with:
> 
> printf.c: In function `_fpmaxtostr':
> printf.c:2454: error: unable to find a register to spill in class `ADDR_REGS'
> printf.c:2454: error: this is the insn:
> (insn 537 536 538 38 (set (subreg:SI (reg/v:QI 50 [ mode ]) 0)
>         (plus:SI (subreg:SI (reg/v:QI 50 [ mode ]) 0)
>             (const_int 32 [0x20]))) 90 {*addsi3_5200} (nil)
>     (nil))
> 
> Breakpoint 1, fancy_abort (
>     file=0x82582e0 "/home/peter/work/src/ucLinux/ucTools/gcc-3.4.0/gcc/reload1.c", 
>     line=1874, function=0x82580f7 "spill_failure")
>     at /home/peter/work/src/ucLinux/ucTools/gcc-3.4.0/gcc/diagnostic.c:584
> 
> 
> Any suggestions where I go from here are most appreciated.

I think INDEX_REG_CLASS looks a bit fishy to me.  GENERAL_REGS is fine
for 68020, but is it really right for 68000 or Coldfire?  Perhaps it
should be DATA_REGS?

Actually, I don't understand the abort.  The last alternative for the
insn permits r,0,r, which can be matched by reloading the constant 32
into any register, so why is the code trying to spill ADDR_REGS?  Why
not GENERAL_REGS?

Using mrIKLs in a constraint is tricky stuff, anyhow.  It can often
lead to better code by causing gcc to use moveq to load some other
register, and then do the addition.  But that increases register
pressure; if no other register is available, then the resulting code
is actually worse.  It might be better to not use the 's' constraint,
but to instead use a peephole2 which uses match_scratch, a technique
which was not available when the m68k backend was written.

Ian

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

>> Now fails with:
>> 
>> printf.c: In function `_fpmaxtostr':
>> printf.c:2454: error: unable to find a register to spill in class `ADDR_REGS'
>> printf.c:2454: error: this is the insn:
>> (insn 537 536 538 38 (set (subreg:SI (reg/v:QI 50 [ mode ]) 0)
>>         (plus:SI (subreg:SI (reg/v:QI 50 [ mode ]) 0)
>>             (const_int 32 [0x20]))) 90 {*addsi3_5200} (nil)
>>     (nil))
>> 
>> Breakpoint 1, fancy_abort (
>>     file=0x82582e0 "/home/peter/work/src/ucLinux/ucTools/gcc-3.4.0/gcc/reload1.c", 
>>     line=1874, function=0x82580f7 "spill_failure")
>>     at /home/peter/work/src/ucLinux/ucTools/gcc-3.4.0/gcc/diagnostic.c:584
>> 
>> 
>> Any suggestions where I go from here are most appreciated.
>
>I think INDEX_REG_CLASS looks a bit fishy to me.  GENERAL_REGS is fine
>for 68020, but is it really right for 68000 or Coldfire?  Perhaps it
>should be DATA_REGS?

ColdFire allows for d8(Ax,Xi*SF) where Xi is either an address or data
register, abd the scalefactor(SF) is either 1,2,4, or 8 if the FPU is
attached, so INDEX_REG_CLASS looks right, at least for ColdFire.

>Using mrIKLs in a constraint is tricky stuff, anyhow.  It can often
>lead to better code by causing gcc to use moveq to load some other
>register, and then do the addition.  But that increases register
>pressure; if no other register is available, then the resulting code
>is actually worse.  It might be better to not use the 's' constraint,
>but to instead use a peephole2 which uses match_scratch, a technique
>which was not available when the m68k backend was written.

The constraint is limited to:

(define_insn ""
  [(set (match_operand:QI 0 "nonimmediate_operand" "=d<Q>,dm,d")
	(match_operand:QI 1 "general_src_operand" "dmi,d<Q>,di"))]
  "TARGET_COLDFIRE"
  "* return output_move_qimode (operands);")

Which doesn't allow for IKLs.  This constraint *should* work, right?
Which ColdFire pattern are you seeing 'mrIKLs' on that you think should be
changed?

-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> >Using mrIKLs in a constraint is tricky stuff, anyhow.  It can often
> >lead to better code by causing gcc to use moveq to load some other
> >register, and then do the addition.  But that increases register
> >pressure; if no other register is available, then the resulting code
> >is actually worse.  It might be better to not use the 's' constraint,
> >but to instead use a peephole2 which uses match_scratch, a technique
> >which was not available when the m68k backend was written.
> 
> The constraint is limited to:
> 
> (define_insn ""
>   [(set (match_operand:QI 0 "nonimmediate_operand" "=d<Q>,dm,d")
> 	(match_operand:QI 1 "general_src_operand" "dmi,d<Q>,di"))]
>   "TARGET_COLDFIRE"
>   "* return output_move_qimode (operands);")
> 
> Which doesn't allow for IKLs.  This constraint *should* work, right?
> Which ColdFire pattern are you seeing 'mrIKLs' on that you think should be
> changed?

addsi3_5200, which is the one which crashed reload in your last
message.

Ian

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

>> Which doesn't allow for IKLs.  This constraint *should* work, right?
>> Which ColdFire pattern are you seeing 'mrIKLs' on that you think should be
>> changed?
>
>addsi3_5200, which is the one which crashed reload in your last
>message.

Ah, I've been staring at RTL dumps too long.  I was focusing on the
move, not the add :)

I changed the constraint from 's' to 'i':

(define_insn "*addsi3_5200"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,r")
	(plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0")
		 (match_operand:SI 2 "general_src_operand" "d,rJK,a,mrIKLi")))]
  "TARGET_COLDFIRE"
  "* return output_addsi3 (operands);")

and the problem has gone away.  The resultant code that failed before looks like :

|(insn 1373 532 533 (set (reg:QI 0 %d0)
|        (mem:QI (plus:SI (reg/f:SI 14 %a6)
|                (const_int -112 [0xffffff90])) [0 S1 A8])) 33 {*m68k.md:826} (nil)
|    (nil))
	move.b -112(%a6),%d0	|,	| 1373	*m68k.md:826/1
|(insn 533 1373 534 (set (reg:SI 0 %d0 [177])
|        (plus:SI (reg:SI 0 %d0)
|            (const_int -32 [0xffffffe0]))) 90 {*addsi3_5200} (nil)
|    (nil))
	add.l #-32,%d0	|, tmp177	| 533	*addsi3_5200/4
|(insn 534 533 536 (set (mem:QI (plus:SI (reg/f:SI 14 %a6)
|                (const_int -112 [0xffffff90])) [0 S1 A8])
|        (reg:QI 0 %d0 [orig:177+3 ] [177])) 33 {*m68k.md:826} (insn_list 533 (nil))
|    (expr_list:REG_DEAD (reg:QI 0 %d0 [orig:177+3 ] [177])
|        (nil)))
	move.b %d0,-112(%a6)	| tmp177,	| 534	*m68k.md:826/2

Which is about as good as it can get on ColdFire since there is no
valid addqi3 pattern.

> >is actually worse.  It might be better to not use the 's' constraint,
> >but to instead use a peephole2 which uses match_scratch, a technique
> >which was not available when the m68k backend was written.

Which backend would be a good place to find examples of this type of
peephole2 pattern?  What is it that the pattern should do, create a
scratch register to stuff the constant?  Are peephole2 patterns run
after reload?

-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> Which is about as good as it can get on ColdFire since there is no
> valid addqi3 pattern.

Ah, now I see why reload was looking for ADDR_REG.  Because
addsi3_5200 has a ?a,a,rJK alternative, and -32 matches the J
constraint.  So I do think the problem is in that insn.  It will tend
to encourage the use of an address register in some cases for which a
general register is appropriate.

> > >is actually worse.  It might be better to not use the 's' constraint,
> > >but to instead use a peephole2 which uses match_scratch, a technique
> > >which was not available when the m68k backend was written.
> 
> Which backend would be a good place to find examples of this type of
> peephole2 pattern?

I dunno, anything really.  See the docs.

Here's a simple one from i386.md; i386.md is probably relevant since
it is also CISC:

(define_peephole2
  [(set (match_operand:SI 0 "push_operand" "")
	(match_operand:SI 1 "memory_operand" ""))
   (match_scratch:SI 2 "r")]
  "! optimize_size && ! TARGET_PUSH_MEMORY"
  [(set (match_dup 2) (match_dup 1))
   (set (match_dup 0) (match_dup 2))]
  "")

This says that if we are trying to push a value loaded from memory,
and we happen to have a free register (as indicated by match_scratch),
then optimize to load the register from memory, and then push the
register.  The resulting two simple instructions are presumably no
slower than the first complex instruction.  This then permits the load
and the push to be scheduled independently.

> What is it that the pattern should do, create a
> scratch register to stuff the constant?

You can use match_scratch to pick up a free register.  If all the
relevant registers are in use, the peephole will not trigger.

> Are peephole2 patterns run
> after reload?

Yes.  They are run after reload, but before the second instruction
scheduling pass.

Ian

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

>> Which is about as good as it can get on ColdFire since there is no
>> valid addqi3 pattern.
>
>Ah, now I see why reload was looking for ADDR_REG.  Because
>addsi3_5200 has a ?a,a,rJK alternative, and -32 matches the J
>constraint.  So I do think the problem is in that insn.  It will tend
>to encourage the use of an address register in some cases for which a
>general register is appropriate.

Why is 'JK' on constraints of both operands for '?a,rJK,rJK'?  SHould
that be 'r,0,rJK'?  Should I rearrange the constraints to put
'r,0,mrIi' ahead of the last two reload constraints as in:

(define_insn "*addsi3_5200"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,r,?a,?a")
	(plus:SI (match_operand:SI 1 "general_operand" "%0,0,a,rJK")
		 (match_operand:SI 2 "general_src_operand" "d,mrIi,rJK")))]
  "TARGET_COLDFIRE"
  "* return output_addsi3 (operands);")


Would this problem also exist for subsi3:

(define_insn "subsi3"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,d,a")
	(minus:SI (match_operand:SI 1 "general_operand" "0,0,0")
		  (match_operand:SI 2 "general_src_operand" "dT,mSrT,mSrs")))]
  ""
  "sub%.l %2,%0")

where the last constraint should add 'i'?

-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> >Ah, now I see why reload was looking for ADDR_REG.  Because
> >addsi3_5200 has a ?a,a,rJK alternative, and -32 matches the J
> >constraint.  So I do think the problem is in that insn.  It will tend
> >to encourage the use of an address register in some cases for which a
> >general register is appropriate.

Here is the current insn for reference:

(define_insn "*addsi3_5200"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,r")
	(plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0")
		 (match_operand:SI 2 "general_src_operand" "d,rJK,a,mrIKLs")))]
  "TARGET_COLDFIRE"
  "* return output_addsi3 (operands);")

Alternative 0 is memory += data register.  Alternative 1 is address
register plus address register plus (register or signed 16 bit
constant or any constant which moveq can't handle).  Alternative 2 is
(register or signed 16 bit constant or any constant which moveq can't
handle) plus address register.  Alternative 3 is any register +=
(memory or any register or addq constant or any constant which moveq
can't handle or subq constant or unknown constant).

I dont know why alternatives 1 and 2 both exist.  Since operand 1 is
commutative, there should be no need to have both alternatives.

I don't know why alternatives 1 and 2 have both 'J' and 'K'.  Every
integer value matches either 'J' or 'K'.  Specifying "rJK" is the same
as specifying "ri".

I tracked back through CVS history, and the constraints have looked
like that since 1992.  Looking through some old releases, I see that
in version 1.33, in 1989, the addsi3 insn looked like this:

(define_insn "addsi3"
  [(set (match_operand:SI 0 "general_operand" "=m,r,!a")
      (plus:SI (match_operand:SI 1 "general_operand" "%0,0,a")
               (match_operand:SI 2 "general_operand" "dIKLs,mrIKLs,rJK")))]

So the "rJK" has existed since then.  I have no idea why this was
written in this way.

The duplicated commutative version was added with this ChangeLog
entry:

Sat Feb 18 02:11:25 1989  Richard Stallman  (rms at sugar-bombs.ai.mit.edu)

	* m68k.md, alliant.md (addsi3): New alternative a=r+a, in addition
	to old a=a+r, needed since reload-insns are not commuted.

As far as I know this is no longer true, and the extra alternative is
no longer required.

> Why is 'JK' on constraints of both operands for '?a,rJK,rJK'?

Well, if I understand you correctly, there is no "?a,rJK,rJK".  There
is "?a,a,rJK", and "?a,rJK,a".

> SHould
> that be 'r,0,rJK'?

No, additions to any register are handled by alternative 3.

> Should I rearrange the constraints to put
> 'r,0,mrIi' ahead of the last two reload constraints as in:

Well, you don't need both 'I' and 'i', since 'i' will match anything
that 'I' does.  And I don't think the order of the alternatives will
matter very much in this case.

> Would this problem also exist for subsi3:
> 
> (define_insn "subsi3"
>   [(set (match_operand:SI 0 "nonimmediate_operand" "=m,d,a")
> 	(minus:SI (match_operand:SI 1 "general_operand" "0,0,0")
> 		  (match_operand:SI 2 "general_src_operand" "dT,mSrT,mSrs")))]
>   ""
>   "sub%.l %2,%0")
> 
> where the last constraint should add 'i'?

Yes, this looks the same to me.  The code is probably avoiding 'i'
because it is more efficient to load the constant into a register.
But again I think it would be better (with today's compiler) to permit
'i', and use a peephole2 to permit using a register when and if there
is a register available.  In fact, I would guess that each alternative
for this insn should support 'i', with appropriate peephole2s,
although I would want to doublecheck that you can indeed subtract any
immediate value from a memory location.

Ian

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Jim Wilson]

Ian Lance Taylor wrote:
> 	* m68k.md, alliant.md (addsi3): New alternative a=r+a, in addition
> 	to old a=a+r, needed since reload-insns are not commuted.
> As far as I know this is no longer true, and the extra alternative is
> no longer required.

We commute when reloading (input) insns, but we don't commute when 
emitting reload (output) insns.  See gen_reload () which generally just 
emits a reload the same way it was passed to it without checking for 
commutative operands.  This might be the reason why rms made this change.

gen_reload does handle the special case where we have 3 register 
operands and the second PLUS operand matches the output operand.  It 
doesn't handle the case where the first PLUS operand is a constant. 
Based on normal canonicalization rules, we shouldn't be getting (plus 
constant reg) here, but maybe there is an unusual case where this can 
happen.

I think it would be reasonable to consider this a reload bug though.

Actually, I see that current code handles SUBREG and the old gcc-2.2.2 
code does not.  Maybe that was the problem.  In which case, there still 
seems to be a bug in the current sourecs, because the the commutative 
check in gen_reload doesn't handle SUBREGs, so in theory we could still 
end up with the a=r+a case.  It will be rejected, and then we will emit 
an alternative sequence, so this will still work, but you will silently 
lose some performance because you get a less efficient reload sequence 
generated.  Again, this probably should be fixed in gen_reload rather 
than in m68k.md.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

>> >Ah, now I see why reload was looking for ADDR_REG.  Because
>> >addsi3_5200 has a ?a,a,rJK alternative, and -32 matches the J
>> >constraint.  So I do think the problem is in that insn.  It will tend
>> >to encourage the use of an address register in some cases for which a
>> >general register is appropriate.
>
>Here is the current insn for reference:
>
>(define_insn "*addsi3_5200"
>  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,r")
>	(plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0")
>		 (match_operand:SI 2 "general_src_operand" "d,rJK,a,mrIKLs")))]
>  "TARGET_COLDFIRE"
>  "* return output_addsi3 (operands);")
>
>Alternative 0 is memory += data register.  Alternative 1 is address
>register plus address register plus (register or signed 16 bit
>constant or any constant which moveq can't handle).  Alternative 2 is
>(register or signed 16 bit constant or any constant which moveq can't
>handle) plus address register.  Alternative 3 is any register +=
>(memory or any register or addq constant or any constant which moveq
>can't handle or subq constant or unknown constant).

I'm still wondering why replacing 's' with 'i' in the last alternative
makes the issue go away, and produces(in this particular case), good
code.

As for the peephole2 to use moveq to load the constant into a data
register, does this look right?

(define_peephole2
  [(match_scratch:SI 2 "d")
   (set (match_operand:SI 0 "register_or_memory_operand" "rm")
        (plus:SI (match_dup 0)
                 (match_operand:SI 1 "immediate_operand" "K")))]
  "!symbolic_operand (operands[1], SImode)"
  [(set (match_dup 2) (match_dup 1))
   (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))]
  "")

-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> >Here is the current insn for reference:
> >
> >(define_insn "*addsi3_5200"
> >  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,r")
> >	(plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0")
> >		 (match_operand:SI 2 "general_src_operand" "d,rJK,a,mrIKLs")))]
> >  "TARGET_COLDFIRE"
> >  "* return output_addsi3 (operands);")
> >
> >Alternative 0 is memory += data register.  Alternative 1 is address
> >register plus address register plus (register or signed 16 bit
> >constant or any constant which moveq can't handle).  Alternative 2 is
> >(register or signed 16 bit constant or any constant which moveq can't
> >handle) plus address register.  Alternative 3 is any register +=
> >(memory or any register or addq constant or any constant which moveq
> >can't handle or subq constant or unknown constant).
> 
> I'm still wondering why replacing 's' with 'i' in the last alternative
> makes the issue go away, and produces(in this particular case), good
> code.

The constraint 's' matches any integer whose value is not known.  A
typical example would be the address of a variable.  In your example,
the value of the second operand was (I think) the constant -32.  Since
that value is known, the constraint 's' does not match.  The other
constraints, "mrIKL", also do not match a constant -32.  So the last
alternative does not match.  The constraint 'i', on the other hand,
matches any constant integer.  So when we change 's' to 'i', the last
alternative does match, and gcc uses it.

Alternative 2 does match with -32, but that alternatives requires
adding an address register to an address register.  My guess is that
gcc chooses that alternative, and tries to find an address register to
use.  It appears that, it can't, perhaps because they have all been
used for other purposes.  That leads to the "unable to find a register
to spill in class ADDR_REGS" message.  This is a plausible scenario,
though it's hard to be certain without the full test case (which I
don't particularly want).

> As for the peephole2 to use moveq to load the constant into a data
> register, does this look right?
> 
> (define_peephole2
>   [(match_scratch:SI 2 "d")
>    (set (match_operand:SI 0 "register_or_memory_operand" "rm")
>         (plus:SI (match_dup 0)
>                  (match_operand:SI 1 "immediate_operand" "K")))]
>   "!symbolic_operand (operands[1], SImode)"
>   [(set (match_dup 2) (match_dup 1))
>    (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))]
>   "")

That's not quite what I was thinking of.  I was thinking that would
match constants which moveq can handle, which is the reverse of the
constraints 'K' (if you can't use moveq for the constant, this
optimization doesn't really help, as far as I can see).  Also, you
don't need to check symbolic_operand condition, because the constraint
'K' will only match a CONST_INT anyhow.  I would try something like
this:

(define_peephole2
  [(match_scratch:SI 2 "d")
   (set (match_operand:SI 0 "register_or_memory_operand" "rm")
        (plus:SI (match_dup 0)
                 (match_operand:SI 1 "immediate_operand" "J")))]
  "INTVAL (operand[1]) >= -0x80 && INTVAL (operand[1]) < 0x80"
  [(set (match_dup 2) (match_dup 1))
   (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))]
  "")

Ian

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

>That's not quite what I was thinking of.  I was thinking that would
>match constants which moveq can handle, which is the reverse of the
>constraints 'K' (if you can't use moveq for the constant, this
>optimization doesn't really help, as far as I can see).  Also, you
>don't need to check symbolic_operand condition, because the constraint
>'K' will only match a CONST_INT anyhow.  I would try something like
>this:
>
>(define_peephole2
>  [(match_scratch:SI 2 "d")
>   (set (match_operand:SI 0 "register_or_memory_operand" "rm")
>        (plus:SI (match_dup 0)
>                 (match_operand:SI 1 "immediate_operand" "J")))]
>  "INTVAL (operand[1]) >= -0x80 && INTVAL (operand[1]) < 0x80"
>  [(set (match_dup 2) (match_dup 1))
>   (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))]
>  "")

I need to ingest more caffeine before I try to write RTL :)  I fat-fingered the 'J'.

For ColdFire(at least on the newer cores), the immediate add is
faster(but larger) than the moveq/add.  If there are data registers
available, will this peephole(if matched multiple times in a block for
the *same* constant), create more than one constant register loaded
with moveq, or is the compiler smart enough to coalesce them together?

I'll give the following a whirl and see what happens:

(define_peephole2
  [(match_scratch:SI 2 "d")
   (set (match_operand:SI 0 "register_or_memory_operand" "rm")
        (plus:SI (match_dup 0)
                 (match_operand:SI 1 "immediate_operand" "J")))]
  "optimize_size && INTVAL (operand[1]) >= -0x80 && INTVAL (operand[1]) < 0x80"
  [(set (match_dup 2) (match_dup 1))
   (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))]
  "")


-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> For ColdFire(at least on the newer cores), the immediate add is
> faster(but larger) than the moveq/add.

OK, then we don't want to use this for Coldfire.  But I suspect that
we do want to use it for 680x0.

> If there are data registers
> available, will this peephole(if matched multiple times in a block for
> the *same* constant), create more than one constant register loaded
> with moveq, or is the compiler smart enough to coalesce them together?

The compiler might coalesce them together.  It's not impossible,
though it's not likely to do a great job on it.  You'll have to see.

Ian

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Peter Barada]

>> For ColdFire(at least on the newer cores), the immediate add is
>> faster(but larger) than the moveq/add.
>
>OK, then we don't want to use this for Coldfire.  But I suspect that
>we do want to use it for 680x0.

For m68k, the pattern is

(define_insn "*addsi3_internal"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,d,a")
        (plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0,0")
                 (match_operand:SI 2 "general_src_operand" "dIKLT,rJK,a,mSrIKLT,mSrIKLs")))]


  "! TARGET_COLDFIRE"
  "* return output_addsi3 (operands);")

Which should work correctly since it would force a register to be used
to hold the constant, and I'm assuming that by seperating the last two
alternatives that the compiler will favor a data register over an
address register.

I'll modify the peephole2 to have TARGET_COLDFIRE in it to prevent it
from triggering for m68k.

-- 
Peter Barada
peter@the-baradas.com

Re: gcc-3.4.0 fails for ColdFire(does not satisfy constraints)

[Ian Lance Taylor]

Peter Barada <peter@the-baradas.com> writes:

> >> For ColdFire(at least on the newer cores), the immediate add is
> >> faster(but larger) than the moveq/add.
> >
> >OK, then we don't want to use this for Coldfire.  But I suspect that
> >we do want to use it for 680x0.
> 
> For m68k, the pattern is
> 
> (define_insn "*addsi3_internal"
>   [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,d,a")
>         (plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0,0")
>                  (match_operand:SI 2 "general_src_operand" "dIKLT,rJK,a,mSrIKLT,mSrIKLs")))]
> 
> 
>   "! TARGET_COLDFIRE"
>   "* return output_addsi3 (operands);")
> 
> Which should work correctly since it would force a register to be used
> to hold the constant, and I'm assuming that by seperating the last two
> alternatives that the compiler will favor a data register over an
> address register.

It will indeed force a register to be used, but I think that in the
general case we do not want to force a register to be used.  If we
always force a register to be used, then we increase register
pressure.  If we force a register to be spilled due to constraint
failure, then we have generated worse code.  The advantage of the
peephole2 approach is that it will use a register only if there is one
available.  If all the registers are being used for something else,
then we won't force one of them to be spilled just to hold our
constant.

Either choice will generate worse code in some situations.  My guess
is that the peephole2 approach will be better on average.  But it
should be tested on some benchmarks.

Ian