[mips] Micromips delay slot fix

> -----Original Message-----
> From: Maciej W. Rozycki [mailto:macro@codesourcery.com]
> Sent: Monday, June 10, 2013 8:50 PM
> To: Steve Ellcey; Moore, Catherine
> Cc: Richard Sandiford; gcc-patches@gcc.gnu.org
> Subject: Re: [patch, mips] Micromips delay slot fix
> 
> On Tue, 11 Jun 2013, Steve Ellcey wrote:
> 
> > Sorry, it should have been 'main(void) {return 0; }.  Then you get
> > (with the patch):
> >
> > 	j	$31
> > 	move	$2,$0
> >
> > instead of:
> >
> > 	move	$2,$0
> > 	j	$31
> 
>  Hmm, something must have been missed then from the microMIPS patches
> as our toolchain seems to get this case right:
> 
> 	.set	noreorder
> 	.set	nomacro
> 	jr	$31
> 	move	$2,$0
> 
> 	.set	macro
> 	.set	reorder
> 
> (no idea where the extraneous newline comes from, hmm).  Catherine, can
> you please double-check you haven't got anything outstanding yet?

There isn't anything outstanding.  This is a bug in the mainline implementation due to the introduction of microMIPS instruction length calculations.  In the original, all microMIPS insns had a length of 4.
> 
> > >  Is it safe to assume an RTL insn whose length is 4 has only a
> > > single machine instruction in the microMIPS mode?  What's the
> > > difference to the
> > > MIPS16 instruction set here?
> >
> > I think so, I don't know of any microMIPS RTL instructions whose
> > length is 4 that would not be a single instruction.  Maybe I should
> > add Catherine in to the discussion to see if she knows differently.
> 
>  I advise care here, even if we currently have no such pattern.  It's easy to
> miss the dependency and any such bug introduced could trigger very rarely
> only.  Also (unlike with the standard ISA) some otherwise ordinary
> instructions can't be scheduled into a delay slot, e.g. MOVEP or
> LWP/LDP/SWP/SDP.
> 
There are only a handful of microMIPS-specific instructions.  They are single instructions, but they can't be placed in a delay slot.  These instructions explicitly set the can_delay attribute to "no".

I'm testing a slightly different patch from the one that Steve posted:

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> I'm testing a slightly different patch from the one that Steve posted:
>
> Index: mips.md
> ===================================================================
> --- mips.md     (revision 199648)
> +++ mips.md     (working copy)
> @@ -703,8 +703,13 @@
>
>  ;; Is it a single instruction?
>  (define_attr "single_insn" "no,yes"
> -  (symbol_ref "(get_attr_length (insn) == (TARGET_MIPS16 ? 2 : 4)
> -               ? SINGLE_INSN_YES : SINGLE_INSN_NO)"))
> +  (if_then_else (ior (and (match_test "TARGET_MIPS16")
> +                         (match_test "get_attr_length (insn) == 2"))
> +                    (and (eq_attr "compression" "micromips,all")
> +                         (match_test "TARGET_MICROMIPS"))
> +                    (match_test "get_attr_length (insn) == 4"))
> +               (const_string "yes")
> +               (const_string "no")))

4 isn't OK for MIPS16 though.  There's also the problem that Maciej
pointed out: a length of 4 doesn't imply a single insn on microMIPS.
E.g. an unsplit doubleword move to or from the accumulator registers
is a pair of 2-byte microMIPS instructions, so although its overall
length is 4, it isn't a single insn.  The original code has the same
problem.  In practice, the split should have happened by dbr_schedule
time, but it seems bad practice to rely on that.

(FWIW, the MIPS16 definition comes from the historical attitude that
extended instructions count as 2 instructions.  The *_insns functions
also follow this counting.)

I'm going to try redefining the length attribute after:

	  ;; "Ghost" instructions occupy no space.
	  (eq_attr "type" "ghost")
	  (const_int 0)

in terms of an "insn_count" attribute.  This will conervatively
count 4 for each microMIPS instruction in an unsplit multi-instruction
sequence, just as we do now.  Any attempt to change that should be
a separate patch anyway.

Thanks,
Richard