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