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

[Richard Sandiford <rsandifo at nildram dot co dot uk>]

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, (c) is a tuning decision.  You're suggesting overriding
(b) based on (c), i.e. overriding an -march decision with an -mtune option.

The deprecation is a general ISA property, not a processor-specific
property.  We're not shirking the instructions because of tuning
decisions (i.e. because we think it's going to produce worse code on
most implementations of these ISAs).  We're shirking them because the
ISA says we should.

>> If there are specific processors that don't benefit from branch-likely
>> instructions, we should add them to the existing !TUNE_SB1 and
>> !TUNE_MIPS5500 checks regardless of whether we keep the deprecation.
>
> Branch likely should not be used for the 20K (and 25K), unless the 
> branch is >~95% likely to be taken.

OK, thanks.  I've no way of testing on those processors.  If you find
that -mno-branch-likely is a win over -mbranch-likely (or, given that
these are your own processors, if you're confident enough that they
will be without the need to run tests), could you propose a patch?

Richard