[mips] Micromips delay slot fix

Submitted by Steve Ellcey on June 10, 2013, 7:48 p.m.

Details

Message ID 859b5b58-e3c6-44b5-ae35-a3515fa16a00@BAMAIL02.ba.imgtec.org
State New
Headers show

Commit Message

Steve Ellcey June 10, 2013, 7:48 p.m.
We found a bug in the micromips implementation where the branch delay slot
was not getting filled for micromips.  You can reproduce this with a program
that just has an empty main function.  Andrew Bennett created this fix for
it and we would like to check it in.  I am submitting it for Andrew since he
doesn't have write access.

OK to checkin?

2013-06-10  Andrew Bennett <andrew.bennett@imgtec.com>
	    Steve Ellcey  <sellcey@mips.com>

	* config/mips/mips.md (single_insn): Fix attribute for micromips.

Comments

Maciej W. Rozycki June 10, 2013, 10:13 p.m.
On Mon, 10 Jun 2013, Steve Ellcey  wrote:

> We found a bug in the micromips implementation where the branch delay slot
> was not getting filled for micromips.  You can reproduce this with a program
> that just has an empty main function.  Andrew Bennett created this fix for
> it and we would like to check it in.  I am submitting it for Andrew since he
> doesn't have write access.

 Your fix aside shouldn't empty main expand simply to:

        jrc     $ra

?  There's no delay slot there.

> diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
> index 2fdc79d..f18ab50 100644
> --- a/gcc/config/mips/mips.md
> +++ b/gcc/config/mips/mips.md
> @@ -704,7 +704,9 @@
>  
>  ;; Is it a single instruction?
>  (define_attr "single_insn" "no,yes"
> -  (symbol_ref "(get_attr_length (insn) == (TARGET_MIPS16 ? 2 : 4)
> +  (symbol_ref "(((TARGET_MIPS16 || TARGET_MICROMIPS)
> +		 && get_attr_length (insn) == 2)
> +		|| (!TARGET_MIPS16 && get_attr_length (insn) == 4)
>  		? SINGLE_INSN_YES : SINGLE_INSN_NO)"))

 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?

  Maciej
Steve Ellcey June 10, 2013, 11:53 p.m.
On Mon, 2013-06-10 at 23:13 +0100, Maciej W. Rozycki wrote:
> On Mon, 10 Jun 2013, Steve Ellcey  wrote:
> 
> > We found a bug in the micromips implementation where the branch delay slot
> > was not getting filled for micromips.  You can reproduce this with a program
> > that just has an empty main function.  Andrew Bennett created this fix for
> > it and we would like to check it in.  I am submitting it for Andrew since he
> > doesn't have write access.
> 
>  Your fix aside shouldn't empty main expand simply to:
> 
>         jrc     $ra
> 
> ?  There's no delay slot there.

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


> > diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
> > index 2fdc79d..f18ab50 100644
> > --- a/gcc/config/mips/mips.md
> > +++ b/gcc/config/mips/mips.md
> > @@ -704,7 +704,9 @@
> >  
> >  ;; Is it a single instruction?
> >  (define_attr "single_insn" "no,yes"
> > -  (symbol_ref "(get_attr_length (insn) == (TARGET_MIPS16 ? 2 : 4)
> > +  (symbol_ref "(((TARGET_MIPS16 || TARGET_MICROMIPS)
> > +		 && get_attr_length (insn) == 2)
> > +		|| (!TARGET_MIPS16 && get_attr_length (insn) == 4)
> >  		? SINGLE_INSN_YES : SINGLE_INSN_NO)"))
> 
>  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?
> 
>   Maciej

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.

Steve Ellcey
sellcey@mips.com
Maciej W. Rozycki June 11, 2013, 12:50 a.m.
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?

> >  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.

  Maciej

Patch hide | download patch | download mbox

diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index 2fdc79d..f18ab50 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -704,7 +704,9 @@ 
 
 ;; Is it a single instruction?
 (define_attr "single_insn" "no,yes"
-  (symbol_ref "(get_attr_length (insn) == (TARGET_MIPS16 ? 2 : 4)
+  (symbol_ref "(((TARGET_MIPS16 || TARGET_MICROMIPS)
+		 && get_attr_length (insn) == 2)
+		|| (!TARGET_MIPS16 && get_attr_length (insn) == 4)
 		? SINGLE_INSN_YES : SINGLE_INSN_NO)"))
 
 ;; Can the instruction be put into a delay slot?