Re: [PATCH]: R10000 Needs LL/SC Workaround in Gcc

[Richard Sandiford <rdsandiford@googlemail.com>]

Thanks for the new patch.  Generally looks good.

Kumba <kumba@gentoo.org> writes:
> Richard Sandiford wrote:
>> (Which I suppose raises the question: should
>> 
>>   -march=r10000 -mno-branch-likely
>> 
>> be an error, or should it silently disable -mfix-r10000?  My vote is
>> for "error".  You can always write -march=r10000 -mno-branch-likely
>> -mno-fix-r10000 is that's really what you mean.
>> 
>> The suggested change -- swapping these two blocks around -- should do that.)
>
> Well, the check I moved around is only calling sorry(), stating that 
> branch-likely isn't available.  Would additional checking be needed to 
> specifically look for -march=r10000 -mno-branch-likely (and not 
> -mno-fix-r10000), and then raise error() instead, warning the user about the 
> invalid flag combinations

It should be sorry() in all cases.

> (and listing them, so they don't scratch their heads wondering why,
> should such logic be buried deep in a Makefile)?  Or should the sorry
> message be made more verbose? (It looks like it completes part of a
> sentence, so I mimed the grammar I saw in other uses of it).

Well, I suppose the current sorry() is too terse even for an
explicit -mfix-r10000.  How about:

  sorry ("%qs requires branch-likely instructions\n", "-mfix-r10000");

(Done that way so that we can reuse any translations in cases where
we need branch-likely instructions for some other option.)  I think
that's OK even for "-march=r10000 -mno-branch-likely"; the documentation
should make it clear that -mfix-r10000 is the default for -march=r10000.

> Also, what about cases where -march=r12000 is passed?  GCC right now
> has no technical difference between r10k through r16k (they all map to
> r10k for scheduling, per my scheduling patch), but r12k and up should
> have this problem fixed, and thus not need it.  Do we need to go that
> far in addressing obscure combinations of the flags?
>
> Perhaps:
>
> if ((mips_matching_cpu_name_p (mips_arch_info->name, "r12000") ||
>       mips_matching_cpu_name_p (mips_arch_info->name, "r14000") ||
>       mips_matching_cpu_name_p (mips_arch_info->name, "r16000"))
>      && TARGET_FIX_R10000)
> {
>    sorry ("R10000 Errata fix not necessary on R12000 and greater CPUs");
> }

No, there's no need for anything like this.

>> Break lines longer than 80 chars.  Here and elsewhere, it's probably
>> best to use:
>> 
>>   return (mips_output_sync_insn
>>           (...stuff...));
>
> Done.  I used parenthesis on all the return statements, even if they stayed on 
> one line, for consistency.  Any qualms with that?

'Fraid so. ;)  You had those right the first time.

> Some of the lines are just shy of the 80-char limit by 2-4 chars, so
> having the parans there I suppose can preempt any future changes that
> might necessitate a newline + indentation being added.

Whoever gets to make such changes also gets to format the new version
correctly.  There's no need to preempt them by adding redundant parens
(which is against coding conventions).

>> Looks good otherwise, thanks.  We just need to sort out the build
>> problem.
>
> I added to that bug you mentioned (in another mail), as I determined
> that the problem flag is -foptimize-sibling-calls combined with -O1.
> With -O0, it will run just fine, so there's another one or more flags
> enabled by -O1 that are causing the fluke.  No idea if it's
> genautomata itself, since I peeked into SVN and that source file
> hasn't seen any activity in two months.  I figure it must be one of
> the other files that get linked in.

Thanks.  Haven't had time to look at the PR yet.  Will be the weekend
at the earliest now.

> Also, what about a test case?  This is pretty dangerous on Rev 2.5
> R10Ks, potentially locking them up, so I don't know if a testcase
> would be necessary.

A testcase would be nice, yes.  It helps people who are testing on
non-R10K hardware.  It doesn't need to be an execution test though:
just write a scan-assembler test to make sure that all __sync_*()
builtins use branch-likely instructions.  See other gcc.target/mips
tests for inspiration.

> +  /* Make sure that branch-likely instructions available when using
> +     -mfix-r10000.  The instructions are not available if either:

Minor nit, but I think the comment is easier to read if we keep the
suggested blank line here.

> +	1. -mno-branch-likely was passed.
> +	2. The selected ISA does not support branch-likely and
> +	   the command line does not include -mbranch-likely  */

You lost the "." at the end of the comment (coding conventions require it).

> diff -Naurp -x .svn gcc.orig/gcc/doc/invoke.texi gcc/gcc/doc/invoke.texi
> --- gcc.orig/gcc/doc/invoke.texi	2008-10-30 22:14:29.000000000 -0400
> +++ gcc/gcc/doc/invoke.texi	2008-11-11 22:38:37.000000000 -0500
> @@ -666,7 +666,7 @@ Objective-C and Objective-C++ Dialects}.
>  -mdivide-traps  -mdivide-breaks @gol
>  -mmemcpy  -mno-memcpy  -mlong-calls  -mno-long-calls @gol
>  -mmad  -mno-mad  -mfused-madd  -mno-fused-madd  -nocpp @gol
> --mfix-r4000  -mno-fix-r4000  -mfix-r4400  -mno-fix-r4400 @gol
> +-mfix-r4000  -mno-fix-r4000  -mfix-r4400  -mno-fix-r4400 -mfix-r10000 -mno-fix-r10000 @gol
>  -mfix-vr4120  -mno-fix-vr4120  -mfix-vr4130  -mno-fix-vr4130 @gol
>  -mfix-sb1  -mno-fix-sb1 @gol
>  -mflush-func=@var{func}  -mno-flush-func @gol

This is preformatted text, so the new line is too long.  Push it onto the
next, and push the -m{no-,}-fix-vr4130 options down to the following line.

You need to document what -mfix-r10000 does as well. ;)  See the other
-mfix-* options for the general idea.

Richard