Re: [PATCH] MIPS: Fix PR target/39079, __builtin___clear_cache() broken on SMP ISA_HAS_SYNCI systems.

[Richard Sandiford <rdsandiford at googlemail dot com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> David Daney <ddaney@caviumnetworks.com> writes:
>> Richard Sandiford wrote:
>>> David Daney <ddaney@caviumnetworks.com> writes:
>>>>  From the bug:
>>>>
>>>>    We expand __builtin___clear_cache() to a 'synci' instruction on
>>>>    ISA_HAS_SYNCI systems, which invalidates the icache only on the
>>>>    local CPU.
>>>>
>>>>    On an SMP system, the caches on all CPUs should be invalidated.  To
>>>>    achieve this we need to drop back to the old way of doing things by
>>>>    using the cache flush system call.
>>>>
>>>>
>>>> To fix this, I added a new command line option (-msynci/-mno-synci) that 
>>>> enables and disables the generation of the synci instruction for 
>>>> clearing the icache.  The default is to disable synci, but the default 
>>>> can be overridden at configure time with the --with-synci option.
>>> 
>>> Thanks for doing this.  My only real concern is that having an -msynci
>>> option and using it in this way might be confusing.  Most other -m
>>> instruction options control whether the ISA supports an instruction,
>>> whereas here we care about whether an available instruction can be
>>> used in a certain way.
>>> 
>>> What do you think about having an -mmp/-mup pair of options instead?
>>> If the idea's OK, suggestions for better names are welcome.
>>> 
>>
>> There are a several issues.
>>
>> -msynci controls whether or not we emit synci instructions.  -mmp says 
>> something about the memory architecture of the target, although exactly 
>> what is not well defined.
>
> As with all these things, it's up to the documentation to define it ;)
>
>> We could arbitrarily say that with -mmp that synci is not safe.  But 
>> what about the 'sync' that is emitted at the start of all the atomic 
>> memory insns and the memory_barrier insn?  Would it be safe to remove 
>> them under -mup?
>
> Well, it's certainly an interesting possibility (although a logically
> separate change).  I suppose we could even have a tristate
> "single processor, weakly-ordered mp, strongly-ordered mp"
> option.
>
> But your question does highlight the spirit of the suggestion.
> There are other things besides using synci that the compiler might
> conceivably do when optimising for uniprocessor systems.
>
> Another thing at the back of my mind was that many targets have
> built-in functions for things like synci.  It would seem odd to
> prevent any future __builin_mips_synci function from being used
> on MIPS32 or MIPS64 targets, but at the same time, it would seem
> odd to allow the function to be used with -mno-synci.  Having a
> higher-level option name would help avoid that kind of situtation.
>
>> Another issue is interlinking -mmp and -mup code.  Should there be 
>> tagging of the objects and linker support to merge the tags?
>
> I'm not sure whether you're saying that's a problem specific to the
> -mmp/-mup choice, but for avoidance of doubt, I don't think it is.
> The effect on clear_cache will be the same whatever we call the option,
> so the question applies equally to all option names.
>
> And I think the answer is generally: yes, it's good in principle to use
> flags or attributes to mark these things, but it isn't a requirement.
> There are several existing options of this kind that don't have attributes,
> and new attributes can always be added at a later date.
>
> I don't have a strong opinion here.  I just thought it was a question
> worth asking.
>
>> In retrospect it seems like a mistake to have added 
>> __builtin___clear_cache().  Perhaps we should just revert it and have 
>> the only option be multiprocessor safe code.
>
> Dunno about that.  The default should be to support multiprocessor
> systems, but optimisations for uniprocessor systems do seem like
> a nice thing to have.

Well, no-one else has offered an opinion, so it's a straight 1-1 draw.
The patch you posted is OK to commit.

Richard