Re: [Patch] MIPS: Fix PR33479 Broken atomic memory primitives.

[Nigel Stephens <nigel at mips dot com>]

Richard Sandiford wrote:
> Nigel Stephens <nigel@mips.com> writes:
>   
> > Richard Sandiford wrote:
> >     
> > > Nigel Stephens <nigel@mips.com> writes:
> > >       
> > > > Richard Sandiford wrote:
> > > >         
> > > > > Nigel Stephens <nigel@mips.com> writes:
> > > > >           
> > > > > > It is certainly correct that we should not generate branch likely 
> > > > > > instructions by default for ISAs which deprecate them. However on most 
> > > > > > MIPS Technologies CPUs a branch likely incurs no penalty and use of it 
> > > > > > results in faster and smaller code. So while "untuned" mips32/mips64 
> > > > > > code should not contain branch likelies, IMHO the following values of 
> > > > > > mips_tune should enable branch likely:
> > > > > >
> > > > > >   PROCESSOR_4KC,
> > > > > >   PROCESSOR_4KP,
> > > > > >   PROCESSOR_5KC,
> > > > > >   PROCESSOR_5KF,
> > > > > >   PROCESSOR_24KC,
> > > > > >   PROCESSOR_24KF2_1,
> > > > > >   PROCESSOR_24KF1_1,
> > > > > >   PROCESSOR_74KC,
> > > > > >   PROCESSOR_74KF2_1,
> > > > > >   PROCESSOR_74KF1_1,
> > > > > >   PROCESSOR_74KF3_2,
> > > > > >   PROCESSOR_M4K,
> > > > > >     
> > > > > >         
> > > > > >             
> > > > > -mtune options shouldn't override decisions based on -march, so either
> > > > > we should forget about this whole deprecation thing, or we should exclude
> > > > > the above values of -march (rather than -mtune).  The latter option is
> > > > > justified because "-march=4kc" represents a fixed architecture, whereas
> > > > > presumably the point of honouring the deprecation is so that -march=mips32
> > > > > stands more chance of working with all future revisions of the ISA.
> > > > >       
> > > > >           
> > > > Hmm.  I don't think that -mtune is overriding -march. The mip32 
> > > > architecture spec defines the branch-likely instruction, so 
> > > > -march=mips32r2 is allowed to generate a branch-likely -- it's just 
> > > > CPU-dependent whether it is efficient to do so or not, with the default 
> > > > being to assume that it isn't.
> > > >         
> > > That's not how it works though.  The user's selection via -mbranch-likely
> > > always wins of course, but when no such option is given, we have three
> > > levels of checks: we use branch-likely if:
> > >
> > >   (a) the ISA supports it,
> > >   (b) the ISA doesn't deprecate it, and
> > >   (c) they're profitable.
> > >
> > > (with each check syntactically on a different line).  (a) and (b) are
> > > ISA decisions,
> > >       
> > (b) is likely to be downrated in the next revision of the ISA 
> > specification from a deprecation to a recommendation for best 
> > performance in portable code. MIPS supports the selective use of Branch 
> > Likely based on the pipeline being optimized for.
> >     
>
> Ah, interesting, thanks.  In that case I'm even more strongly of the
> opinion that we should just remove the deprecation handling.  We don't
> have x86-like "generic" tunings for -march=mips32, etc (and I'm not sure
> we need to).  


> We instead just pick a specific implementation of the ISA
> and tune for that processor by default. 

I think that's reasonable, but maybe we should reconsider what the 
default processor should be for some of the ISAs. Defaulting to the 74K 
(the most complex pipeline in the list for mips32r2) might result in 
code that runs reasonably better across the full range of 
mips32r2-compliant processors  than tuning for the simple 4K/M4K 
pipeline. Of course we'd need to measure that...


>  The processors chosen for
> -mips32, -mips32r2 and -mips64 are all on your list of targets that
> benefit from branch-likely instructions, but given that the instructions
> seem to be a win for more implementations than not, I think it'd make
> sense to keep those mappings, even with the newly-enabled instructions.
>   

Hmm, I'm not so sure. We'd probably like a generic MIPS32 Linux 
userland, say, to give decent performance across a range of cores. 
Although the strict deprecation may be removed, the performance impact 
of using branch likely on processors which predict branch likely as 
"always taken" would be severe. That's certainly true for the 20K, but 
might be true on other vendors' implementations of the MIPS32 ISA too.

What we did in gcc-3.4 was detach some of these code generation options 
from the processor type used for tuning: the mips_cpu_info table gained 
an extra flag word which allowed you to specify defaults like: 
enable/disable branch likely, hard/soft float, ASEs implemented, etc. 
That way -march=mips32 entry could disable branch-likely, while 
-march=24kc enabled it. Is there any value in adding something like that?

> If implementations of revision 3 ISAs are generally better without,
> we can make -mips32r3 and -mips64r3 map to such implementions.
>
> Does that sound OK (Nigel and others on the list)?  If so, I'll whip
> up a patch.
>
>   
Nigel