On Tue, 18 Nov 2014, Matthew Fortune wrote:
> > > 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.
Sure, fine with me too. It looks to me it'll be more natural even to
have `mips_branch_likely' preset on entering `mips_process_sync_loop'.
Maciej
