Re: RFA: MN10300: Extend LIW support

[Nick Clifton <nickc at redhat dot com>]

Hi Richard,

  Thanks for the review.  Attached is a revised patch which addresses 
the points you raised.

> Not check_liw_constraints, but liw_candidate.  All you have to do for that
> is move the single_set check to the top of extract_bundle.

Ah yes, that makes more sense - done.


> I wonder if this would be better as
>
>    bool allow_const = true;
> ...
>      case LIW_OP_AND:
>      case LIW_OP_OR:
>      case LIW_OP_XOR:
>        allow_const = false;
>        /* FALLTHRU */
>      default:
>        ...

It would - also done.


>    return satisfies_constraint_O (pdata->src);
>
> At minimum, notice the last line instead of constraint_satisfied_p.

Well I was using constraint_satisfied_p so that I would not need to 
include another header file (tm-constrs.h) but I guess it does not 
matter too much, so - done.


> Why not split out the swap operands test from the constraints test?
> I think that would make this logic much easier to follow.
>
> static bool
> check_liw_swap_valid (struct liw_data * pliw1, struct liw_data * pliw2)
> {
>    if (pliw2->op == LIW_OP_CMP)
>      return true;
>    if (REGNO (pliw1->dest) == REGNO (pliw2->dest))
>      return false;
>    if (REG_P (pliw2->src)&&  REGNO (pliw1->dest) == REGNO (pliw2->src))
>      return false;
>    return true;
> }

Hmm - the problem with this is that is looses the optimization of 
changing the source of LIW2 to be the source of LIW1 when the 
destination of LIW1 is the source of LIW2.

But you are right, the "swapped" variable is confusing, and once I had 
thought about it, completely unnecessary.  The revised patch eliminates 
it altogether.


> >   (define_insn "ashlsi3"
> > -  [(set (match_operand:SI  0 "register_operand"   "=r,D,d,d,D,D,r")
> > +  [(set (match_operand:SI  0 "register_operand"   "=r,D,d,d,D,D,D,r")
> >   	(ashift:SI
> > -	  (match_operand:SI 1 "register_operand"  " 0,0,0,0,0,0,r")
> > -	  (match_operand:QI 2 "nonmemory_operand" " J,K,M,L,D,i,r")))
> > +	  (match_operand:SI 1 "register_operand"  " 0,0,0,0,0,0,0,r")
> > +	  (match_operand:QI 2 "nonmemory_operand" " J,K,M,L,D,O,i,r")))

> Note that all those J,K,M,L alternatives satisfy O as well.

Ah thanks.

So the revised patch still works, does not introduce any regressions and 
produces improved code for the AM33+.  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2011-02-15  Nick Clifton  <nickc@redhat.com>

	* config/mn10300/mn10300.c: Include tm-constrs.h.
	(struct liw_data): New data structure describing an LIW candidate
	instruction.
	(extract_bundle): Use struct liw_data.  Allow small integer
	operands for some instructions.
	(check_liw_constraints): Use struct liw_data.  Remove swapped
	parameter.  Add comments describing the checks.  Fix bug when
	assigning the source of liw1 to the source of liw2.
	(liw_candidate): Delete.  Code moved into extract_bundle.
	(mn10300_bundle_liw): Use struct liw_data.  Check constraints
	before swapping.
	* config/mn10300/predicates.md (liw_operand): New predicate.
	Allows registers and small integer constants.
	* config/mn10300/constraints.md (O): New constraint.  Accetps
	integers in the range -8 to +7 inclusive.
	* config/mn10300/mn10300.md (movesi_internal): Add an alternative
	for moving a small integer into a register.  Give this alternative
	LIW attributes.
	(addsi3, subsi3, cmpsi, lshrsi3, ashrsi3): Likewise.
	(ashlsi3): Likewise, plus give LIW attributes to the alternatives
	using the J,K,L and M constraints,
	(liw): Remove SI mode on second operands to allow for HI and QI
	mode values.
	(cmp_liw, liw_cmp): Likewise.  Plus fix order of operands in the
	instruction.

Attachment: mn10300.liw.patch.6
Description: Unix manual page