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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

David Ung

This patch allows the assembler to patch MIPS16 jr/jalr instructions
with their "compact" versions (jrc/jalrc in MIPS16e for
MIPS32/MIPS32R2/MIPS64/MIPS64R2) in the case where a nop would normally
be emitted as a delay slot instruction after the jump.

Tested and passes gcc regressions under mips-sim-idt32/-mips32/-mips16.
Ok for mainline?

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.

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 13 Oct 2005 16:08:05 -0000
*************** append_insn (struct mips_cl_insn *ip, ex
*** 2697,2704 ****
--- 2697,2722 ----
  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.  */
+      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 */
+  ip->insn_opcode |= 0x0080;
+  md_number_to_chars (ip->frag->fr_literal + ip->where,
+      ip->insn_opcode, 2);
+  insert_into_history (0, 1, ip);
+ }
+      else
+ {
   insert_into_history (0, 1, ip);
   emit_nop ();
+ }
+
       if (mips_relax.sequence)
  mips_relax.sizes[mips_relax.sequence - 1] += 4;
     }


Reply | Threaded
Open this post in threaded view
|

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

Richard Sandiford-3
[Insert usual caveat about not being a maintainer ;)]

David Ung <[hidden email]> writes:

>   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.  */
> +      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 */
> +  ip->insn_opcode |= 0x0080;
> +  md_number_to_chars (ip->frag->fr_literal + ip->where,
> +      ip->insn_opcode, 2);
> +  insert_into_history (0, 1, ip);
> + }
> +      else
> + {
>    insert_into_history (0, 1, ip);
>    emit_nop ();
> + }
> +

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
Reply | Threaded
Open this post in threaded view
|

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

Richard Sandiford-3
Richard Sandiford <[hidden email]> writes:
>> +  md_number_to_chars (ip->frag->fr_literal + ip->where,
>> +      ip->insn_opcode, 2);

Oh, and I think this would be better written as:

        install_insn (ip);

Richard
Reply | Threaded
Open this post in threaded view
|

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

David Ung
In reply to this post by Richard Sandiford-3

> 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. */
+  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" }
  }



mips16e-jrc.d (350 bytes) Download Attachment
mips16e-jrc.s (111 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Thiemo Seufer
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
Reply | Threaded
Open this post in threaded view
|

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

David Ung
On Fri, 2005-10-14 at 19:35 +0200, Thiemo Seufer wrote:

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

ok..


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

I'll add "li $2,1" after the jrc instruction, (although one only need to
verify that the first jalrc works, since jrc uses the same code).
patch ok after these change?

David.

Reply | Threaded
Open this post in threaded view
|

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

David Ung
In reply to this post by Thiemo Seufer

latest revised patch attached.
gas regression ok.  ok to comit??

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 19 Oct 2005 12:17:37 -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.  */
+  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 19 Oct 2005 12:17:55 -0000
*************** if { [istarget mips*-*-*] } then {
*** 771,774 ****
--- 771,775 ----
     run_dump_test "mips16-dwarf2-n32"
  }
      }
+     if { !$no_mips16 } { run_dump_test "mips16e-jrc" }
  }

Index: gas/testsuite/gas/mips/mips16e-jrc.d
===================================================================
#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: 6a01       li v0,1
   6: 6500       nop
   8: 6500       nop
   a: 6500       nop
   c: 6500       nop
   e: 6500       nop


Index: gas/testsuite/gas/mips/mips16e-jrc.s
===================================================================
# Test the generation of jalrc/jrc opcodes
        jalr    $31,$2
        jr      $31
        li $2,1

        .p2align 4

Reply | Threaded
Open this post in threaded view
|

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

Thiemo Seufer
David Ung wrote:

>
> latest revised patch attached.
> gas regression ok.  ok to comit??
>
> 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.

Ok.


Thiemo