This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: MN10300: Add LIW and SETLB support


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~


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]