Re: [patch] MIPS Gas: generating mips16e jrc/jalrc instruction.

[Thiemo Seufer <ths at networkno dot de>]

David Ung wrote:
> 
> > The code looks good, but it orphans the emit_nop() code from the comment
> > that describes it.  I think the comment block should be moved into the
> > "else" as well.
> > 
> > Your comment also doesn't follow FSF conventions (start with a capital
> > letter, end with ".  ".)
> > 
> > IMO, this sort of thing really needs a testcase.
> > 
> > Richard
> > 
> 
> ok, test case attached.  here is the new patch.
> gas regressions ok.
> 
> David.
> 
> 	* config/tc-mips.c (append_insn): Convert MIPS16 jr/jalr jumps
> 	into jrc/jalrc versions if ISA_MIPS32+ and not doing the swap,
> 	hence avoiding to emit a nop.
> 
> 	* gas/mips/mips.exp: Run new test.
> 	* gas/testsuite/gas/mips/mips16e-jrc.s: New test for converting
> 	jalr/jr to the compact jalrc/jrc instructions.
> 	* gas/testsuite/gas/mips/mips16e-jrc.d: New.
> 
> Index: gas/config/tc-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gas/config/tc-mips.c,v
> retrieving revision 1.323
> diff -c -p -b -r1.323 tc-mips.c
> *** gas/config/tc-mips.c	11 Oct 2005 11:16:16 -0000	1.323
> --- gas/config/tc-mips.c	14 Oct 2005 17:23:59 -0000
> *************** append_insn (struct mips_cl_insn *ip, ex
> *** 2693,2704 ****
> --- 2693,2721 ----
>   		 sync.p, we can not swap.  */
>   	      || (prev_pinfo & INSN_SYNC))
>   	    {
> + 	      if (mips_opts.mips16
> + 		  && (pinfo & INSN_UNCOND_BRANCH_DELAY)
> + 		  && (pinfo & (MIPS16_INSN_READ_X | MIPS16_INSN_READ_31))
> + 		  && (mips_opts.isa == ISA_MIPS32
> + 		      || mips_opts.isa == ISA_MIPS32R2
> + 		      || mips_opts.isa == ISA_MIPS64
> + 		      || mips_opts.isa == ISA_MIPS64R2))
> + 		{
> + 		  /* Convert MIPS16 jr/jalr into a "compact" jump. */

The comment formatting rules want two spaces at the end of a sentence.

> + 		  ip->insn_opcode |= 0x0080;
> + 		  install_insn (ip);
> + 		  insert_into_history (0, 1, ip);
> + 		} 
> + 	      else
> + 		{
>   		  /* We could do even better for unconditional branches to
>   		     portions of this object file; we could pick up the
>   		     instruction at the destination, put it in the delay
>   		     slot, and bump the destination address.  */
>   		  insert_into_history (0, 1, ip);
>   		  emit_nop ();
> + 		}
> + 		
>   	      if (mips_relax.sequence)
>   		mips_relax.sizes[mips_relax.sequence - 1] += 4;
>   	    }
> 
> Index: gas/testsuite/gas/mips/mips.exp
> ===================================================================
> RCS file: /cvs/src/src/gas/testsuite/gas/mips/mips.exp,v
> retrieving revision 1.109
> diff -c -p -b -r1.109 mips.exp
> *** gas/testsuite/gas/mips/mips.exp	6 Sep 2005 18:56:21 -0000	1.109
> --- gas/testsuite/gas/mips/mips.exp	14 Oct 2005 17:24:54 -0000
> *************** if { [istarget mips*-*-*] } then {
> *** 771,774 ****
> --- 771,775 ----
>   	    run_dump_test "mips16-dwarf2-n32"
>   	}
>       }
> +     if { !$no_mips16 } { run_dump_test "mips16e-jrc" }
>   }
> 
> 

> #objdump: -dr -mmips:isa32 -mmips:16
> #as: -march=mips32 -mips16
> #name: mips16e jalrc/jrc
> .*:     file format .*
> Disassembly of section .text:
> 00000000 <.text>:
>    0:	eac0      	jalrc	v0
>    2:	e8a0      	jrc	ra
>    4:	6500      	nop
>    6:	6500      	nop
>    8:	6500      	nop
>    a:	6500      	nop
>    c:	6500      	nop
>    e:	6500      	nop

> # Test the generation of jalrc/jrc opcodes
>         jalr    $31,$2
>         jr      $31
> 
>         .p2align 4

Could you expand that to jrc not being followed by a nop? Otherwise it
might not be an effective test.


Thiemo