This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
- From: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- To: Andrew Bennett <Andrew dot Bennett at imgtec dot com>, "Maciej W. Rozycki" <macro at codesourcery dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, "Moore, Catherine (Catherine_Moore at mentor dot com)" <Catherine_Moore at mentor dot com>
- Date: Tue, 18 Nov 2014 20:14:59 +0000
- Subject: RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
- Authentication-results: sourceware.org; auth=none
- References: <0DA23CC379F5F945ACB41CF394B9827720F331A8 at LEMAIL01 dot le dot imgtec dot org> <alpine dot DEB dot 1 dot 10 dot 1411181345380 dot 2881 at tp dot orcam dot me dot uk> <0DA23CC379F5F945ACB41CF394B9827720F332BC at LEMAIL01 dot le dot imgtec dot org> <alpine dot DEB dot 1 dot 10 dot 1411181450250 dot 2881 at tp dot orcam dot me dot uk> <0DA23CC379F5F945ACB41CF394B9827720F333C0 at LEMAIL01 dot le dot imgtec dot org> <alpine dot DEB dot 1 dot 10 dot 1411181551000 dot 2881 at tp dot orcam dot me dot uk> <0DA23CC379F5F945ACB41CF394B9827720F3348E at LEMAIL01 dot le dot imgtec dot org>
> > From: Maciej W. Rozycki [mailto:macro@codesourcery.com]
> >
> > On Tue, 18 Nov 2014, Andrew Bennett wrote:
> >
> > > Produces (for the atomic operation):
> > >
> > > .set noat
> > > sync
> > > 1:
> > > ll $3,0($5)
> > > and $1,$3,$4
> > > bne $1,$7,2f
> > > and $1,$3,$6
> > > or $1,$1,$8
> > > sc $1,0($5)
> > > beql $1,$0,1b
> > > nop
> > > nop
> > > sync
> > > 2:
> > > .set at
> >
> > With a quick look at `mips_process_sync_loop' it looks to me the
> > other NOP is produced from here:
> >
> > else if (!(required_oldval && cmp))
> > mips_multi_add_insn ("nop", NULL);
> >
> > -- correct? If so, then can't you just make it:
>
> Correct.
>
> >
> > else if (!(required_oldval && cmp) && !TARGET_FIX_R10000)
> > mips_multi_add_insn ("nop", NULL);
> >
> > instead? It appears plain and simple, and if you're concerned about
> > it being unobvious to the casual reader, then add a one-line comment
> > or suchlike. It's not like TARGET_FIX_R10000 is going to imply
> > anything else about branches in the future (and r6 code won't run on
> > an R10k anyway).
I'm afraid I disagree about it being plain and simple even with a comment.
The amount of code in the backend that makes assumptions based on derived
variables is very high and it makes the code hard to read (especially w.r.t.
branches). I'm OK with adding the code to avoid the extra NOP but have it
based on mips_branch_likely and fix the callers so that it is set
appropriately.
Thanks,
Matthew