RFA: MN10300: Add LIW and SETLB support

RFA: MN10300: Add LIW and SETLB support

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 3051 bytes --]

Hi Guys,

  Here is the next MN10300 patch.  This one adds support for the Long
  Instruction Word instructions and the SETLB and Lcc instructions.

  The new instructions are not enabled by default.  I am not exactly
  sure why this is, but I do remember that it was at the request of
  Mitsubishi when the work was being done.  So I have also created a new
  set of multilibs with the instructions enabled.

  Tested without regressions, both with and without the new instructions
  enabled, using an mn10300-elf toolchain.

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2010-11-05  Nick Clifton  <nickc@redhat.com>

	* config/mn10300/mn10300.c (liw_op_names): New array.
	(mn10300_print_operand): Handle 'W' operand.
	(mn10300_encode_section_info): Call default_encode_section_info
	after mn10300 specific processing.
	(extract_bundle): New function: Extracts the components
	of a LIW instruction.
	(liw_op_uses_only_regs): New function.
	(check_liw_constraints): New function.
	(liw_candidate): New function.  Determines in the given insn is
	a suitable candidate for conversion into an LIW instruction.
	(mn10300_bundle_liw): New function.  Converts pairs of insns
	into single LIW instructions.
	(mn10300_bundle_lcc): New function.  Combines a MOV and an LCC
	instruction into a single LIW instruction.
	(mn10300_insert_setlb_lcc): New function.  Replaces a JUMP with
	an LCC instruction.
	(mn10300_scan_for_setlb_lcc): New function.  Look for small loops
	that can use the SETLB/LCC instruction pair.
	(scan_for_redundant_compares): New function.  Eliminates unnecessary
	compare instructions.
	(mn10300_reorg): New function.  Run MN10300 specific optimizations.
	(TARGET_MACHINE_DEPENDENT_REORG): Define.
	* config/mn10300/mn10300.opt (mliw): New command line option.
	Enables the generation of LIW instructions.
	(msetlb): New command line option.  Enables the generation of SETLB
	LCC instructions.
	* config/mn10300/t-mn10300 (MULTILIB_OPTIONS): Add mliw|msetlb.
	(MULTILIB_DIRNAMES): Add liw.
	* config/mn10300/mn10300.h (TARGET_CPP_CPU_BUILTINS): Add __LIW__ and
	__SETLB__.
	* config/mn10300/constraints.md (O): New constraint.
	* config/mn10300/mn10300.md (UNSPEC_LIW_1, UNSPEC_LIW_2,
	UNSPEC_LCC, UNSPEC_SETLB): New constants.
	(liw_bundling): New automaton.
	(liw): New attribute.
	(liw_op): New attribute.
	(movsi_internal): Add liw and liw_op attributes.
	(am33_addsi3, am33_subsi3, am33_andsi3, am33_iorsi3, am33_xorsi3,
	 cmpsi, am33_ashlsi3, am33_lshrsi3, am33_ashrsi3): Likewise.
	(udivmodsi4): Remove unnecessary expander.
	(am33_subsi3): Separate alternatives involving immediates.
	(am33_andis3, am33_iorsi3, am33_xorsi3, am33_ashlsi3,
	 am33_lshrsi3, am33_ashrsi3): Likewise.
	(cbranchsi4_post_reload): Remove anonymity.
	(cbranchsf4_post_reload): Likewise.
	(loop_integer_conditional_branch): New pattern.
	(loop_floating_conditional_branch): New pattern.
	(setlb): New pattern.
	(lcc): New pattern.
	(liw_1): New pattern.
	(liw_lcc): New pattern.
	(* doc/invoke.texi: Document new command line options -mliw and
	-msetlb.

[-- Attachment #2: am33.liw.patch.bz2 --]
[-- Type: application/x-bzip2, Size: 8467 bytes --]

Re: RFA: MN10300: Add LIW and SETLB support

[Richard Henderson]

On 11/05/2010 02:52 AM, Nick Clifton wrote:
>   Here is the next MN10300 patch.  This one adds support for the Long
>   Instruction Word instructions and the SETLB and Lcc instructions.
> 
>   The new instructions are not enabled by default.  I am not exactly
>   sure why this is, but I do remember that it was at the request of
>   Mitsubishi when the work was being done.  So I have also created a new
>   set of multilibs with the instructions enabled.
> 
>   Tested without regressions, both with and without the new instructions
>   enabled, using an mn10300-elf toolchain.

If the patch is not "too big", can you please not compress the patch file?
Having to extract the thing and uncompress it just adds an unnecessary step.

Also, I think the compare optimization ought to have been submitted as a
separate patch.  It's not really related to the LIW and SETLB bundling.

Thanks.

> +static const char *liw_op_names[] =

const char * const

> +	fputs (liw_op_names[ops / LIW_OP_MAX], file);
> +	fputc ('_', file);
> +	fputs (liw_op_names[ops % LIW_OP_MAX], file);

Ug.  What a sucky assembler syntax.

It seems that a good fraction of the ugliness in this
patch is due to this.  I guess we have to live with it now,
but an assembler syntax that used some sort of bundling
marker (e.g. "||") would have made insn output here much
saner.  Oh well.

> +liw_candidate (rtx insn)
> +{
> +  rtx p;
> +
> +  if (insn == NULL || ! INSN_P (insn))
> +    return false;
> +
> +  p = PATTERN (insn);
> +  if (GET_CODE (p) == PARALLEL
> +      && GET_CODE (XVECEXP (p, 0, 1)) == CLOBBER)
> +    p = XVECEXP (p, 0, 0);
> +
> +  return GET_CODE (p) == SET;
> +}

First I was going to say, isn't this single_set_p?  Then
I was going to wonder why bother, as the most important
filter -- i.e. the one that discards the most first try --
is the get_attr_liw check.  But then I remembered such
things as bare USE and CLOBBER insns, which seem like they
can hang around in the stream long after they're useful.

Perhaps better would be something like

enum attr_liw
safe_get_attr_liw (rtx insn)
{
  if (INSN_P (insn) && recog_memoized (insn) >= 0)
    return get_attr_liw (insn);
  else
    return LIW_BOTH;
}

> +      insn2 = NEXT_INSN (r);
> +      /* Issue 115903: Do not let line number notes prevent this optimization,
> +	 as otherwise the code produce with "-g" enabled will be different
> +	 from the code produced without "-g".  */
> +      if (GET_CODE (insn2) == NOTE
> +	  && NOTE_KIND (insn2) > 0
> +	  && NEXT_INSN (insn2) != NULL)
> +	insn2 = NEXT_INSN (insn2);

next_nonnote_nondebug_insn

Between this search and the loop over R, it would seem
that you're doing more searching than necessary.

> +      if (liw1 == LIW_OP2 || liw2 == LIW_OP1)
> +	{
> +	  rtx r;

Shadowing R variable.  A different name would be better.

> +  setlb = emit_insn_before (gen_setlb (), label);

One of these days we won't throw away the CFG before machine_reorg
and this will start to fail.  Indeed:

> +  /* What we're doing here is looking for a conditional branch which
> +     branches backwards without crossing any non-debug labels.  I.e.
> +     the loop has to enter from the top.  */

... it seems like this bit would be greatly aided by re-generating
the loop nest, and looking at that.  See flow_loops_find.

Of course, we'd have to rebuild the CFG in order to get this.  Perhaps
we can arrange for some sort of bit on the targetm structure to delay
the CFG destruction until after machine_reorg.

I guess I'm ok with this section going in as-is, but consider it for a
future cleanup.

> +	  /* FIXME: We should scan backwards until the first ESPW
> +	     setter or clobber insn is found (or the beginning of
> +	     the block).  At the moment we just look back one insn.  */
> +	  prev_insn = prev_nonnote_insn (cur_insn);

prev_nonnote_nondebug_insn.  Of course, given that you're scanning
forward in the first place, you really could just hang on to it
when moving forward to the next insn.  That would also automatically
solve your some-previous-setter problem.

> +	  /* Adding 1 or 4 to an address register results in an
> +	     INC/INC4 instruction that doesn't set the flags.  */
> +	  if (GET_CODE (SET_SRC (pattern)) == PLUS
> +	      && REG_P (SET_DEST (pattern))
> +	      && REGNO (SET_DEST (pattern)) >= FIRST_ADDRESS_REGNUM
> +	      && REGNO (SET_DEST (pattern)) <= LAST_ADDRESS_REGNUM
> +	      && REG_P (XEXP (SET_SRC (pattern), 0))
> +	      && REGNO (XEXP (SET_SRC (pattern), 0)) == REGNO (SET_DEST (pattern))
> +	      && CONST_INT_P (XEXP (SET_SRC (pattern), 1))
> +	      && (INTVAL (XEXP (SET_SRC (pattern), 1)) == 1
> +		  || INTVAL (XEXP (SET_SRC (pattern), 1)) == 4))
> +	    continue;

You should *really* describe this instead via an insn attribute.
I.e. mark the alternative that generates this as not setting the
flags.  You really don't want to get into the business of matching
all sorts of patterns here.

> +#if 0
> +	      /* Some alternatives in the AND pattern use EXTBU which does
> +		 not set the flags.  Hence a CMP following an AND might be
> +		 needed.  */
> +	    case AND:

Case in point.

> +(define_insn "setlb"
> +  [(unspec [(const_int 0)] UNSPEC_SETLB)]
> +  "TARGET_ALLOW_SETLB"
> +  "setlb"
> +)
> +
> +(define_expand "lcc"
> +  [(parallel [(match_operand 0 "" "")
> +	      (unspec [(const_int 0)] UNSPEC_LCC)
> +	     ])
> +  ]
> +  "TARGET_ALLOW_SETLB"
> +  ""
> +)

Any good reason not to represent the hard register involved here?  I.e.

(define_insn "setlb"
  [(set (reg:P LB) (pc))]
  ...)

(define_insn "loop_integer_conditional_branch"
  [(set (pc)
	(if_then_else (match_operator ...)
	  (reg:P LB)
	  (pc))
   (use (label_ref (match_operand)))]
  ...)

Not terribly important either way, but just wondering...

> +(define_insn "liw_1"
> +  [(unspec [(match_operand 0 "" "")
> +	    (match_operand 1 "" "")
> +	    (match_operand 2 "" "")
> +	    (match_operand 3 "" "")
> +	    (match_operand 4 "" "")
> +	    ] UNSPEC_LIW_1)]
> +  "TARGET_ALLOW_LIW"
> +  "%W0 %2, %1, %4, %3"
> +  [(set (attr "timings") (if_then_else (eq_attr "cpu" "am34") (const_int 13) (const_int 12)))]
> +)
> +
> +(define_insn "liw_lcc"
> +  [(unspec [(match_operand 0 "" "")
> +	    (match_operand 1 "" "")
> +	    (match_operand 2 "" "")
> +	    ] UNSPEC_LIW_2)]
> +  "TARGET_ALLOW_SETLB"
> +  "mov_l%b2 %Q1, %0"
> +  [(set_attr "timings" "13")]
> +)

These I really don't like at all, since they break rtl.  I'd much prefer
a representation that at least retains a modicum of correctness.  E.g.

(define_insn "liw_1"
  [(set (match_operand 0 "register_operand" "")
	(unspec [(match_operand 2 "" "")] UNSPEC_LIW_1))
   (set (match_operand 1 "register_operand" "")
	(unspec [(match_operand 3 "" "")] UNSPEC_LIW_2))
   (use (match_operand 4 "const_int_operand" ""))]
  "TARGET_ALLOW_LIW"
  "%W4 %2, %0, %3, %1"
  ...)

(define_insn "liw_lcc"
  [(set (match_operand 0) (match_operand 1))
   (set (pc) (...)]
  ...)

Although it would seem that with a little effort we wouldn't need to use
unspecs at all, but could just use a couple of match_operators and really
honestly create the parallel out of proper rtl.


r~