[PATCH] MIPS/GAS: Disable branch relaxation for BPOSGE32/64

[PATCH] MIPS/GAS: Disable branch relaxation for BPOSGE32/64

[Maciej W. Rozycki]

Hi,

 Here's a change to disable branch relaxation for the BPOSGE32 and 
BPOSGE64 MIPS DSP instructions.  They have no complementing branches and 
therefore cannot be relaxed using our current infrastructure -- an extra 
unconditional branch over the long jump would have to be produced and I 
think there's little demand for such a feature, hence my simple change.  

 Currently trying to assemble an out-of-range branch of this kind yields 
an error like this:

relax-bposge.s:12: Internal error!
Assertion failure in md_convert_frag at [...]/gas/config/tc-mips.c line 17346.
Please report this bug.

 Regression-tested with the mips-sde-elf and mips-gnu-linux targets.  OK 
to apply?

2011-02-21  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c (append_insn): Disable branch relaxation for
	DSP instructions.

	gas/testsuite/
	* gas/mips/relax-bposge.l: New test for DSP branch relaxation.
	* gas/mips/relax-bposge.s: Source for the new test.
	* gas/mips/mips.exp: Run the new test.

  Maciej

binutils-gas-mips-relax-dsp.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2011-02-24 23:46:08.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2011-02-24 23:46:09.000000000 +0000
@@ -3132,6 +3132,8 @@ append_insn (struct mips_cl_insn *ip, ex
 	 out that the branch was out-of-range, we'll get an error.  */
       && !mips_opts.warn_about_macros
       && (mips_opts.at || mips_pic == NO_PIC)
+      /* Don't relax BPOSGE32/64 as they have no complementing branches.  */
+      && !(ip->insn_mo->membership & (INSN_DSP64 | INSN_DSP))
       && !mips_opts.mips16)
     {
       relaxed_branch = TRUE;
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/testsuite/gas/mips/mips.exp	2011-02-24 23:46:08.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/mips.exp	2011-02-24 23:46:09.000000000 +0000
@@ -708,6 +708,8 @@ if { [istarget mips*-*-vxworks*] } {
     run_dump_test "relax-swap1-mips1"
     run_dump_test "relax-swap1-mips2"
     run_dump_test "relax-swap2"
+    run_list_test_arches "relax-bposge" "-mdsp -relax-branch" \
+					[mips_arch_list_matching mips64r2]
 
     run_list_test "illegal" "-32"
     run_list_test "baddata1" "-32"
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-bposge.l
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-bposge.l	2011-02-24 23:46:09.000000000 +0000
@@ -0,0 +1,5 @@
+.*: Assembler messages:
+.*:6: Warning: Relaxed out-of-range branch into a jump
+.*:9: Warning: Relaxed out-of-range branch into a jump
+.*:7: Error: Branch out of range
+.*:8: Error: Branch out of range
Index: binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-bposge.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/testsuite/gas/mips/relax-bposge.s	2011-02-24 23:46:09.000000000 +0000
@@ -0,0 +1,12 @@
+# Source file to test branch relaxation with the BPOSGE32 and BPOSGE64
+# instructions.
+
+	.text
+foo:
+	b	bar
+	bposge32 bar
+	bposge64 bar
+	bal	bar
+
+	.space	0x20000
+bar:

Re: [PATCH] MIPS/GAS: Disable branch relaxation for BPOSGE32/64

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  Here's a change to disable branch relaxation for the BPOSGE32 and 
> BPOSGE64 MIPS DSP instructions.  They have no complementing branches and 
> therefore cannot be relaxed using our current infrastructure -- an extra 
> unconditional branch over the long jump would have to be produced and I 
> think there's little demand for such a feature, hence my simple change.  

Agreed.

> 2011-02-21  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	gas/
> 	* config/tc-mips.c (append_insn): Disable branch relaxation for
> 	DSP instructions.
>
> 	gas/testsuite/
> 	* gas/mips/relax-bposge.l: New test for DSP branch relaxation.
> 	* gas/mips/relax-bposge.s: Source for the new test.
> 	* gas/mips/mips.exp: Run the new test.

OK.

For the record, I wasn't sure at first whether keying off ISA_DSP* was
conceptually a good idea, because there doesn't seem to be anything
inherent in the DSP extensions that would prevent complementing branches
from being defined in DSPr3.  Of course, if that happened, the change is
still conservatively correct.  We'd just lack a potential feature for
those branches.  But I was wondering whether it would be better to have
a separate flag for branches without complements.

I agree that that's probably overkill as things stand though.
If we ever need to use the condition elsewhere, or if more branches
of this kind are added, then we can add a separate flag at that stage.

Thanks,
Richard

Re: [PATCH] MIPS/GAS: Disable branch relaxation for BPOSGE32/64

[Maciej W. Rozycki]

On Sat, 26 Feb 2011, Richard Sandiford wrote:

> > 2011-02-21  Maciej W. Rozycki  <macro@codesourcery.com>
> >
> > 	gas/
> > 	* config/tc-mips.c (append_insn): Disable branch relaxation for
> > 	DSP instructions.
> >
> > 	gas/testsuite/
> > 	* gas/mips/relax-bposge.l: New test for DSP branch relaxation.
> > 	* gas/mips/relax-bposge.s: Source for the new test.
> > 	* gas/mips/mips.exp: Run the new test.
> 
> OK.

 Checked in, thanks.

> For the record, I wasn't sure at first whether keying off ISA_DSP* was
> conceptually a good idea, because there doesn't seem to be anything
> inherent in the DSP extensions that would prevent complementing branches
> from being defined in DSPr3.  Of course, if that happened, the change is
> still conservatively correct.  We'd just lack a potential feature for
> those branches.  But I was wondering whether it would be better to have
> a separate flag for branches without complements.

 I've been somewhat conservative about new flag consumption through the 
course of recent development.  Long-term I think it may make sense to add 
a flag for these branches though, for two reasons.  First is this issue 
and second is the DSP_VOLA flag -- it disables reordering of the affected 
instructions into delay slots of any branches even though only these two 
have a data dependency.  It looks to me two flags will be needed though 
and I don't plan to investigate it any further now.

> I agree that that's probably overkill as things stand though.
> If we ever need to use the condition elsewhere, or if more branches
> of this kind are added, then we can add a separate flag at that stage.

 Indeed, I don't think a specific flag for branches with no complements is 
really needed now (as opposed to one for DSP braches) -- these two are an 
exception rather than the rule and I gather only because of the 
asymmetrical nature of DSP programming specifics.  I wouldn't expect more 
such branches to be added and certainly not outside some future revision 
of the DSP ASE.  And if complements are indeed added, then we can relax 
this restriction.

  Maciej