: R10000 Needs LL/SC Workaround in Gcc
PATCH 1/1 : R10000 Needs LL/SC Workaround in Gcc
The attached patch adds a workaround to have gcc emit branch likely instructions (beqzl) in atomic operations for R10000 CPUs. This is because revisions of this CPU before 3.0 misbehave, while revisions 2.6 and earlier will deadlock. This issue has been noted on SGI IP28 (Indigo2 Impact R10000) systems and SGI IP27 Origin systems.
After creating a patch to glibc based off of Debian Bug #462112 (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462112), it was suggested by David Daney that a similar patch be created for GCC.
Feedback would be welcome on any suggestions for improving this patch (please CC, as I'm not subscribed to the ML).
Thanks!
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
gcc/config/mips/mips.h +38 -13
@@ -3066,6 +3066,31 @@ while (0)#ifndef HAVE_AS_TLS #define HAVE_AS_TLS 0#endif + +/* Certain revisions of the R10000 Processor need an LL/SC Workaround +enabled. Revisions before 3.0 misbehave on atomic operations, and +Revs 2.6 and lower deadlock after several seconds due to other errata. + +To quote the R10K Errata: +Workaround: The basic idea is to inhibit the four instructions +from simultaneously becoming active in R10000. Padding all +ll/sc sequences with nops or changing the looping branch in the +routines to a branch likely (which is always predicted taken +by R10000) will work. The nops should go after the loop, and the +number of them should be 28. This number could be decremented for +each additional instruction in the ll/sc loop such as the lock +modifier(s) between the ll and sc, the looping branch and its +delay slot. For typical short routines with one ll/sc loop, any +instructions after the loop could also count as a decrement. The +nop workaround pollutes the cache more but would be a few cycles +faster if all the code is in the cache and the looping branch +is predicted not taken. */ + +#ifndef (_MIPS_ARCH_R10000) +#define R10K_BEQZ_INSN "\tbeq\t%@,%.,1b\n" +#else +#define R10K_BEQZ_INSN "\tbeqzl\t%@,1b\n" +#endif /* Return an asm string that atomically:
@@ -3083,7 +3108,7 @@ while (0)"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3108,7 +3133,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3128,7 +3153,7 @@ while (0)"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3153,7 +3178,7 @@ while (0)"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3186,7 +3211,7 @@ while (0)"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3216,7 +3241,7 @@ while (0)"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3236,7 +3261,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3253,7 +3278,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3270,7 +3295,7 @@ while (0)"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3289,7 +3314,7 @@ while (0)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3308,7 +3333,7 @@ while (0)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3326,7 +3351,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3350,7 +3375,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
Quoted 7 lines · Kumba
The attached patch adds a workaround to have gcc emit branch likely instructions (beqzl) in atomic operations for R10000 CPUs. This is because revisions of this CPU before 3.0 misbehave, while revisions 2.6 and earlier will deadlock. This issue has been noted on SGI IP28 (Indigo2 Impact R10000) systems and SGI IP27 Origin systems.
After creating a patch to glibc based off of Debian Bug #462112 (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462112), it was suggested by David Daney that a similar patch be created for GCC.
Feedback would be welcome on any suggestions for improving this patch (please CC, as I'm not subscribed to the ML).
Thanks!
Oops, typo in my first patch. Stray parenthesis around the macro check. Fixed patch is included.
I'm wondering whether this should be limited to _MIPS_ARCH_R10000, though. Maybe _MIPS_ARCH_MIPS4 instead, because the R10000 is at minimum, a MIPS-IV CPU,
and there might be cases where a userland compiled with -march=mips4 could get
used instead of one optimized for -march=r10000?
Or would MIPS-II be better, which is when the branch likely instruction was added?
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
gcc/config/mips/mips.h +38 -13
@@ -3066,6 +3066,31 @@ while (0)#ifndef HAVE_AS_TLS #define HAVE_AS_TLS 0#endif + +/* Certain revisions of the R10000 Processor need an LL/SC Workaround +enabled. Revisions before 3.0 misbehave on atomic operations, and +Revs 2.6 and lower deadlock after several seconds due to other errata. + +To quote the R10K Errata: +Workaround: The basic idea is to inhibit the four instructions +from simultaneously becoming active in R10000. Padding all +ll/sc sequences with nops or changing the looping branch in the +routines to a branch likely (which is always predicted taken +by R10000) will work. The nops should go after the loop, and the +number of them should be 28. This number could be decremented for +each additional instruction in the ll/sc loop such as the lock +modifier(s) between the ll and sc, the looping branch and its +delay slot. For typical short routines with one ll/sc loop, any +instructions after the loop could also count as a decrement. The +nop workaround pollutes the cache more but would be a few cycles +faster if all the code is in the cache and the looping branch +is predicted not taken. */ + +#ifndef _MIPS_ARCH_R10000 +#define R10K_BEQZ_INSN "\tbeq\t%@,%.,1b\n" +#else +#define R10K_BEQZ_INSN "\tbeqzl\t%@,1b\n" +#endif /* Return an asm string that atomically:
@@ -3083,7 +3108,7 @@ while (0)"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3108,7 +3133,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3128,7 +3153,7 @@ while (0)"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3153,7 +3178,7 @@ while (0)"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3186,7 +3211,7 @@ while (0)"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3216,7 +3241,7 @@ while (0)"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3236,7 +3261,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3253,7 +3278,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3270,7 +3295,7 @@ while (0)"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3289,7 +3314,7 @@ while (0)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3308,7 +3333,7 @@ while (0)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3326,7 +3351,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3350,7 +3375,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +R10K_BEQZ_INSN \"\tnop\n" \"\tsync%-%]%>%)"
Quoted 19 lines · Kumba
Quoted 6 lines · Kumba
The attached patch adds a workaround to have gcc emit branch likely instructions (beqzl) in atomic operations for R10000 CPUs. This is because revisions of this CPU before 3.0 misbehave, while revisions 2.6 and earlier will deadlock. This issue has been noted on SGI IP28 (Indigo2 Impact R10000) systems and SGI IP27 Origin systems.
Quoted 8 lines
After creating a patch to glibc based off of Debian Bug #462112
(http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462112), it was suggested by David Daney that a similar patch be created for GCC.Feedback would be welcome on any suggestions for improving this patch (please CC, as I'm not subscribed to the ML).
Thanks!
Oops, typo in my first patch. Stray parenthesis around the macro check. Fixed patch is included.
Quoted 8 lines
I'm wondering whether this should be limited to
_MIPS_ARCH_R10000, though. Maybe_MIPS_ARCH_MIPS4instead, because the R10000 is at minimum, a MIPS-IV CPU, and there might be cases where a userland compiled with -march=mips4 could get used instead of one optimized for -march=r10000?Or would MIPS-II be better, which is when the branch likely instruction was added?
As Maciej said, this should really be controlled by an -mfix-r10000 command-line option, not by the _MIPS_ARCH_* macro. (In this context, _MIPS_ARCH_* is a property of the compiler that you're using to build gcc itself.)
There are two ways we could handle this:
- Make -mfix-r10000 require -mbranch-likely. (It mustn't _imply_ -mbranch-likely. It should simply check that -mbranch-likely is already in effect.)
- Make -mfix-r10000 insert nops when -mbranch-likely is not in effect.
Richard
Quoted 6 lines · Richard Sandiford
As Maciej said, this should really be controlled by an -mfix-r10000 command-line option, not by the _MIPS_ARCH_* macro. (In this context, _MIPS_ARCH_* is a property of the compiler that you're using to build gcc itself.)
Quoted 7 lines
There are two ways we could handle this:
- Make -mfix-r10000 require -mbranch-likely. (It mustn't _imply_ -mbranch-likely. It should simply check that -mbranch-likely is already in effect.)
- Make -mfix-r10000 insert nops when -mbranch-likely is not in effect.
Does using -mbranch-likely change the output of those specific asm commands that my original patch was altering? Or will -mfix-r10000 need to not only check the status of -mbranch-likely and set it if not set, but also need to modify the referenced beq/beqzl sets in mips.h?
If so, I assume a test for both TARGET_FIX_R10000 and TARGET_BRANCHLIKELY would be needed, and then if TARGET_BRANCHLIKELY doesn't exist, but TARGET_FIX_R10000 is, insert 28 nops before beq. Sound correct?
On setting -mbranch-likely, I found what I think is the appropriate section in mips.c around Line 13810:
/* If neither -mbranch-likely nor -mno-branch-likely was given
on the command line, set MASK_BRANCHLIKELY based on the target architecture and tuning flags. Annulled delay slots are a size win, so we only consider the processor-specific tuning for !optimize_size. */
if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
{
if (ISA_HAS_BRANCHLIKELY
&& (optimize_size
|| (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0))
target_flags |= MASK_BRANCHLIKELY;
else
target_flags &= ~MASK_BRANCHLIKELY;
} else if (TARGET_BRANCHLIKELY && !ISA_HAS_BRANCHLIKELY)
warning (0, "the %qs architecture does not support branch-likely"
" instructions", mips_arch_info->name);
I'm kind of thinking that the -mfix-r10000 setting to include -mbranch-likely would fit here (Assuming this is what can enable/disable that option via MASK_BRANCHLIKELY), but if I'm reading it right, optimizing for size disables brach-likely instructions. Shouldn't -mfix-r10000 override that?
Would an equivalent conditional like this be close?:
if (ISA_HAS_BRANCHLIKELY
&& ((optimize_size || TARGET_FIX_R10000)
|| (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0))Also, does anyone have a copy of the R10000 Silicon Errata documentation kicking around? Thiemo brought up a point that we may need ssnop instead of nop, but I'd need to check the errata for that, and that doesn't seem to exist anywhere anymore. I found an old link to it on MIPS' site, but nothing else. I've only got Vr10000 manuals from SGI and NEC, and they don't seem to cover revision-specific errata any.
Thanks!
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
Quoted 9 lines · Kumba
Quoted 8 lines · Richard Sandiford
As Maciej said, this should really be controlled by an -mfix-r10000 command-line option, not by the _MIPS_ARCH_* macro. (In this context, _MIPS_ARCH_* is a property of the compiler that you're using to build gcc itself.)
There are two ways we could handle this:
Quoted 8 lines
Quoted 5 lines
- Make -mfix-r10000 require -mbranch-likely. (It mustn't _imply_ -mbranch-likely. It should simply check that -mbranch-likely is already in effect.)
- Make -mfix-r10000 insert nops when -mbranch-likely is not in effect.
Does using -mbranch-likely change the output of those specific asm commands that my original patch was altering?
No. In current sources, the asm templates never use branch-likely instructions.
Quoted 3 lines
Or will -mfix-r10000 need to not only check the status of -mbranch-likely and set it if not set, but also need to modify the referenced beq/beqzl sets in
mips.h?
To be clear, the first option above was to check -- in mips_override_options -- that -mfix-r10000 is only used in cases where -mbranch-likely is in effect. If we pick that option, it would be an error to use -mfix-r10000 in other cases, and any code protected by TARGET_FIX_R10000 would be free to use branch-likely instructions. (Actually, we should use sorry() instead of error() to report something like this.)
Quoted 4 lines
If so, I assume a test for both
TARGET_FIX_R10000andTARGET_BRANCHLIKELYwould be needed, and then ifTARGET_BRANCHLIKELYdoesn't exist, butTARGET_FIX_R10000is, insert 28 nops before beq. Sound correct?
That's the second option above, yes. In other words, -mfix-r10000 would support both -mbranch-likely and -mno-branch-likely, and act accordingly.
Quoted 21 lines
On setting -mbranch-likely, I found what I think is the appropriate section in
mips.caround Line 13810:/* If neither -mbranch-likely nor -mno-branch-likely was given on the command line, set MASK_BRANCHLIKELY based on the target architecture and tuning flags. Annulled delay slots are a size win, so we only consider the processor-specific tuning for !optimize_size. */ if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0) { if (ISA_HAS_BRANCHLIKELY && (optimize_size || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0)) target_flags |= MASK_BRANCHLIKELY; else target_flags &= ~MASK_BRANCHLIKELY; } else if (TARGET_BRANCHLIKELY && !ISA_HAS_BRANCHLIKELY) warning (0, "the %qs architecture does not support branch-likely" " instructions", mips_arch_info->name);
Quoted 4 lines
I'm kind of thinking that the -mfix-r10000 setting to include -mbranch-likely would fit here (Assuming this is what can enable/disable that option via
MASK_BRANCHLIKELY), but if I'm reading it right, optimizing for size disables brach-likely instructions.
Well, optimize_size _enables_ branch-likely, but...
Quoted 1 line
Shouldn't -mfix-r10000 override that?
...that's a good question. My take is "no". I don't think we want -mfix-r10000 to enable branch-likely instructions in cases where it isn't necessary for R10000 errata. If we take the first option, we can simply raise an error if:
TARGET_FIX_R10000
&& (target_flags_explicit & MASK_BRANCHLIKELY) == 0
? !ISA_HAS_BRANCH_LIKELY
? !TARGET_BRANCH_LIKELY)
Quoted 7 lines
Also, does anyone have a copy of the R10000 Silicon Errata documentation kicking around? Thiemo brought up a point that we may need ssnop instead of nop, but I'd need to check the errata for that, and that doesn't seem to exist anywhere anymore. I found an old link to it on MIPS' site, but nothing else. I've only got Vr10000 manuals from SGI and NEC, and they don't seem to cover revision-specific errata any.
Yeah, I was wondering that too. I did a search, but couldn't find anything.
Richard
Quoted 7 lines · Richard Sandiford
To be clear, the first option above was to check -- in
mips_override_options-- that -mfix-r10000 is only used in cases where -mbranch-likely is in effect. If we pick that option, it would be an error to use -mfix-r10000 in other cases, and any code protected byTARGET_FIX_R10000would be free to use branch-likely instructions. (Actually, we should usesorry()instead oferror()to report something like this.)
[snip]
Quoted 3 lines
That's the second option above, yes. In other words, -mfix-r10000 would support both -mbranch-likely and -mno-branch-likely, and act accordingly.
So do I need to worry about modifying the asm templates at all? Or is that only needed if pursuing option #2?
The branch-likely stuff is only going to work for MIPS-II or higher targets. In the odd (but possible) cases where MIPS-I might be used with -mfix-r10000, I assume we'll still have to emit 28 nops prior to a beq/beqz instruction. Is this already taken care of someplace?
Quoted 4 lines
...that's a good question. My take is "no". I don't think we want -mfix-r10000 to enable branch-likely instructions in cases where it isn't necessary for R10000 errata. If we take the first option, we can simply raise an error if:
Hmm, okay. Might this work to enable -mbranch-likely if -mfix-r10000? (Kind of guessing by looking at other segments of code).
if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
{
if (ISA_HAS_BRANCHLIKELY
&& (optimize_size
|| (!(target_flags_explicit & MASK_FIX_R10000) == 0)
|| (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0))
target_flags |= MASK_BRANCHLIKELY;
else
target_flags &= ~MASK_BRANCHLIKELY;
}My understanding so far for -mfix-r10000:
- Gets enabled if -march=r10000 is passed (done)
- Enable -mbranch-likely if not already enabled on >= MIPS-II (working on)
- Emits beqzl in the asm templates if enabled and >= MIPS-II (unsure)
- Emits 28 nops prior to beq/beqz if enabled and == MIPS-I (unsure)
- Ditto for asm templates (unsure)
- Documentation (not done)
Missing anything?
Quoted 2 lines
Yeah, I was wondering that too. I did a search, but couldn't find anything.
It seems we just need to use nop only and not worry about ssnop.
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
Quoted 9 lines · Kumba
Quoted 7 lines · Richard Sandiford
To be clear, the first option above was to check -- in
mips_override_options-- that -mfix-r10000 is only used in cases where -mbranch-likely is in effect. If we pick that option, it would be an error to use -mfix-r10000 in other cases, and any code protected byTARGET_FIX_R10000would be free to use branch-likely instructions. (Actually, we should usesorry()instead oferror()to report something like this.)
Quoted 8 lines
[snip]
Quoted 3 lines
That's the second option above, yes. In other words, -mfix-r10000 would support both -mbranch-likely and -mno-branch-likely, and act accordingly.
So do I need to worry about modifying the asm templates at all? Or is that only needed if pursuing option #2?
You need to modify the asm templates whatever you do.
Quoted 4 lines
The branch-likely stuff is only going to work for MIPS-II or higher targets. In the odd (but possible) cases where MIPS-I might be used with -mfix-r10000, I assume we'll still have to emit 28 nops prior to a beq/beqz instruction. Is this already taken care of someplace?
Hmm? No, option #2 was supposed to include this work.
I feel we're talking at cross-purposes, so just to be clear:
- In current gcc sources, the code generated by the __sync*() functions is not suitable for R10K processors. That's true for all current command-line options. We therefore need to add a new command-line option (-mfix-r10000) that selects the required behaviour.
- As you said in your original message, there are two workarounds for the R10K errata:
a) Make the asm templates use branch-likely instructions instead of normal branches when -mfix-r10000 is in effect.
b) Separate loops by at least 28 instructions when -mfix-r10000 is in effect.
Both workarounds require work, because gcc does neither of these things at present. However, (a) is much easier to implement than (b).
So it seems to me that there are two possible ways of implementing the -mfix-r10000 option:
1) Only do (a). Make it an error to use -mfix-r10000 when branch-likely instructions are not available. We should check for this error in mips_override_options and use sorry() rather than error() to report it.
Branch-likely instructions are not available if either:
i) the command line includes -mno-branch-likely; or ii) both of the following hold:
- the selected architecture does not provide branch-likely instructions; and
- the command line does not include -mbranch-likely.
The C condition for this is the one I gave in my previous message:
(target_flags_explicit & MASK_BRANCHLIKELY) == 0
? !ISA_HAS_BRANCH_LIKELY
? !TARGET_BRANCH_LIKELY)
So we should give up and call sorry() if:
TARGET_FIX_R10000
&& (target_flags_explicit & MASK_BRANCHLIKELY) == 0
? !ISA_HAS_BRANCHLIKELY
? !TARGET_BRANCHLIKELY)
is true.
If we take this approach, any gcc code guarded by TARGET_FIX_R10000 can make free use of branch-likely instructions. No separate *_BRANCHLIKELY condition would be needed.
In other words, the asm template could always use branch-likely instructions when TARGET_FIX_R10000 is true, and always use the current branch sequences otherwise.
2) Implement both (a) and (b). In this case, any gcc code guarded by TARGET_FIX_R10000 would need to check whether branch-likely instructions are available. If they are, we can use either workaround (a) or workaroudn (b). If they aren't, we must use workaround (b).
These two options correspond to the two in my original reply.
Quoted 8 lines
Quoted 4 lines
...that's a good question. My take is "no". I don't think we want -mfix-r10000 to enable branch-likely instructions in cases where it isn't necessary for R10000 errata. If we take the first option, we can simply raise an error if:
Hmm, okay. Might this work to enable -mbranch-likely if -mfix-r10000? (Kind of guessing by looking at other segments of code).
Quoted 10 lines
if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
{
if (ISA_HAS_BRANCHLIKELY
&& (optimize_size
|| (!(target_flags_explicit & MASK_FIX_R10000) == 0)
|| (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0))
target_flags |= MASK_BRANCHLIKELY;
else
target_flags &= ~MASK_BRANCHLIKELY;
}No. What I was trying to say in the quoted text was: -mfix-r10000 should have _no_ effect on whether branch-likely instructions are available for general use.
As the comment above this code says:
/* If neither -mbranch-likely nor -mno-branch-likely was given
on the command line, set MASK_BRANCHLIKELY based on the target architecture and tuning flags. Annulled delay slots are a size win, so we only consider the processor-specific tuning for !optimize_size. */
In other words, this bit of the condition:
(optimize_size
|| (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0)is a _tuning_ decision. It is used in cases where both TARGET_BRANCHLIKELY and !TARGET_BRANCH_LIKELY would be OK from a correctness standpoint.
So suppose we only implement workaround (a) (== option (1) above). We now need branch-likely instructions _for correctness_, but only in certain asm templates. It's therefore OK to override the _tuning_ decision for those asm templates: correctness trumps tuning. But we shouldn't change the tuning decision for _other_ branches (i.e. for branches where -mfix-r10000 does not require a branch-likely instruction).
Quoted 2 lines
My understanding so far for -mfix-r10000:
- Gets enabled if -march=r10000 is passed (done)
Yes, provided that you never override an explicit -mfix-r10000 or -mno-fix-r10000.
Quoted 4 lines
Quoted 2 lines
Yeah, I was wondering that too. I did a search, but couldn't find anything.
It seems we just need to use nop only and not worry about ssnop.
Actually, I meant: I was wondering about the fact that there seems to be no online copy of the errata sheet that describes this problem. I've only ever seen a description of the workaround. I've never seen a verbatim copy of the errata itself.
Richard
Richard Sandiford wrote:
(Quoting out of order)
Quoted 2 lines
I feel we're talking at cross-purposes, so just to be clear:
Not really cross-purposes. I'm just rather new to patching something big like a full-blown, multi-target compiler with 20 years of history to it. Mostly trying to get an appropriate understanding of your two options so I can work the logic out in my head and know how to go about attacking this.
This is a step up from writing a md file for processor scheduling :)
Quoted 5 lines
2) Implement both (a) and (b). In this case, any gcc code guarded by
TARGET_FIX_R10000would need to check whether branch-likely instructions are available. If they are, we can use either workaround (a) or workaroudn (b). If they aren't, we must use workaround (b).
I think it's better to target this path. While it's probably an extremely rare case, because this problem only affects a specific set of processor revisions, triggering a problem only noticed (so far) on SGI machines running Linux, I tend to err on the side of caution and think it's probably a good idea to do it right the first time.
Also, Murphy's Law.
Quoted 1 line
You need to modify the asm templates whatever you do.
This is what has me a little perplexed. The asm templates are #define macros, and it's kind of dawned on me that my attempts made so far to correct the errata has me using preprocessor macros that are going to get translated into something else when gcc itself is compiled, rather than gcc changing what it outputs based on the flags we send it.
So I'm assuming that, poking around in the sync.md file some, the better approach might be to pass an extra argument to these atomic macros as they're evaluated in sync.md. This extra argument being the resultant branch likely instruction:
- If -mfix-r10000 isn't needed or -mbranch-likely isn't called, "beq" gets passed in.
- If -mfix-r10000 is called, and ISA_HAS_BRANCHLIKELY is false, pass in 28 nops plus "beq" (is there some kind of macro that can expand a single nop 28 times?).
- If -mfix-r10000 is called and ISA_HAS_BRANCHLIKELY is true and -mno-branch-likely was not called, then pass in the beqzl instruction.
I think that's all the relevant combinations. It's also probably a good idea too to determine the value to pass as the extra argument before the atomic macro is called.
Is this kind of check something that would need to be done in the md file directly, referencing the various macros as needed, or would it need to be defined as a function in mips.c and called inline in sync.md, returning a string value to the function as it exists? Or is there a better way?
Quoted 2 lines
Yes, provided that you never override an explicit -mfix-r10000 or -mno-fix-r10000.
I copied the same code for R4000 and R4400 for this:
/* Default to working around R10000 errata only if the processor
was selected explicitly. */
if ((target_flags_explicit & MASK_FIX_R10000) == 0
&& mips_matching_cpu_name_p (mips_arch_info->name, "r10000"))
target_flags |= MASK_FIX_R10000;I assume that won't fire on r12000/r14000/r16000, right? I know R14K isn't affected, but I haven't tried plugging my old R12K module back into the system to see what it does. R16K is likely safe.
Quoted 4 lines
Actually, I meant: I was wondering about the fact that there seems to be no online copy of the errata sheet that describes this problem. I've only ever seen a description of the workaround. I've never seen a verbatim copy of the errata itself.
I tried seeing whether archive.org had anything old off of the mips.com site, but nothing close to the old directory structure seems to exist. If I new what the PDF file name was, it might be possible to track something down on Google pertaining to the last publicly released revision. Bit surprised, too, on why NEC doesn't have anything on necel.com. They produced the actual silicon and had a hand in designing it, if I'm not mistaken. I'd think they would at least have a copy if no one else.
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
Quoted 11 lines · Kumba
Quoted 5 lines
2) Implement both (a) and (b). In this case, any gcc code guarded by
TARGET_FIX_R10000would need to check whether branch-likely instructions are available. If they are, we can use either workaround (a) or workaroudn (b). If they aren't, we must use workaround (b).I think it's better to target this path. While it's probably an extremely rare case, because this problem only affects a specific set of processor revisions, triggering a problem only noticed (so far) on SGI machines running Linux, I tend to err on the side of caution and think it's probably a good idea to do it right the first time.
Agreed, but that's just as true of option 1. Each option is as correct as the other. It's just a question of whether we need the combination:
-mips1 -mllsc -mfix-r10000
to be accepted, or whether we can treat it as a compile-time error.
If you do go for option 2, you then have to decide whether to insert 28 nops after every LL/SC loop, or whether you try to do some analysis to avoid unnecessary nops. The natural place for this analysis would be mips_avoid_hazard.
Quoted 22 lines
Quoted 1 line
You need to modify the asm templates whatever you do.
This is what has me a little perplexed. The asm templates are #define macros, and it's kind of dawned on me that my attempts made so far to correct the errata has me using preprocessor macros that are going to get translated into something else when gcc itself is compiled, rather than gcc changing what it outputs based on the flags we send it.
So I'm assuming that, poking around in the sync.md file some, the better approach might be to pass an extra argument to these atomic macros as they're evaluated in sync.md. This extra argument being the resultant branch likely instruction:
- If -mfix-r10000 isn't needed or -mbranch-likely isn't called, "beq" gets passed in.
- If -mfix-r10000 is called, andISA_HAS_BRANCHLIKELYis false, pass in 28 nops plus "beq" (is there some kind of macro that can expand a single nop 28 times?).
- If -mfix-r10000 is called andISA_HAS_BRANCHLIKELYis true and -mno-branch-likely was not called, then pass in the beqzl instruction.
Quoted 3 lines
I think that's all the relevant combinations. It's also probably a good idea too to determine the value to pass as the extra argument before the atomic macro is called.
If you go for option 1, you could replace things like:
"\tbeq\t%@,%.,1b\n" \ "\tnop\n" \
with:
"\tbeq%?\t%@,%.,1b\n" \ "\tnop\n" \
and make the .md insn do:
mips_branch_likely = TARGET_FIX_R10000;
But something nattier is needed for MIPS_SYNC_NEW_OP and MIPS_SYNC_NEW_NAND, where the branch delay slot is not a nop. In this case, we need to replace things like:
"\tbeq\t%@,%.,1b\n" \ "\t" INSN "\t%0,%0,%2\n" \
with:
"\tbeql\t%@,%.,1b\n" \ "\tnop\n" \ "\t" INSN "\t%0,%0,%2\n" \
(INSN does not need to be executed when the branch is taken.)
I suppose adding a macro parameter would work, but it would lead to combinatorial explosion. I think we need to replace these MIPS_SYNC_* macros with a function that uses output_asm_insn to emit the loop insn-by-insn. (This might make it possible to factor things more than they're factored now.)
Quoted 10 lines
Quoted 2 lines
Yes, provided that you never override an explicit -mfix-r10000 or -mno-fix-r10000.
I copied the same code for R4000 and R4400 for this:
/* Default to working around R10000 errata only if the processor was selected explicitly. */ if ((target_flags_explicit & MASK_FIX_R10000) == 0 && mips_matching_cpu_name_p (mips_arch_info->name, "r10000")) target_flags |= MASK_FIX_R10000;
Looks good.
Quoted 1 line
I assume that won't fire on r12000/r14000/r16000, right?
Right.
Quoted 5 lines
Quoted 4 lines
Actually, I meant: I was wondering about the fact that there seems to be no online copy of the errata sheet that describes this problem. I've only ever seen a description of the workaround. I've never seen a verbatim copy of the errata itself.
Quoted 8 lines
I tried seeing whether archive.org had anything old off of the mips.com site, but nothing close to the old directory structure seems to exist. If I new what the PDF file name was, it might be possible to track something down on Google pertaining to the last publicly released revision. Bit surprised, too, on why NEC doesn't have anything on necel.com. They produced the actual silicon and had a hand in designing it, if I'm not mistaken. I'd think they would at least have a copy if no one else.
Yeah. Maybe they just want to erase bad memories ;)
Richard
Quoted 14 lines · Richard Sandiford
Quoted 5 lines
Quoted 4 lines
Actually, I meant: I was wondering about the fact that there seems to be no online copy of the errata sheet that describes this problem. I've only ever seen a description of the workaround. I've never seen a verbatim copy of the errata itself.
Quoted 8 lines
I tried seeing whether archive.org had anything old off of the mips.com site, but nothing close to the old directory structure seems to exist. If I new what the PDF file name was, it might be possible to track something down on Google pertaining to the last publicly released revision. Bit surprised, too, on why NEC doesn't have anything on necel.com. They produced the actual silicon and had a hand in designing it, if I'm not mistaken. I'd think they would at least have a copy if no one else.
Quoted 1 line
Yeah. Maybe they just want to erase bad memories ;)
The R10000 processor is not a MIPS Technologies but SGI product. So you've been looking at the wrong site. However to shorten your search, only some of the relativly early versions of the errata sheets were published.
Ralf
Quoted 7 lines · Richard Sandiford
Agreed, but that's just as true of option 1. Each option is as correct as the other. It's just a question of whether we need the combination:
-mips1 -mllsc -mfix-r10000
to be accepted, or whether we can treat it as a compile-time error.
Hmm, which do you think makes sense? From a usage perspective, most developers are working in the MIPS32/MIPS64 ISA stuff. In Gentoo, the mips port mostly supports SGI systems, mostly anything with a MIPS-IV capable processor (haven't decided on MIPS-III's fate just yet). Debian I know has switched off of MIPS-I being the default for their binaries, and I think is MIPS-II now. In all these cases, the target OS is usually Linux, although I know there are some Irix folks still out there, plus the *BSDs all got their own ports.
But I guess the question I'm pondering, is just how rare would it be for someone to actually need a MIPS-I binary with ll/sc and branch-likely fixes to run on something like an R10000? Rare enough to justify denying them that particular command argument combination, and thus taking Option #1? Or go the extra mile for Option #2? I don't know if that's my call to really make, since I lack the statistical data to know who would be affected, and in what ways (i.e., do they have alternative methods, such as MIPS-II, etc..).
Quoted 4 lines
If you do go for option 2, you then have to decide whether to insert 28 nops after every LL/SC loop, or whether you try to do some analysis to avoid unnecessary nops. The natural place for this analysis would be
mips_avoid_hazard.
Hmm, just looking at this out of curiosity to get an idea of what I might have to tackle, but this particular sequence looks like the key:
/* Work out how many nops are needed. Note that we only care about
registers that are explicitly mentioned in the instruction's pattern.
It doesn't matter that calls use the argument registers or that they
clobber hi and lo. */
if (*hilo_delay < 2 && reg_set_p (lo_reg, pattern))
nops = 2 - *hilo_delay;
else if (*delayed_reg != 0 && reg_referenced_p (*delayed_reg, pattern))
nops = 1;
else
nops = 0;I'd have to do some reading around the code to get an understanding of how this function works and is called, but I'm taking a guess that it's just an extra 'else if ... nops = 28 ...' for the simple approach (more complex if one were to try actual analysis). Ot at minimum, another entire if block, since this does look like it's specifically for HI/LO checks.
Quoted 9 lines
If you go for option 1, you could replace things like:
"\tbeq\t%@,%.,1b\n" \ "\tnop\n" \with:
"\tbeq%?\t%@,%.,1b\n" \ "\tnop\n" \
Looks simple enough. I found the block explaining what the %? parameter does. Is that in any actual documentation aside from a comment block in mips.c? I'm only looking at the 4.3.2 Internals Manual, cause I don't know if 4.4.x Internals is online yet. Wasn't sure if that was addressed from a documentation standpoint (or whether it's something that even needs to be listed online).
Quoted 3 lines
and make the .md insn do:
mips_branch_likely = TARGET_FIX_R10000;
Can this go anywhere in sync.md (i.e., at the top in a proper place), or does it need to go before any call to the macro templates?
Quoted 4 lines
But something nattier is needed for
MIPS_SYNC_NEW_OPandMIPS_SYNC_NEW_NAND, where the branch delay slot is not a nop. In this case, we need to replace things like:
Quoted 8 lines
"\tbeq\t%@,%.,1b\n" \ "\t" INSN "\t%0,%0,%2\n" \with:
"\tbeql\t%@,%.,1b\n" \ "\tnop\n" \ "\t" INSN "\t%0,%0,%2\n" \
Looking at what %# and %/ do, Maybe a new punctuation character that simply dumps out a nop instead if mips_branch_likely is true? I.e.:
case '~':
if (mips_branch_likely)
fputs ("\n\tnop", file);
break;And:
"\tbeq%?\t%@,%.,1b%~\n" \
"\t" INSN "\t%0,%0,%2\n" \'~' seems to be one of the last unused characters on my keyboard. Seems like a good fit.
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
Quoted 8 lines · Kumba
Quoted 6 lines
Agreed, but that's just as true of option 1. Each option is as correct as the other. It's just a question of whether we need the combination:
-mips1 -mllsc -mfix-r10000
to be accepted, or whether we can treat it as a compile-time error.
Hmm, which do you think makes sense? From a usage perspective, most
It's a crude way of asking for a generic MIPS binary that runs on anything but works best on MIPS II+.
Makes me wonder if there is a point in having a single gcc option, something like -march=generic which selects something like this, including all workarounds?
Ralf
Quoted 3 lines · Ralf Baechle
Makes me wonder if there is a point in having a single gcc option, something like -march=generic which selects something like this, including all workarounds?
No, please don't. If we decide to introduce it, someone will actually
decide to use it wasting computing power of good machines to handle corner cases. If somebody has a broken machine or a hardware vendor or a distributor has interest in supporting a particular flavour of breakage, then they are of course free to do so. But please do not make it too easy to spread. Let's give the hardware folks some incentive to fix their bugs. ;)
Maciej
Quoted 11 lines · Maciej W. Rozycki
Quoted 3 lines
Makes me wonder if there is a point in having a single gcc option, something like -march=generic which selects something like this, including all workarounds?
No, please don't. If we decide to introduce it, someone will actually
decide to use it wasting computing power of good machines to handle corner cases. If somebody has a broken machine or a hardware vendor or a distributor has interest in supporting a particular flavour of breakage, then they are of course free to do so. But please do not make it too easy to spread. Let's give the hardware folks some incentive to fix their bugs. ;)
I guess honorable mention for the years to come in the GCC man page (see -mfix-two-by-two-equals-five ;-) can do wonders.
Ralf
Quoted 7 lines · Kumba
But I guess the question I'm pondering, is just how rare would it be for someone to actually need a MIPS-I binary with ll/sc and branch-likely fixes to run on something like an R10000? Rare enough to justify denying them that particular command argument combination, and thus taking Option #1? Or go the extra mile for Option #2? I don't know if that's my call to really make, since I lack the statistical data to know who would be affected, and in what ways (i.e., do they have alternative methods, such as MIPS-II, etc..).
Workarounds should be as cheap as possible maintenance-wise. My vote is
for requiring people in the need to use broken R10k revisions to choose a compatible ISA. It actually makes sense to use 64-bit software on these systems implying at least MIPS III, which is also another argument not to try to bend backwards and support MIPS I with R10k workarounds.
Maciej
Quoted 17 lines · Kumba
Quoted 6 lines · Richard Sandiford
Agreed, but that's just as true of option 1. Each option is as correct as the other. It's just a question of whether we need the combination:
-mips1 -mllsc -mfix-r10000
to be accepted, or whether we can treat it as a compile-time error.
Hmm, which do you think makes sense? From a usage perspective, most developers are working in the MIPS32/MIPS64 ISA stuff. In Gentoo, the mips port mostly supports SGI systems, mostly anything with a MIPS-IV capable processor (haven't decided on MIPS-III's fate just yet). Debian I know has switched off of MIPS-I being the default for their binaries, and I think is MIPS-II now. In all these cases, the target OS is usually Linux, although I know there are some Irix folks still out there, plus the *BSDs all got their own ports.
Quoted 8 lines
But I guess the question I'm pondering, is just how rare would it be for someone to actually need a MIPS-I binary with ll/sc and branch-likely fixes to run on something like an R10000? Rare enough to justify denying them that particular command argument combination, and thus taking Option #1? Or go the extra mile for Option #2? I don't know if that's my call to really make, since I lack the statistical data to know who would be affected, and in what ways (i.e., do they have alternative methods, such as MIPS-II, etc..).
I'm not sure I have the statistical knowledge either. I've tended to work in embedded environments where -march=<my cpu> is almost always the right option to use. But like Maciej, I suspect it isn't worth supporting the combination. So my preference is for option #1.
You make a convincing case that the combination isn't useful for current Linux distributions. And it isn't useful for IRIX 6 either: the o32 system libraries are -mips2 rather than -mips1, and both GCC and MIPSpro default to -mips2 for o32.
Also, I believe the glibc patch doesn't cope with -mips1 -mllsc either. Is that right? If so, that's another reason not to worry about it for GCC.
I don't have a strong opinion though.
Quoted 19 lines
Quoted 4 lines
If you do go for option 2, you then have to decide whether to insert 28 nops after every LL/SC loop, or whether you try to do some analysis to avoid unnecessary nops. The natural place for this analysis would be
mips_avoid_hazard.Hmm, just looking at this out of curiosity to get an idea of what I might have to tackle, but this particular sequence looks like the key:
/* Work out how many nops are needed. Note that we only care about registers that are explicitly mentioned in the instruction's pattern. It doesn't matter that calls use the argument registers or that they clobber hi and lo. */ if (*hilo_delay < 2 && reg_set_p (lo_reg, pattern)) nops = 2 - *hilo_delay; else if (*delayed_reg != 0 && reg_referenced_p (*delayed_reg, pattern)) nops = 1; else nops = 0;
Quoted 6 lines
I'd have to do some reading around the code to get an understanding of how this function works and is called, but I'm taking a guess that it's just an extra 'else if ... nops = 28 ...' for the simple approach (more complex if one were to try actual analysis). Ot at minimum, another entire if block, since this does look like it's specifically for HI/LO checks.
It's a bit more complicated than that. The current code takes advantage of a nice property: that a gap of two instructions will avoid all the hazards that we currently handle. So if we come across a branch, we only ever need to look at the branch and its delay slot; we never need to look at the target of a branch. For the R10000 errata, you would either:
(1) need to do proper global (inter-block) analaysis, which is a fair bit of new code; or
(2) conservatively assume that the target of a branch might be a
__sync_*() operation.
Also, the "nops =" part of the current code inserts "#nop" rather than "nop" for ".set reorder" functions, because the assembler is supposed to avoid the hazards in that case. Unless you make GAS do the same for this errata, you would need to:
(1) insert a real nop instead of a hazard_nop; and
(2) treat any asm as a potential atomic operation.
Quoted 10 lines
Quoted 9 lines
If you go for option 1, you could replace things like:
"\tbeq\t%@,%.,1b\n" \ "\tnop\n" \with:
"\tbeq%?\t%@,%.,1b\n" \ "\tnop\n" \
Quoted 6 lines
Looks simple enough. I found the block explaining what the %? parameter does. Is that in any actual documentation aside from a comment block in
mips.c? I'm only looking at the 4.3.2 Internals Manual, cause I don't know if 4.4.x Internals is online yet. Wasn't sure if that was addressed from a documentation standpoint (or whether it's something that even needs to be listed online).
The internals manual intentionally doesn't cover things like this. It's for target-independent stuff, not for internal details about each port. So the mips.c comment _is_ the documentation.
Quoted 6 lines
Quoted 3 lines
and make the .md insn do:
mips_branch_likely = TARGET_FIX_R10000;Can this go anywhere in sync.md (i.e., at the top in a proper place), or does it need to go before any call to the macro templates?
mips_branch_likely only applies to the _current_ insn, so it needs to go before any call the macro templates. Please use a helper function such as:
const char *mips_output_sync_insn (const char *template)
{
mips_branch_likely = TARGET_FIX_R10000;
return template;
}
Quoted 21 lines
Quoted 4 lines
But something nattier is needed for
MIPS_SYNC_NEW_OPandMIPS_SYNC_NEW_NAND, where the branch delay slot is not a nop. In this case, we need to replace things like:Quoted 8 lines
"\tbeq\t%@,%.,1b\n" \ "\t" INSN "\t%0,%0,%2\n" \with:
"\tbeql\t%@,%.,1b\n" \ "\tnop\n" \ "\t" INSN "\t%0,%0,%2\n" \Looking at what %# and %/ do, Maybe a new punctuation character that simply dumps out a nop instead if
mips_branch_likelyis true? I.e.:case '~': if (mips_branch_likely) fputs ("\n\tnop", file); break;
Quoted 7 lines
And:
"\tbeq%?\t%@,%.,1b%~\n" \ "\t" INSN "\t%0,%0,%2\n" \'~' seems to be one of the last unused characters on my keyboard. Seems like a good fit.
Yeah, looks good. I'm a bit worried about running of % characters, but like I say, we could always replace the templates with individual output_asm_insns at some point in the future.
Richard
For what it's worth:
R10K should be able to compile with -mips4
On SGI-systems I think they deprecated all the R3K and earlier systems with IRIX 5.3 IIRC.
My old SGI Indigo (yeah, the original Indigo) R4K still runs fine with IRIX 6.5.22 and -mips3/n32-binaries last time I checked.
If in doubt ask on http://www.nekochan.net
//Markus
Quoted 159 lines · Richard Sandiford
Quoted 17 lines · Kumba
Quoted 6 lines · Richard Sandiford
Agreed, but that's just as true of option 1. Each option is as correct as the other. It's just a question of whether we need the combination:
-mips1 -mllsc -mfix-r10000
to be accepted, or whether we can treat it as a compile-time error.
Hmm, which do you think makes sense? From a usage perspective, most developers are working in the MIPS32/MIPS64 ISA stuff. In Gentoo, the mips port mostly supports SGI systems, mostly anything with a MIPS-IV capable processor (haven't decided on MIPS-III's fate just yet). Debian I know has switched off of MIPS-I being the default for their binaries, and I think is MIPS-II now. In all these cases, the target OS is usually Linux, although I know there are some Irix folks still out there, plus the *BSDs all got their own ports.
Quoted 8 lines
But I guess the question I'm pondering, is just how rare would it be for someone to actually need a MIPS-I binary with ll/sc and branch-likely fixes to run on something like an R10000? Rare enough to justify denying them that particular command argument combination, and thus taking Option #1? Or go the extra mile for Option #2? I don't know if that's my call to really make, since I lack the statistical data to know who would be affected, and in what ways (i.e., do they have alternative methods, such as MIPS-II, etc..).
I'm not sure I have the statistical knowledge either. I've tended to work in embedded environments where -march=<my cpu> is almost always the right option to use. But like Maciej, I suspect it isn't worth supporting the combination. So my preference is for option #1.
You make a convincing case that the combination isn't useful for current Linux distributions. And it isn't useful for IRIX 6 either: the o32 system libraries are -mips2 rather than -mips1, and both GCC and MIPSpro default to -mips2 for o32.
Also, I believe the glibc patch doesn't cope with -mips1 -mllsc either. Is that right? If so, that's another reason not to worry about it for GCC.
I don't have a strong opinion though.
Quoted 22 lines
Quoted 4 lines
If you do go for option 2, you then have to decide whether to insert 28 nops after every LL/SC loop, or whether you try to do some analysis to avoid unnecessary nops. The natural place for this analysis would be
mips_avoid_hazard.Hmm, just looking at this out of curiosity to get an idea of what I might have to tackle, but this particular sequence looks like the key:
/* Work out how many nops are needed. Note that we only care about
registers that are explicitly mentioned in the instruction's
pattern.
It doesn't matter that calls use the argument registers or
that they
clobber hi and lo. */
if (*hilo_delay< 2 &®_set_p(lo_reg, pattern))nops = 2 - *hilo_delay; else if (*delayed_reg != 0 && reg_referenced_p (*delayed_reg, pattern)) nops = 1; else nops = 0;Quoted 6 lines
I'd have to do some reading around the code to get an understanding of how this function works and is called, but I'm taking a guess that it's just an extra 'else if ... nops = 28 ...' for the simple approach (more complex if one were to try actual analysis). Ot at minimum, another entire if block, since this does look like it's specifically for HI/LO checks.
It's a bit more complicated than that. The current code takes advantage of a nice property: that a gap of two instructions will avoid all the hazards that we currently handle. So if we come across a branch, we only ever need to look at the branch and its delay slot; we never need to look at the target of a branch. For the R10000 errata, you would either:
(1) need to do proper global (inter-block) analaysis, which is a fair bit of new code; or
(2) conservatively assume that the target of a branch might be a
__sync_*() operation.Also, the "nops =" part of the current code inserts "#nop" rather than "nop" for ".set reorder" functions, because the assembler is supposed to avoid the hazards in that case. Unless you make GAS do the same for this errata, you would need to:
(1) insert a real nop instead of a
hazard_nop; and(2) treat any asm as a potential atomic operation.
Quoted 10 lines
Quoted 9 lines
If you go for option 1, you could replace things like:
"\tbeq\t%@,%.,1b\n" \
"\tnop\n" \with:
"\tbeq%?\t%@,%.,1b\n" \
"\tnop\n" \Quoted 6 lines
Looks simple enough. I found the block explaining what the %? parameter does. Is that in any actual documentation aside from a comment block in
mips.c? I'm only looking at the 4.3.2 Internals Manual, cause I don't know if 4.4.x Internals is online yet. Wasn't sure if that was addressed from a documentation standpoint (or whether it's something that even needs to be listed online).The internals manual intentionally doesn't cover things like this. It's for target-independent stuff, not for internal details about each port. So the
mips.ccomment _is_ the documentation.Quoted 6 lines
Quoted 3 lines
and make the .md insn do:
mips_branch_likely=TARGET_FIX_R10000;Can this go anywhere in sync.md (i.e., at the top in a proper place), or does it need to go before any call to the macro templates?
mips_branch_likelyonly applies to the _current_ insn, so it needs to go before any call the macro templates. Please use a helper function such as:const char *
mips_output_sync_insn(const char *template)
{
mips_branch_likely=TARGET_FIX_R10000;
return template;
}Quoted 26 lines
Quoted 4 lines
But something nattier is needed for
MIPS_SYNC_NEW_OPandMIPS_SYNC_NEW_NAND, where the branch delay slot is not a nop. In this case, we need to replace things like:Quoted 8 lines
"\tbeq\t%@,%.,1b\n" \
"\t" INSN "\t%0,%0,%2\n" \with:
"\tbeql\t%@,%.,1b\n" \
"\tnop\n" \
"\t" INSN "\t%0,%0,%2\n" \Looking at what %# and %/ do, Maybe a new punctuation character that simply dumps out a nop instead if
mips_branch_likelyis true? I.e.:case '~': if (mips_branch_likely) fputs ("\n\tnop", file); break;And:
"\tbeq%?\t%@,%.,1b%~\n" \ "\t" INSN "\t%0,%0,%2\n" \
Quoted 9 lines
Quoted 2 lines
'~' seems to be one of the last unused characters on my keyboard. Seems like a good fit.
Yeah, looks good. I'm a bit worried about running of % characters, but like I say, we could always replace the templates with individual
output_asm_insnsat some point in the future.Richard
_______________________________________
Mr Markus Gothe
Software Engineer
Phone: +46 (0)13 21 81 20 (ext. 1046)
Fax: +46 (0)13 21 21 15
Mobile: +46 (0)70 348 44 35 Diskettgatan 11, SE-583 35 Linköping, Sweden www.27m.com
Quoted 10 lines · Richard Sandiford
I'm not sure I have the statistical knowledge either. I've tended to work in embedded environments where -march=<my cpu> is almost always the right option to use. But like Maciej, I suspect it isn't worth supporting the combination. So my preference is for option #1.
You make a convincing case that the combination isn't useful for current Linux distributions. And it isn't useful for IRIX 6 either: the o32 system libraries are -mips2 rather than -mips1, and both GCC and MIPSpro default to -mips2 for o32.
Yeah, trying to handle MIPS-I stuff looks like it'll be above my head for now, so I'm going to aim at the second option after all.
FYI, I explain about the two different patches attached to this below. They're not final by any means, but I'm doing something wrong somewhere in both them.
Quoted 3 lines
Also, I believe the glibc patch doesn't cope with -mips1 -mllsc either. Is that right? If so, that's another reason not to worry about it for GCC.
It doesn't as I coded it. I plan on addressing that patch after the gcc-side of things. Ralf suggested in that patch on libc-ports that I handle the MIPS-I case there, though, but if we're not going to support it in the gcc patch, then it probably isn't needed in the glibc patch either. We'll see, though!
Quoted 11 lines
mips_branch_likelyonly applies to the _current_ insn, so it needs to go before any call the macro templates. Please use a helper function such as:const char *
mips_output_sync_insn(const char *template)
{
mips_branch_likely=TARGET_FIX_R10000;
return template;
}
Done. This is referenced in the first patch (gcc-4.4-trunk-fixr10k-z1.patch). The second patch (gcc-4.4-trunk-fixr10k-z2.patch) contains a form whereby I just re-declared mips_branch_likely and set it once-per template. More on this below.
Quoted 3 lines
Yeah, looks good. I'm a bit worried about running of % characters, but like I say, we could always replace the templates with individual
output_asm_insnsat some point in the future.
Yeah, ~ is one of the last characters that doesn't seem to be completely used up and looks good. That still leaves !, &, {, }, and a comma. But those could look confusing with surrounding characters.
So about the two patches. Both of these appears to accomplish the job, and allow gcc to begin compiling, but at one point about two hours into the build, genautomata will segfault when attempting to output tmp-automata.c. I don't know which stage this is in...it's one of the early stages, and it's using xgcc at this point.
I tried running gdb on that particular invocation of genautomata, but it there's not much data I could gather, since the -O2 optimization removes some of the useful debugging info. It segfaults at an fprintf() invocation, and tmp-automata.c is 0 bytes.
Here's the last few lines I get:
/usr/cvsroot/gcc/host-mips-unknown-linux-gnu/prev-gcc/xgcc -B/usr/cvsroot/gcc/host-mips-unknown-linux-gnu/prev-gcc/ -B/usr//mips-unknown-linux-gnu/bin/ -g -O2 -DIN_GCC -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wcast-qual -Wold-style-definition -Wc++-compat -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -DHAVE_CONFIG_H -DGENERATOR_FILE
-o build/genautomata \
build/genautomata.o build/rtl.o build/read-rtl.o build/ggc-none.obuild/vec.o build/min-insn-modes.o build/gensupport.o build/print-rtl.o build/errors.o ../../host-mips-unknown-linux-gnu/libiberty/libiberty.a -lm build/genautomata ../.././gcc/config/mips/mips.md \
insn-conditions.md > tmp-automata.c/bin/sh: line 1: 28620 Segmentation fault build/genautomata ../.././gcc/config/mips/mips.md insn-conditions.md > tmp-automata.c
make[3]: *** [s-automata] Error 139
make[3]: Leaving directory `/usr/cvsroot/gcc/host-mips-unknown-linux-gnu/gcc'
make[2]: *** [all-stage2-gcc] Error 2
make[2]: Leaving directory `/usr/cvsroot/gcc'
make[1]: *** [stage2-bubble] Error 2
make[1]: Leaving directory `/usr/cvsroot/gcc'
make: *** [all] Error 2
I thought at first, it was the use of the helper function, so I backed that out and went with the form seen in the second patch, but that didn't help things either. So I'm assuming this is related to the changes to the atomic macro templates, and xgcc must have something inside itself that's a little wonky. Not real sure how to approach this.
However, there's more. If I rebuild genautomata by hand (using args from the command line), and I drop the optimization down a notch to -O1, then I can run the command to create tmp-automata.c, and it'll complete successfully (and the output in that file looks good). So I'm a bit baffled. I assume the issue is caused by my patch, unless I'm running into a regression in trunk that my patch simply exposes.
Is there another way to maybe extract some info on what's causing this?
Thanks!
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
gcc/config/mips/mips.c +35 -1
@@ -6909,6 +6909,7 @@ mips_print_operand_reloc (FILE *file, rt '#' Print a nop if in a ".set noreorder" block. '/' Like '#', but do nothing within a delayed-branch sequence. '?' Print "l" if mips_branch_likely is true + '~' Print a nop if mips_branch_likely is true '.' Print the name of the register with a hard-wired zero (zero or $0). '@' Print the name of the assembler temporary register (at or $1). '^' Print the name of the pic call-through register (t9 or $25).
@@ -6983,6 +6984,11 @@ mips_print_operand_punctuation (FILE *fiputc ('l' , file);break ; +case '~' : +if (mips_branch_likely) +fputs ("\n\tnop" , file); +break ; +case '.' :fputs (reg_names[GP_REG_FIRST +0 ], file);break ;
@@ -7026,7 +7032,7 @@ mips_init_print_operand_punct (void) {const char *p; -for (p ="()[]<>*#/?.@^+$|-" ; *p; p++) +for (p ="()[]<>*#/?~.@^+$|-" ; *p; p++) mips_print_operand_punct[(unsigned char ) *p] = true; }
@@ -10250,6 +10256,17 @@ mips_output_order_conditional_branch (rtreturn mips_output_conditional_branch (insn, operands, branch[1 ], branch[0 ]); } +/* Return a template for the __sync_* functions after setting mips_branch_likely +to the value of TARGET_FIX_R10000 to enable a proper workaround of R10000 +errata. */ + +const char * +mips_output_sync_insn (const char *template) +{ + mips_branch_likely =TARGET_FIX_R10000 ; +return template; +} +/* Return the assembly code for DIV or DDIV instruction DIVISION, which has the operands given by OPERANDS. Add in a divide-by-zero check if needed.
@@ -13824,6 +13841,17 @@ mips_override_options (void)warning (0 ,"the %qs architecture does not support branch-likely" " instructions" , mips_arch_info->name); +/* Check to see whether branch-likely instructions are not available +when using -mfix-r10000. This will be true if: +1. -mno-branch-likely was passed. +2. The selected ISA does not support branch-likely and +the command line does not include -mbranch-likely */ +if ((TARGET_FIX_R10000 + && (target_flags_explicit &MASK_BRANCHLIKELY ) ==0 ) + ? !ISA_HAS_BRANCHLIKELY + ? !TARGET_BRANCHLIKELY : false : false) +sorry ("branch-likely instructions not available" ); +/* The effect of -mabicalls isn't defined for the EABI. */ if (mips_abi ==ABI_EABI &&TARGET_ABICALLS ) {
@@ -13971,6 +13999,12 @@ mips_override_options (void) && mips_matching_cpu_name_p (mips_arch_info->name, "r4400")) target_flags |=MASK_FIX_R4400 ; +/* Default to working around R10000 errata only if the processor +was selected explicitly. */ +if ((target_flags_explicit &MASK_FIX_R10000 ) ==0 + &&mips_matching_cpu_name_p (mips_arch_info->name,"r10000" )) + target_flags |=MASK_FIX_R10000 ; +/* Save base state of options. */ mips_base_target_flags = target_flags; mips_base_delayed_branch = flag_delayed_branch;
gcc/config/mips/mips.h +13 -13
@@ -3083,7 +3083,7 @@ while (0)"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3108,7 +3108,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3128,7 +3128,7 @@ while (0)"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3153,7 +3153,7 @@ while (0)"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3186,7 +3186,7 @@ while (0)"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3216,7 +3216,7 @@ while (0)"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3236,7 +3236,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3253,7 +3253,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3270,7 +3270,7 @@ while (0)"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3289,7 +3289,7 @@ while (0)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3308,7 +3308,7 @@ while (0)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3326,7 +3326,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3350,7 +3350,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
gcc/config/mips/mips.opt +4 -0
@@ -112,6 +112,10 @@ mfix-r4400 Target Report Mask(FIX_R4400) Work around certain R4400 errata +mfix-r10000 +Target Report Mask(FIX_R10000) +Work around certain R10000 errata + mfix-sb1 Target Report Var(TARGET_FIX_SB1) Work around errata for early SB-1 revision 2 cores
gcc/config/mips/sync.md +42 -37
223 lines not on this page · raw message
gcc/doc/invoke.texi +1 -1
@@ -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
gcc/config/mips/mips.c +24 -1
@@ -6909,6 +6909,7 @@ mips_print_operand_reloc (FILE *file, rt '#' Print a nop if in a ".set noreorder" block. '/' Like '#', but do nothing within a delayed-branch sequence. '?' Print "l" if mips_branch_likely is true + '~' Print a nop if mips_branch_likely is true '.' Print the name of the register with a hard-wired zero (zero or $0). '@' Print the name of the assembler temporary register (at or $1). '^' Print the name of the pic call-through register (t9 or $25).
@@ -6983,6 +6984,11 @@ mips_print_operand_punctuation (FILE *fiputc ('l' , file);break ; +case '~' : +if (mips_branch_likely) +fputs ("\n\tnop" , file); +break ; +case '.' :fputs (reg_names[GP_REG_FIRST +0 ], file);break ;
@@ -7026,7 +7032,7 @@ mips_init_print_operand_punct (void) {const char *p; -for (p ="()[]<>*#/?.@^+$|-" ; *p; p++) +for (p ="()[]<>*#/?~.@^+$|-" ; *p; p++) mips_print_operand_punct[(unsigned char ) *p] = true; }
@@ -13824,6 +13830,17 @@ mips_override_options (void)warning (0 ,"the %qs architecture does not support branch-likely" " instructions" , mips_arch_info->name); +/* Check to see whether branch-likely instructions are not available +when using -mfix-r10000. This will be true if: +1. -mno-branch-likely was passed. +2. The selected ISA does not support branch-likely and +the command line does not include -mbranch-likely */ +if ((TARGET_FIX_R10000 + && (target_flags_explicit &MASK_BRANCHLIKELY ) ==0 ) + ? !ISA_HAS_BRANCHLIKELY + ? !TARGET_BRANCHLIKELY : false : false) +sorry ("branch-likely instructions not available" ); +/* The effect of -mabicalls isn't defined for the EABI. */ if (mips_abi ==ABI_EABI &&TARGET_ABICALLS ) {
@@ -13971,6 +13988,12 @@ mips_override_options (void) && mips_matching_cpu_name_p (mips_arch_info->name, "r4400")) target_flags |=MASK_FIX_R4400 ; +/* Default to working around R10000 errata only if the processor +was selected explicitly. */ +if ((target_flags_explicit &MASK_FIX_R10000 ) ==0 + &&mips_matching_cpu_name_p (mips_arch_info->name,"r10000" )) + target_flags |=MASK_FIX_R10000 ; +/* Save base state of options. */ mips_base_target_flags = target_flags; mips_base_delayed_branch = flag_delayed_branch;
gcc/config/mips/mips.h +13 -13
@@ -3083,7 +3083,7 @@ while (0)"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3108,7 +3108,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3128,7 +3128,7 @@ while (0)"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3153,7 +3153,7 @@ while (0)"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3186,7 +3186,7 @@ while (0)"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3216,7 +3216,7 @@ while (0)"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3236,7 +3236,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3253,7 +3253,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3270,7 +3270,7 @@ while (0)"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3289,7 +3289,7 @@ while (0)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3308,7 +3308,7 @@ while (0)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3326,7 +3326,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3350,7 +3350,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
gcc/config/mips/mips.opt +4 -0
@@ -112,6 +112,10 @@ mfix-r4400 Target Report Mask(FIX_R4400) Work around certain R4400 errata +mfix-r10000 +Target Report Mask(FIX_R10000) +Work around certain R10000 errata + mfix-sb1 Target Report Var(TARGET_FIX_SB1) Work around errata for early SB-1 revision 2 cores
gcc/config/mips/sync.md +44 -0
176 lines not on this page · raw message
gcc/doc/invoke.texi +1 -1
@@ -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
Quoted 13 lines · Kumba
Quoted 11 lines · Richard Sandiford
mips_branch_likelyonly applies to the _current_ insn, so it needs to go before any call the macro templates. Please use a helper function such as:const char *
mips_output_sync_insn(const char *template)
{
mips_branch_likely=TARGET_FIX_R10000;
return template;
}
Quoted 5 lines
Done. This is referenced in the first patch (gcc-4.4-trunk-fixr10k-z1.patch). The second patch (gcc-4.4-trunk-fixr10k-z2.patch) contains a form whereby I just re-declared
mips_branch_likelyand set it once-per template. More on this below.
The first version looks good, except for a couple of formatting issues. The second version doesn't work: those static mips_branch_likely variables are local to insn-output.c, so mips.c:print_operand will never see them.
Quoted 3 lines
Yeah, ~ is one of the last characters that doesn't seem to be completely used up and looks good. That still leaves !, &, {, }, and a comma. But those could look confusing with surrounding characters.
Ah, well, that's not too bad. I wouldn't have many qualms about using %! and %& if need be.
Quoted 43 lines
So about the two patches. Both of these appears to accomplish the job, and allow gcc to begin compiling, but at one point about two hours into the build, genautomata will segfault when attempting to output
tmp-automata.c. I don't know which stage this is in...it's one of the early stages, and it's using xgcc at this point.I tried running gdb on that particular invocation of genautomata, but it there's not much data I could gather, since the -O2 optimization removes some of the useful debugging info. It segfaults at an
fprintf()invocation, andtmp-automata.cis 0 bytes.Here's the last few lines I get:
/usr/cvsroot/gcc/host-mips-unknown-linux-gnu/prev-gcc/xgcc(quoted diff) +0
-5-B/usr/cvsroot/gcc/host-mips-unknown-linux-gnu/prev-gcc/ -B/usr//mips-unknown-linux-gnu/bin/ -g -O2 -DIN_GCC -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wcast-qual -Wold-style-definition -Wc++-compat -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -DHAVE_CONFIG_H -DGENERATOR_FILE -o build/genautomata \ build/genautomata.o build/rtl.o build/read-rtl.o build/ggc-none.o
build/vec.obuild/min-insn-modes.obuild/gensupport.obuild/print-rtl.obuild/errors.o../../host-mips-unknown-linux-gnu/libiberty/libiberty.a -lm build/genautomata../.././gcc/config/mips/mips.md\
insn-conditions.md >tmp-automata.c/bin/sh: line 1: 28620 Segmentation fault build/genautomata../.././gcc/config/mips/mips.mdinsn-conditions.md >tmp-automata.cmake[3]: *** [s-automata] Error 139
make[3]: Leaving directory `/usr/cvsroot/gcc/host-mips-unknown-linux-gnu/gcc' make[2]: *** [all-stage2-gcc] Error 2
make[2]: Leaving directory `/usr/cvsroot/gcc'
make[1]: *** [stage2-bubble] Error 2
make[1]: Leaving directory `/usr/cvsroot/gcc'
make: *** [all] Error 2I thought at first, it was the use of the helper function, so I backed that out and went with the form seen in the second patch, but that didn't help things either. So I'm assuming this is related to the changes to the atomic macro templates, and xgcc must have something inside itself that's a little wonky. Not real sure how to approach this.
Quoted 8 lines
However, there's more. If I rebuild genautomata by hand (using args from the command line), and I drop the optimization down a notch to -O1, then I can run the command to create
tmp-automata.c, and it'll complete successfully (and the output in that file looks good). So I'm a bit baffled. I assume the issue is caused by my patch, unless I'm running into a regression in trunk that my patch simply exposes.Is there another way to maybe extract some info on what's causing this?
For avoidance of doubt, I suppose the first thing to ask is: do you get the segfault with the same checkout after you revert your patch? It could certainly be transient breakage on trunk, like you say.
gcc/config/mips/mips.c mips_override_options (void) · line 13848
Quoted 9 lines
gcc/config/mips/mips.c +5
-0@@ -13824,6 +13841,17 @@ mips_override_options (void)warning (0 ,"the %qs architecture does not support branch-likely" " instructions" , mips_arch_info->name); +/* Check to see whether branch-likely instructions are not available +when using -mfix-r10000. This will be true if: +1. -mno-branch-likely was passed. +2. The selected ISA does not support branch-likely and +the command line does not include -mbranch-likely */
Nitlet, but "to see" is redundant. Maybe:
/* Make sure that branch-likely instructions available when using
-mfix-r10000. The instructions are not available if either:
1. -mno-branch-likely was passed.
2. The selected ISA does not support branch-likely and the command line does not include -mbranch-likely. */
gcc/config/mips/mips.c mips_override_options (void) · line 13852
Quoted 4 lines
gcc/config/mips/mips.c +4
-0+if ((TARGET_FIX_R10000 + && (target_flags_explicit &MASK_BRANCHLIKELY ) ==0 ) + ? !ISA_HAS_BRANCHLIKELY + ? !TARGET_BRANCHLIKELY : false : false)
Should just be:
if (TARGET_FIX_R10000
&& ((target_flags_explicit & MASK_BRANCHLIKELY) == 0
? !ISA_HAS_BRANCHLIKELY
: !TARGET_BRANCHLIKEL))
sorry ("branch-likely instructions not available");And the check should go after...
gcc/config/mips/mips.c mips_override_options (void) · line 14010
Quoted 13 lines
gcc/config/mips/mips.c +6
-0@@ -13971,6 +13999,12 @@ mips_override_options (void) && mips_matching_cpu_name_p (mips_arch_info->name, "r4400")) target_flags |=MASK_FIX_R4400 ; +/* Default to working around R10000 errata only if the processor +was selected explicitly. */ +if ((target_flags_explicit &MASK_FIX_R10000 ) ==0 + &&mips_matching_cpu_name_p (mips_arch_info->name,"r10000" )) + target_flags |=MASK_FIX_R10000 ; +/* Save base state of options. */ mips_base_target_flags = target_flags; mips_base_delayed_branch = flag_delayed_branch;
...this.
(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.)
gcc/config/mips/sync.md line 81
Quoted 9 lines
gcc/config/mips/sync.md +2
-2@@ -76,9 +76,9 @@ "GENERATE_LL_SC" { if (which_alternative == 0) - return MIPS_COMPARE_AND_SWAP_12 (MIPS_COMPARE_AND_SWAP_12_NONZERO_OP); + return mips_output_sync_insn (MIPS_COMPARE_AND_SWAP_12 (MIPS_COMPARE_AND_SWAP_12_NONZERO_OP)); else - return MIPS_COMPARE_AND_SWAP_12 (MIPS_COMPARE_AND_SWAP_12_ZERO_OP); + return mips_output_sync_insn (MIPS_COMPARE_AND_SWAP_12 (MIPS_COMPARE_AND_SWAP_12_ZERO_OP));
Break lines longer than 80 chars. Here and elsewhere, it's probably best to use:
return (mips_output_sync_insn
(...stuff...));rather than things like:
gcc/config/mips/sync.md line 165
Quoted 9 lines
gcc/config/mips/sync.md +3
-2@@ -160,8 +160,9 @@ (clobber (match_scratch:SI 5 "=&d"))] "GENERATE_LL_SC" { - return MIPS_SYNC_OLD_OP_12 ("<insn>", MIPS_SYNC_OLD_OP_12_NOT_NOP, - MIPS_SYNC_OLD_OP_12_NOT_NOP_REG); + return mips_output_sync_insn (MIPS_SYNC_OLD_OP_12 ("<insn>", + MIPS_SYNC_OLD_OP_12_NOT_NOP, + MIPS_SYNC_OLD_OP_12_NOT_NOP_REG));
...this. Arguments should generally be indented at least as far as the opening "(".
Looks good otherwise, thanks. We just need to sort out the build problem.
Richard
Quoted 3 lines · Richard Sandiford
For avoidance of doubt, I suppose the first thing to ask is: do you get the segfault with the same checkout after you revert your patch? It could certainly be transient breakage on trunk, like you say.
Looks like it is: PR38052.
Richard
Quoted 9 lines · Richard Sandiford
(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.)
FWIW, my preference is for an error too. The main reason being to warn
the user about the incompatibility of these options.
For the opposite case a scenario where in a building system they come
from different places each can be imagined. Or one could be buried somewhere down the Makefiles in a platform-specific section of an odd package. In the end the user might not be fully aware they are doing something wrong and the result would be broken packages would fail randomly on the affected processors at the run time.
Always prefer a build error to a run-time error if possible.
Maciej
Quoted 5 lines · Richard Sandiford
The first version looks good, except for a couple of formatting issues. The second version doesn't work: those static
mips_branch_likelyvariables are local toinsn-output.c, so mips.c:print_operand will never see them.
Yeah, I'm going to drop the second patch now that I know the genautomata thing is a regression of some sort.
Quoted 9 lines
Nitlet, but "to see" is redundant. Maybe:
/* Make sure that branch-likely instructions available when using
-mfix-r10000. The instructions are not available if either:1. -mno-branch-likely was passed.
2. The selected ISA does not support branch-likely and the command line does not include -mbranch-likely. */
Fixed.
Quoted 9 lines
Should just be:
if (TARGET_FIX_R10000 && ((target_flags_explicit & MASK_BRANCHLIKELY) == 0 ? !ISA_HAS_BRANCHLIKELY : !TARGET_BRANCHLIKEL)) sorry ("branch-likely instructions not available");And the check should go after...
Fixed & Done.
Quoted 9 lines
(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 (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).
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");
}
Quoted 5 lines
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? 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.
Quoted 2 lines
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.
I'm running a build now on my x86 box to see if I hit the same error there, or if this is limited only to mips (the bugs suggests it might, but one never knows).
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. No idea if there is a "safer" way to check for this flaw and recover from it. The below link is the snippet of code that reliably hung an IP28 machine:
http://www.linux-mips.org/archives/linux-mips/2008-01/msg00186.html
Thanks!
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
gcc/config/mips/mips.c +35 -1
@@ -6909,6 +6909,7 @@ mips_print_operand_reloc (FILE *file, rt '#' Print a nop if in a ".set noreorder" block. '/' Like '#', but do nothing within a delayed-branch sequence. '?' Print "l" if mips_branch_likely is true + '~' Print a nop if mips_branch_likely is true '.' Print the name of the register with a hard-wired zero (zero or $0). '@' Print the name of the assembler temporary register (at or $1). '^' Print the name of the pic call-through register (t9 or $25).
@@ -6983,6 +6984,11 @@ mips_print_operand_punctuation (FILE *fiputc ('l' , file);break ; +case '~' : +if (mips_branch_likely) +fputs ("\n\tnop" , file); +break ; +case '.' :fputs (reg_names[GP_REG_FIRST +0 ], file);break ;
@@ -7026,7 +7032,7 @@ mips_init_print_operand_punct (void) {const char *p; -for (p ="()[]<>*#/?.@^+$|-" ; *p; p++) +for (p ="()[]<>*#/?~.@^+$|-" ; *p; p++) mips_print_operand_punct[(unsigned char ) *p] = true; }
@@ -10250,6 +10256,17 @@ mips_output_order_conditional_branch (rtreturn mips_output_conditional_branch (insn, operands, branch[1 ], branch[0 ]); } +/* Return a template for the __sync_* functions after setting mips_branch_likely +to the value of TARGET_FIX_R10000 to enable a proper workaround of R10000 +errata. */ + +const char * +mips_output_sync_insn (const char *template) +{ + mips_branch_likely =TARGET_FIX_R10000 ; +return template; +} +/* Return the assembly code for DIV or DDIV instruction DIVISION, which has the operands given by OPERANDS. Add in a divide-by-zero check if needed.
@@ -13971,6 +13988,23 @@ mips_override_options (void) && mips_matching_cpu_name_p (mips_arch_info->name, "r4400")) target_flags |=MASK_FIX_R4400 ; +/* Default to working around R10000 errata only if the processor +was selected explicitly. */ +if ((target_flags_explicit &MASK_FIX_R10000 ) ==0 + &&mips_matching_cpu_name_p (mips_arch_info->name,"r10000" )) + target_flags |=MASK_FIX_R10000 ; + +/* Make sure that branch-likely instructions available when using +-mfix-r10000. The instructions are not available if either: +1. -mno-branch-likely was passed. +2. The selected ISA does not support branch-likely and +the command line does not include -mbranch-likely */ +if (TARGET_FIX_R10000 + && ((target_flags_explicit &MASK_BRANCHLIKELY ) ==0 + ? !ISA_HAS_BRANCHLIKELY + : !TARGET_BRANCHLIKEL )) +sorry ("branch-likely instructions not available" ); +/* Save base state of options. */ mips_base_target_flags = target_flags; mips_base_delayed_branch = flag_delayed_branch;
gcc/config/mips/mips.h +13 -13
@@ -3089,7 +3089,7 @@ while (0)"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3114,7 +3114,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3134,7 +3134,7 @@ while (0)"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3159,7 +3159,7 @@ while (0)"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3192,7 +3192,7 @@ while (0)"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3222,7 +3222,7 @@ while (0)"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3242,7 +3242,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3259,7 +3259,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3276,7 +3276,7 @@ while (0)"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3295,7 +3295,7 @@ while (0)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3314,7 +3314,7 @@ while (0)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3332,7 +3332,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3356,7 +3356,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
gcc/config/mips/mips.opt +4 -0
@@ -112,6 +112,10 @@ mfix-r4400 Target Report Mask(FIX_R4400) Work around certain R4400 errata +mfix-r10000 +Target Report Mask(FIX_R10000) +Work around certain R10000 errata + mfix-sb1 Target Report Var(TARGET_FIX_SB1) Work around errata for early SB-1 revision 2 cores
gcc/config/mips/sync.md +51 -37
232 lines not on this page · raw message
gcc/doc/invoke.texi +1 -1
@@ -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
Thanks for the new patch. Generally looks good.
Quoted 9 lines · Kumba
Quoted 8 lines · Richard Sandiford
(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.
Quoted 7 lines
Quoted 1 line
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 raiseerror()instead, warning the user about the invalid flag combinations
It should be sorry() in all cases.
Quoted 4 lines
(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.
Quoted 8 lines
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:
Quoted 7 lines
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.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 3 lines
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).
Quoted 10 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
gcc/config/mips/mips.c mips_override_options (void) · line 13998
Quoted 2 lines
gcc/config/mips/mips.c +2
-0+/* 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.
gcc/config/mips/mips.c mips_override_options (void) · line 14001
Quoted 3 lines
gcc/config/mips/mips.c +3
-0+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).
gcc/doc/invoke.texi Objective-C and Objective-C++ Dialects}. · line 672
Quoted 12 lines
diff -Naurp -x .svn gcc.orig/gcc/doc/invoke.texi gcc/gcc/doc/invoke.texi
gcc/doc/invoke.texi +1
-1@@ -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
Quoted 10 lines · Richard Sandiford
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.
Done.
Quoted 11 lines
Quoted 9 lines
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.
Gotcha. I guess any such combination, if used, will have to be dealt with by the CPU. All the binaries on my Octane, running an R14000 are -march=r10000 anyways, so when 4.4 final comes out and I start rebuilding stuff, I'll notice pretty fast if something is amiss.
Quoted 1 line
'Fraid so. ;) You had those right the first time.
Fixed!
Quoted 2 lines
Thanks. Haven't had time to look at the PR yet. Will be the weekend at the earliest now.
Yeah, it's an odd one. It forced me to use the early xgcc compiler to test my testcases, since I can't fully build the compiler just yet until this PR gets addressed.
Quoted 5 lines
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.
Okay, I think I got these right. There's 16 of them, fix-r10000-1.c through fix-r10000-16.c. Each file tests a single __sync_* function, and has the scan-assembler look for the 'beql' instruction in the output. I test compiled all of them by hand and checked their output with both -mno-fix-r10000 and -mfix-r10000, and a diff shows that exact instruction changing, so I think these all pass.
I couldn't tell which __sync_* function outputs the asm code found in MIPS_SYNC_NEW_OP and MIPS_SYNC_NEW_NAND, but other than that, the output all looked good.
I may not need fix-r10000-15.c, since that calls __sync_synchronize(), which doesn't output any branch-likely instructions at all. Wasn't sure where it's proper to call that function from (or if it even needs testing).
Quoted 3 lines
Minor nit, but I think the comment is easier to read if we keep the suggested blank line here.
[snip]
Quoted 1 line
You lost the "." at the end of the comment (coding conventions require it).
Fixed.
Quoted 5 lines
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.
Fixed, and added the documentation. If this looks good, then I'll cut a final w/ the changelog notations.
Thanks!
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
gcc/config/mips/mips.c +36 -1
@@ -6909,6 +6909,7 @@ mips_print_operand_reloc (FILE *file, rt '#' Print a nop if in a ".set noreorder" block. '/' Like '#', but do nothing within a delayed-branch sequence. '?' Print "l" if mips_branch_likely is true + '~' Print a nop if mips_branch_likely is true '.' Print the name of the register with a hard-wired zero (zero or $0). '@' Print the name of the assembler temporary register (at or $1). '^' Print the name of the pic call-through register (t9 or $25).
@@ -6983,6 +6984,11 @@ mips_print_operand_punctuation (FILE *fiputc ('l' , file);break ; +case '~' : +if (mips_branch_likely) +fputs ("\n\tnop" , file); +break ; +case '.' :fputs (reg_names[GP_REG_FIRST +0 ], file);break ;
@@ -7026,7 +7032,7 @@ mips_init_print_operand_punct (void) {const char *p; -for (p ="()[]<>*#/?.@^+$|-" ; *p; p++) +for (p ="()[]<>*#/?~.@^+$|-" ; *p; p++) mips_print_operand_punct[(unsigned char ) *p] = true; }
@@ -10250,6 +10256,17 @@ mips_output_order_conditional_branch (rtreturn mips_output_conditional_branch (insn, operands, branch[1 ], branch[0 ]); } +/* Return a template for the __sync_* functions after setting mips_branch_likely +to the value of TARGET_FIX_R10000 to enable a proper workaround of R10000 +errata. */ + +const char * +mips_output_sync_insn (const char *template) +{ + mips_branch_likely =TARGET_FIX_R10000 ; +return template; +} +/* Return the assembly code for DIV or DDIV instruction DIVISION, which has the operands given by OPERANDS. Add in a divide-by-zero check if needed.
@@ -13971,6 +13988,24 @@ mips_override_options (void) && mips_matching_cpu_name_p (mips_arch_info->name, "r4400")) target_flags |=MASK_FIX_R4400 ; +/* Default to working around R10000 errata only if the processor +was selected explicitly. */ +if ((target_flags_explicit &MASK_FIX_R10000 ) ==0 + &&mips_matching_cpu_name_p (mips_arch_info->name,"r10000" )) + target_flags |=MASK_FIX_R10000 ; + +/* Make sure that branch-likely instructions available when using +-mfix-r10000. The instructions are not available if either: + +1. -mno-branch-likely was passed. +2. The selected ISA does not support branch-likely and +the command line does not include -mbranch-likely. */ +if (TARGET_FIX_R10000 + && ((target_flags_explicit &MASK_BRANCHLIKELY ) ==0 + ? !ISA_HAS_BRANCHLIKELY + : !TARGET_BRANCHLIKEL )) +sorry ("%qs requires branch-likely instructions\n" ,"-mfix-r10000" ); +/* Save base state of options. */ mips_base_target_flags = target_flags; mips_base_delayed_branch = flag_delayed_branch;
gcc/config/mips/mips.h +13 -13
@@ -3089,7 +3089,7 @@ while (0)"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3114,7 +3114,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3134,7 +3134,7 @@ while (0)"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3159,7 +3159,7 @@ while (0)"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3192,7 +3192,7 @@ while (0)"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3222,7 +3222,7 @@ while (0)"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3242,7 +3242,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3259,7 +3259,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3276,7 +3276,7 @@ while (0)"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3295,7 +3295,7 @@ while (0)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3314,7 +3314,7 @@ while (0)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3332,7 +3332,7 @@ while (0)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3356,7 +3356,7 @@ while (0)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
gcc/config/mips/mips.opt +4 -0
@@ -112,6 +112,10 @@ mfix-r4400 Target Report Mask(FIX_R4400) Work around certain R4400 errata +mfix-r10000 +Target Report Mask(FIX_R10000) +Work around certain R10000 errata + mfix-sb1 Target Report Var(TARGET_FIX_SB1) Work around errata for early SB-1 revision 2 cores
gcc/config/mips/sync.md +51 -37
232 lines not on this page · raw message
gcc/doc/invoke.texi +17 -2
@@ -666,8 +666,9 @@ 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-vr4120 -mno-fix-vr4120 -mfix-vr4130 -mno-fix-vr4130 @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 -mbranch-cost=@var{num} -mbranch-likely -mno-branch-likely @gol
@@ -12827,6 +12828,20 @@ A double-word or a variable shift may gi immediately after starting an integer division. @end itemize +@item -mfix-r10000 +@itemx -mno-fix-r10000 +@opindex mfix-r10000 +@opindex mno-fix-r10000 +LL/SC workaround for R10000 errata in specific revisions of the CPU +silicon. Revs before 3.0 will misbehave on atomic operations, and revs +2.6 and below will deadlock due to other conflicting errata. + +This workaround only works for MIPS-II and above binaries due to the +use of branch-likely instructions (@code{beql}, @code{beqzl}). Although +a workaround exists for MIPS-I, it is not implemented by this flag. + +This flag is enabled by default when @code{-march=r10000} is used. + @item -mfix-vr4120 @itemx -mno-fix-vr4120 @opindex mfix-vr4120
gcc/testsuite/gcc.target/mips/fix-r10000-1.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_fetch_and_add (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-10.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_and_and_fetch (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-11.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_xor_and_fetch (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-12.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_nand_and_fetch (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-13.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_bool_compare_and_swap (&z,0 ,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-14.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_val_compare_and_swap (&z,0 ,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-15.c +9 -0
@@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +__sync_synchronize (); +}
gcc/testsuite/gcc.target/mips/fix-r10000-16.c +12 -0
@@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_lock_test_and_set (&z,42 ); +__sync_lock_release (&z); +}
gcc/testsuite/gcc.target/mips/fix-r10000-2.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_fetch_and_sub (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-3.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_fetch_and_or (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-4.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_fetch_and_and (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-5.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_fetch_and_xor (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-6.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_fetch_and_nand (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-7.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_add_and_fetch (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-8.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_sub_and_fetch (&z,42 ); +}
gcc/testsuite/gcc.target/mips/fix-r10000-9.c +11 -0
@@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler "beql" } } */ + +NOMIPS16 void +foo (void ) +{ +unsigned z =0 ; + +__sync_or_and_fetch (&z,42 ); +}
Hi,
Thanks for the patch.
Quoted 13 lines · Kumba
Quoted 5 lines
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.Okay, I think I got these right. There's 16 of them,
fix-r10000-1.cthroughfix-r10000-16.c. Each file tests a single __sync_* function, and has the scan-assembler look for the 'beql' instruction in the output. I test compiled all of them by hand and checked their output with both -mno-fix-r10000 and -mfix-r10000, and a diff shows that exact instruction changing, so I think these all pass.
Quoted 3 lines
I couldn't tell which __sync_* function outputs the asm code found in
MIPS_SYNC_NEW_OPandMIPS_SYNC_NEW_NAND, but other than that, the output all looked good.
__sync_<op>_and_fetch uses MIPS_SYNC_NEW_OP while __sync_fetch_and_<op> uses MIPS_SYNC_OLD_OP. So your tests did cover the necessary functions. The problem was that the tests ignored the return value, so __sync_<op>_and_fetch reduced to __sync_fetch_and_<op>.
Quoted 3 lines
I may not need
fix-r10000-15.c, since that calls__sync_synchronize(), which doesn't output any branch-likely instructions at all. Wasn't sure where it's proper to call that function from (or if it even needs testing).
Yeah, that test is wrong. __sync_synchronize isn't a loop (and isn't meant to be a loop) so the test rightly fails.
Quoted 2 lines
Fixed, and added the documentation. If this looks good, then I'll cut a final w/ the changelog notations.
Yeah, it looks generally good. I think we've got to the point where it's easier for me to make changes directly rather than ask you to follow a tortuous list of vaguely-described requests, so:
- I added a missing @gol after "-mfix-r4400".
- I tweaked the documentation so that it was more consistent with the other -mfix-* options. Let me know if you spot a problem with the new version, or if you aren't happy with it.
- I changed the name of the helper function from mips_output_sync_insn (my original suggestion) to mips_output_sync_loop, which seems a bit more descriptive. Sorry for going back on myself. Related...
- ...I changed the name of the parameter from TEMPLATE to LOOP to avoid a bootstrap-breaking warning about using a C++ identifier. (Again my fault. I'd used TEMPLATE when suggesting the function, but it was a completely untested suggestion.)
- I added a prototype to mips-protos.h, again to avoid a bootstrap- breaking warning.
- I removed the "\n" from the sorry message (my fault again).
- I fixed a typo: s/!TARGET_BRANCHLIKEL/!TARGET_BRANCHLIKELY/. GCC wouldn't build without this, so perhaps the posted patch wasn't the final one.
- I made the tests check for "\tbeql\t", which is a bit more robust than plain "beql". (OK, it's not likely to make a difference for this particular example, but it seems like good practice.)
- I changed the tests to operate on caller-provided memory. The old versions operated on automatic variables that obviously couldn't escape, sp I was worried they might be optimised in future.
- I made the subtraction tests take the subtrahend as an argument, because subtracting 42 reduces to adding -42.
- I added "short" and "char" versions of each test function.
- I made the test functions return the result of the __sync_* builtin, for the reasons given above.
- As discussed above, I removed the original test 15 and
renamed 16 to 15.
- I tweaked the formatting in a couple of places, but nothing major.
Applied with those changes. I've attached the final changelog and patch below. Thanks for the contribution, and for your patience.
Richard
gcc/
2008-11-15 Joshua Kinard <[email protected]>
* doc/invoke.texi (-mfix-r10000): Document. * config/mips/mips.opt (mfix-r10000): New option. * config/mips/mips-protos.h (mips_output_sync_loop): Declare. * config/mips/mips.h (MIPS_COMPARE_AND_SWAP): Use %?. (MIPS_COMPARE_AND_SWAP_12): Likewise. (MIPS_SYNC_OP): Likewise. (MIPS_SYNC_OP_12): Likewise. (MIPS_SYNC_OLD_OP_12): Likewise. (MIPS_SYNC_NEW_OP_12): Likewise. (MIPS_SYNC_OLD_OP): Likewise. (MIPS_SYNC_NAND): Likewise. (MIPS_SYNC_OLD_NAND): Likewise. (MIPS_SYNC_EXCHANGE): Likewise. (MIPS_SYNC_EXCHANGE_12): Likewise. (MIPS_SYNC_NEW_OP): Likewise, using %~ to fill branch-likely delay slots. (MIPS_SYNC_NEW_NAND): Likewise. * config/mips/mips.c (mips_print_operand_punctuation): Handle '~'. (mips_init_print_operand_punct): Treat '~' as a punctuation character. (mips_output_sync_loop): New function. (mips_override_options): Make -march=r10000 imply -mfix-r10000. Make -mfix-r10000 require branch-likely instructions. * config/mips/sync.md (sync_compare_and_swap<mode>): Use mips_output_sync_loop. (compare_and_swap_12): Likewise. (sync_add<mode>): Likewise. (sync_<optab>_12): Likewise. (sync_old_<optab>_12): Likewise. (sync_new_<optab>_12): Likewise. (sync_nand_12): Likewise. (sync_old_nand_12): Likewise. (sync_new_nand_12): Likewise. (sync_sub<mode>): Likewise. (sync_old_add<mode>): Likewise. (sync_old_sub<mode>): Likewise. (sync_new_add<mode>): Likewise. (sync_new_sub<mode>): Likewise. (sync_<optab><mode>): Likewise. (sync_old_<optab><mode>): Likewise. (sync_new_<optab><mode>): Likewise. (sync_nand<mode>): Likewise. (sync_old_nand<mode>): Likewise. (sync_new_nand<mode>): Likewise. (sync_lock_test_and_set<mode>): Likewise. (test_and_set_12): Likewise.
gcc/testsuite/
2008-11-15 Joshua Kinard <[email protected]>
Richard Sandiford <[email protected]>
* gcc.target/mips/fix-r10000-1.c: New test.
* gcc.target/mips/fix-r10000-2.c: Likewise.
* gcc.target/mips/fix-r10000-3.c: Likewise.
* gcc.target/mips/fix-r10000-4.c: Likewise.
* gcc.target/mips/fix-r10000-5.c: Likewise.
* gcc.target/mips/fix-r10000-6.c: Likewise.
* gcc.target/mips/fix-r10000-7.c: Likewise.
* gcc.target/mips/fix-r10000-8.c: Likewise.
* gcc.target/mips/fix-r10000-9.c: Likewise.
* gcc.target/mips/fix-r10000-10.c: Likewise.
* gcc.target/mips/fix-r10000-11.c: Likewise.
* gcc.target/mips/fix-r10000-12.c: Likewise.
* gcc.target/mips/fix-r10000-13.c: Likewise.
* gcc.target/mips/fix-r10000-14.c: Likewise.
* gcc.target/mips/fix-r10000-15.c: Likewise.
doc/invoke.texi +18 -2
@@ -668,8 +668,8 @@ Objective-C and Objective-C++ Dialects}. -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-vr4120 -mno-fix-vr4120 -mfix-vr4130 -mno-fix-vr4130 @gol --mfix-sb1 -mno-fix-sb1 @gol +-mfix-r10000 -mno-fix-r10000 -mfix-vr4120 -mno-fix-vr4120 @gol +-mfix-vr4130 -mno-fix-vr4130 -mfix-sb1 -mno-fix-sb1 @gol -mflush-func=@var{func} -mno-flush-func @gol -mbranch-cost=@var{num} -mbranch-likely -mno-branch-likely @gol -mfp-exceptions -mno-fp-exceptions @gol
@@ -12833,6 +12833,22 @@ A double-word or a variable shift may gi immediately after starting an integer division. @end itemize +@item -mfix-r10000 +@itemx -mno-fix-r10000 +@opindex mfix-r10000 +@opindex mno-fix-r10000 +Work around certain R10000 errata: +@itemize @minus +@item +@code{ll}/@code{sc} sequences may not behave atomically on revisions +prior to 3.0. They may deadlock on revisions 2.6 and earlier. +@end itemize + +This option can only be used if the target architecture supports +branch-likely instructions. @option{-mfix-r10000} is the default when +@option{-march=r10000} is used; @option{-mno-fix-r10000} is the default +otherwise. + @item -mfix-vr4120 @itemx -mno-fix-vr4120 @opindex mfix-vr4120
config/mips/mips.opt +4 -0
@@ -112,6 +112,10 @@ mfix-r4400 Target Report Mask(FIX_R4400) Work around certain R4400 errata +mfix-r10000 +Target Report Mask(FIX_R10000) +Work around certain R10000 errata + mfix-sb1 Target Report Var(TARGET_FIX_SB1) Work around errata for early SB-1 revision 2 cores
config/mips/mips-protos.h +1 -0
@@ -300,6 +300,7 @@ extern const char *mips_output_load_labeextern const char *mips_output_conditional_branch (rtx ,rtx *,const char *,const char *);extern const char *mips_output_order_conditional_branch (rtx ,rtx *,bool ); +extern const char *mips_output_sync_loop (const char *);extern const char *mips_output_division (const char *,rtx *);extern unsigned int mips_hard_regno_nregs (int ,enum machine_mode );extern bool mips_linked_madd_p (rtx ,rtx );
config/mips/mips.h +13 -13
@@ -3090,7 +3090,7 @@ #define MIPS_COMPARE_AND_SWAP(SUFFIX, OP"\tbne\t%0,%z2,2f\n" \"\t" OP "\t%@,%3\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3115,7 +3115,7 @@ #define MIPS_COMPARE_AND_SWAP_12(OPS) \"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)\n" \"2:\n"
@@ -3135,7 +3135,7 @@ #define MIPS_SYNC_OP(SUFFIX, INSN) \"1:\tll" SUFFIX "\t%@,%0\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3160,7 +3160,7 @@ #define MIPS_SYNC_OP_12(INSN, NOT_OP) \"\tand\t%4,%4,%1\n" \"\tor\t%@,%@,%4\n" \"\tsc\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3193,7 +3193,7 @@ #define MIPS_SYNC_OLD_OP_12(INSN, NOT_OP"\tand\t%5,%5,%2\n" \"\tor\t%@,%@,%5\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3223,7 +3223,7 @@ #define MIPS_SYNC_NEW_OP_12(INSN, NOT_OP"\tand\t%0,%0,%2\n" \"\tor\t%@,%@,%0\n" \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3243,7 +3243,7 @@ #define MIPS_SYNC_OLD_OP(SUFFIX, INSN)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3260,7 +3260,7 @@ #define MIPS_SYNC_NEW_OP(SUFFIX, INSN)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3277,7 +3277,7 @@ #define MIPS_SYNC_NAND(SUFFIX, INSN) \"\tnor\t%@,%@,%.\n" \"\t" INSN "\t%@,%@,%1\n" \"\tsc" SUFFIX "\t%@,%0\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3296,7 +3296,7 @@ #define MIPS_SYNC_OLD_NAND(SUFFIX, INSN)"\tnor\t%@,%0,%.\n" \"\t" INSN "\t%@,%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3315,7 +3315,7 @@ #define MIPS_SYNC_NEW_NAND(SUFFIX, INSN)"\tnor\t%0,%0,%.\n" \"\t" INSN "\t%@,%0,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b%~\n" \"\t" INSN "\t%0,%0,%2\n" \"\tsync%-%]%>%)"
@@ -3333,7 +3333,7 @@ #define MIPS_SYNC_EXCHANGE(SUFFIX, OP)"1:\tll" SUFFIX "\t%0,%1\n" \"\t" OP "\t%@,%2\n" \"\tsc" SUFFIX "\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
@@ -3357,7 +3357,7 @@ #define MIPS_SYNC_EXCHANGE_12(OPS)"\tand\t%@,%0,%3\n" \OPS \"\tsc\t%@,%1\n" \ -"\tbeq\t%@,%.,1b\n" \ +"\tbeq%?\t%@,%.,1b\n" \"\tnop\n" \"\tsync%-%]%>%)"
config/mips/mips.c +36 -1
@@ -6909,6 +6909,7 @@ mips_print_operand_reloc (FILE *file, rt '#' Print a nop if in a ".set noreorder" block. '/' Like '#', but do nothing within a delayed-branch sequence. '?' Print "l" if mips_branch_likely is true + '~' Print a nop if mips_branch_likely is true '.' Print the name of the register with a hard-wired zero (zero or $0). '@' Print the name of the assembler temporary register (at or $1). '^' Print the name of the pic call-through register (t9 or $25).
@@ -6983,6 +6984,11 @@ mips_print_operand_punctuation (FILE *fiputc ('l' , file);break ; +case '~' : +if (mips_branch_likely) +fputs ("\n\tnop" , file); +break ; +case '.' :fputs (reg_names[GP_REG_FIRST +0 ], file);break ;
@@ -7026,7 +7032,7 @@ mips_init_print_operand_punct (void) {const char *p; -for (p ="()[]<>*#/?.@^+$|-" ; *p; p++) +for (p ="()[]<>*#/?~.@^+$|-" ; *p; p++) mips_print_operand_punct[(unsigned char ) *p] = true; }
@@ -10250,6 +10256,17 @@ mips_output_order_conditional_branch (rtreturn mips_output_conditional_branch (insn, operands, branch[1 ], branch[0 ]); } +/* Return the assembly code for __sync_*() loop LOOP. The loop should support +both normal and likely branches, using %? and %~ where appropriate. */ + +const char * +mips_output_sync_loop (const char *loop) +{ +/* Use branch-likely instructions to work around the LL/SC R10000 errata. */ + mips_branch_likely =TARGET_FIX_R10000 ; +return loop; +} +/* Return the assembly code for DIV or DDIV instruction DIVISION, which has the operands given by OPERANDS. Add in a divide-by-zero check if needed.
@@ -13971,6 +13988,24 @@ mips_override_options (void) && mips_matching_cpu_name_p (mips_arch_info->name, "r4400")) target_flags |=MASK_FIX_R4400 ; +/* Default to working around R10000 errata only if the processor +was selected explicitly. */ +if ((target_flags_explicit &MASK_FIX_R10000 ) ==0 + &&mips_matching_cpu_name_p (mips_arch_info->name,"r10000" )) + target_flags |=MASK_FIX_R10000 ; + +/* Make sure that branch-likely instructions available when using +-mfix-r10000. The instructions are not available if either: + +1. -mno-branch-likely was passed. +2. The selected ISA does not support branch-likely and +the command line does not include -mbranch-likely. */ +if (TARGET_FIX_R10000 + && ((target_flags_explicit &MASK_BRANCHLIKELY ) ==0 + ? !ISA_HAS_BRANCHLIKELY + : !TARGET_BRANCHLIKELY )) +sorry ("%qs requires branch-likely instructions" ,"-mfix-r10000" ); +/* Save base state of options. */ mips_base_target_flags = target_flags; mips_base_delayed_branch = flag_delayed_branch;
config/mips/sync.md +49 -37
230 lines not on this page · raw message
testsuite/gcc.target/mips/fix-r10000-1.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_fetch_and_add (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_fetch_and_add (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_fetch_and_add (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-2.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z, int amt) +{ +return __sync_fetch_and_sub (z, amt); +} + +NOMIPS16 short +f2 (short *z,short amt) +{ +return __sync_fetch_and_sub (z, amt); +} + +NOMIPS16 char +f3 (char *z, char amt) +{ +return __sync_fetch_and_sub (z, amt); +}
testsuite/gcc.target/mips/fix-r10000-3.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_fetch_and_or (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_fetch_and_or (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_fetch_and_or (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-4.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_fetch_and_and (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_fetch_and_and (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_fetch_and_and (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-5.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_fetch_and_xor (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_fetch_and_xor (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_fetch_and_xor (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-6.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_fetch_and_nand (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_fetch_and_nand (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_fetch_and_nand (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-7.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_add_and_fetch (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_add_and_fetch (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_add_and_fetch (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-8.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z, int amt) +{ +return __sync_sub_and_fetch (z, amt); +} + +NOMIPS16 short +f2 (short *z,short amt) +{ +return __sync_sub_and_fetch (z, amt); +} + +NOMIPS16 char +f3 (char *z, char amt) +{ +return __sync_sub_and_fetch (z, amt); +}
testsuite/gcc.target/mips/fix-r10000-9.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_or_and_fetch (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_or_and_fetch (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_or_and_fetch (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-10.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_and_and_fetch (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_and_and_fetch (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_and_and_fetch (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-11.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_xor_and_fetch (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_xor_and_fetch (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_xor_and_fetch (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-12.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_nand_and_fetch (z,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_nand_and_fetch (z,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_nand_and_fetch (z,42 ); +}
testsuite/gcc.target/mips/fix-r10000-13.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_bool_compare_and_swap (z,0 ,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_bool_compare_and_swap (z,0 ,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_bool_compare_and_swap (z,0 ,42 ); +}
testsuite/gcc.target/mips/fix-r10000-14.c new file +21 -0
@@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +return __sync_val_compare_and_swap (z,0 ,42 ); +} + +NOMIPS16 short +f2 (short *z) +{ +return __sync_val_compare_and_swap (z,0 ,42 ); +} + +NOMIPS16 char +f3 (char *z) +{ +return __sync_val_compare_and_swap (z,0 ,42 ); +}
testsuite/gcc.target/mips/fix-r10000-15.c new file +33 -0
@@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-mips-options "-O2 -march=mips4 -mfix-r10000" } */ +/* { dg-final { scan-assembler-times "\tbeql\t" 3 } } */ + +NOMIPS16 int +f1 (int *z) +{ +int result; + + result =__sync_lock_test_and_set (z,42 ); +__sync_lock_release (z); +return result; +} + +NOMIPS16 short +f2 (short *z) +{ +short result; + + result =__sync_lock_test_and_set (z,42 ); +__sync_lock_release (z); +return result; +} + +NOMIPS16 char +f3 (char *z) +{ +char result; + + result =__sync_lock_test_and_set (z,42 ); +__sync_lock_release (z); +return result; +}
Quoted 4 lines · Richard Sandiford
Yeah, it looks generally good. I think we've got to the point where it's easier for me to make changes directly rather than ask you to follow a tortuous list of vaguely-described requests, so:
Tortuous isn't the word I'd use. More like fun and challenging! Still a ways to go understand things, but it's getting easier to walk around in the mips backend of gcc and have an idea of what's going on somewhat.
Thanks for answering all my questions!
Quoted 1 line
- I added a missing @gol after "-mfix-r4400".
Yeah, I wasn't sure if I needed that or not. I was building the info pages and comparing the output, and noticed a somewhat logical order where alike arguments were grouped onto their own line (i.e., the fix-4000 and fix-4400 were separated from the fix-sb1 and fix-vr* args). Don't know Texi code at all, so I thought omitting the @gol would allow the patch to stay in the 80-char limit, but allow the info output to place fix-r10000 on the same line as the 4000/4400 args, since it appears all three were better related (in terms of implementation) than the sb1 and vr* args.
Quoted 3 lines
- I tweaked the documentation so that it was more consistent with the other -mfix-* options. Let me know if you spot a problem with the new version, or if you aren't happy with it.
Looks good to me. I added the bit about MIPS-II stuff and mentioning it won't work on MIPS-I, but wasn't sure if that level of detail was necessary.
Quoted 11 lines
- ...I changed the name of the parameter from TEMPLATE to LOOP to avoid a bootstrap-breaking warning about using a C++ identifier. (Again my fault. I'd used TEMPLATE when suggesting the function, but it was a completely untested suggestion.)
- I added a prototype to
mips-protos.h, again to avoid a bootstrap- breaking warning.- I fixed a typo: s/!TARGET_BRANCHLIKEL/!TARGET_BRANCHLIKELY/. GCC wouldn't build without this, so perhaps the posted patch wasn't the final one.
Ah, I probably would have caught these but PR38052 was getting in the way of a full build. Although, building as a cross-compiler might've avoided that bug.
Quoted 2 lines
Applied with those changes. I've attached the final changelog and patch below. Thanks for the contribution, and for your patience.
Looks good to me. This'll let me play with the glibc-side of things next. I think I did stumble on one or two omissions from my R10000 scheduling patch I'll send over in a separate e-mail (it's a one-line fix I believe).
Thanks again!
Signature · 7 lines
Joshua Kinard Gentoo/MIPS [email protected] "The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between." --Emperor Turhan, Centauri Republic
Quoted 4 lines · Kumba
The branch-likely stuff is only going to work for MIPS-II or higher targets. In the odd (but possible) cases where MIPS-I might be used with -mfix-r10000, I assume we'll still have to emit 28 nops prior to a beq/beqz instruction. Is this already taken care of someplace?
MIPS I does not support LL/SC, so that case is not to be concerned.
These instructions are emulated on such processors and as such can appear in assembly snippets, but GCC is expected not to emit them itself (I hope it hasn't changed; if it has, then we are in trouble -- I suppose a compiler error is justified for such an odd case).
Quoted 16 lines
Hmm, okay. Might this work to enable -mbranch-likely if -mfix-r10000? (Kind of guessing by looking at other segments of code).
if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0) { if (ISA_HAS_BRANCHLIKELY && (optimize_size || (!(target_flags_explicit & MASK_FIX_R10000) == 0) || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0)) target_flags |= MASK_BRANCHLIKELY; else target_flags &= ~MASK_BRANCHLIKELY; }
Quoted 7 lines
My understanding so far for -mfix-r10000:
- Gets enabled if -march=r10000 is passed (done)
- Enable -mbranch-likely if not already enabled on >= MIPS-II (working on)
- Emits beqzl in the asm templates if enabled and >= MIPS-II (unsure)
- Emits 28 nops prior to beq/beqz if enabled and == MIPS-I (unsure)
- Ditto for asm templates (unsure)
- Documentation (not done)
I think the best option is to leave -mbranch-likely intact and bail out
if -mfix-r10000 and -mno-branch-likely are passed at the same time. Anything else is likely to cause confusion. Then -march=r10000 should enable both -mfix-r10000 and -mbranch-likely.
I believe (but have not checked) that all CPUs/ISAs that are within the
MIPS II - MIPS IV range enable -mbranch-likely by default, so there is no problem here unless somebody requests -mno-branch-likely in which case they get what they asked for (i.e. a compiler error). I believe all MIPS architecture CPUs/ISAs disable -mbranch-likely by default, but such code cannot run on the R10k anyway.
I think it covers all cases. Comments?
Maciej
Quoted 11 lines · Maciej W. Rozycki
Quoted 4 lines · Kumba
The branch-likely stuff is only going to work for MIPS-II or higher targets. In the odd (but possible) cases where MIPS-I might be used with -mfix-r10000, I assume we'll still have to emit 28 nops prior to a beq/beqz instruction. Is this already taken care of someplace?
MIPS I does not support LL/SC, so that case is not to be concerned.
These instructions are emulated on such processors and as such can appear in assembly snippets, but GCC is expected not to emit them itself (I hope it hasn't changed; if it has, then we are in trouble -- I suppose a compiler error is justified for such an odd case).
Not quite true. Use of LL and SC is controlled by a separate option, -mllsc. As you'd expect, this option is on by default if the architecture provides LL and SC. It's therefore on by default for generic MIPS II and above, and for -march=<cpu> if <cpu> provides the instructions.
However, the default for other architectures depends on the configuration. If the configured target is known to emulate the LL and SC instructions (as Linux targets are), the default is -mllsc. It is -mno-llsc otherwise.
Quoted 29 lines
Quoted 16 lines
Hmm, okay. Might this work to enable -mbranch-likely if -mfix-r10000? (Kind of guessing by looking at other segments of code).
if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0) { if (ISA_HAS_BRANCHLIKELY && (optimize_size || (!(target_flags_explicit & MASK_FIX_R10000) == 0) || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0)) target_flags |= MASK_BRANCHLIKELY; else target_flags &= ~MASK_BRANCHLIKELY; }Quoted 7 lines
My understanding so far for -mfix-r10000:
- Gets enabled if -march=r10000 is passed (done)
- Enable -mbranch-likely if not already enabled on >= MIPS-II (working on)
- Emits beqzl in the asm templates if enabled and >= MIPS-II (unsure)
- Emits 28 nops prior to beq/beqz if enabled and == MIPS-I (unsure)
- Ditto for asm templates (unsure)
- Documentation (not done)I think the best option is to leave -mbranch-likely intact and bail out
if -mfix-r10000 and -mno-branch-likely are passed at the same time. Anything else is likely to cause confusion. Then -march=r10000 should enable both -mfix-r10000 and -mbranch-likely.
Quoted 8 lines
I believe (but have not checked) that all CPUs/ISAs that are within the
MIPS II - MIPS IV range enable -mbranch-likely by default, so there is no problem here unless somebody requests -mno-branch-likely in which case they get what they asked for (i.e. a compiler error). I believe all MIPS architecture CPUs/ISAs disable -mbranch-likely by default, but such code cannot run on the R10k anyway.I think it covers all cases. Comments?
For avoidance of doubt, this is exactly option #1 in my messages. As you say, -march=r10000 already implies -mbranch-likely unless explicitly overridden.
Richard
Quoted 4 lines
From: Maciej W. Rozycki [mailto:[email protected]]
I believe (but have not checked) that all CPUs/ISAs that are within the MIPS II - MIPS IV range enable -mbranch-likely by default,
Not quite. sb1 has no-branch-likely. It actually does implement the instruction but the documentation clearly states that it should be avoided.
paul
Quoted 6 lines · On Mon, 3 Nov 2008, [email protected] wrote:
Quoted 2 lines
I believe (but have not checked) that all CPUs/ISAs that are within the MIPS II - MIPS IV range enable -mbranch-likely by default,
Not quite. sb1 has no-branch-likely. It actually does implement the instruction but the documentation clearly states that it should be avoided.
Well, the SB-1 is a MIPS architecture processor (a MIPS64 one to be
exact) and as such not within the MIPS II - MIPS IV ISA range and what you say by definition stands for any other MIPS architecture implementation too. This is not relevant though as MIPS64 code won't run on a MIPS IV processor such as the R10k anyway.
Maciej
Quoted 4 lines · On Mon, Nov 03, 2008 at 10:51:49AM -0600, [email protected] wrote:
Quoted 2 lines
I believe (but have not checked) that all CPUs/ISAs that are within the MIPS II - MIPS IV range enable -mbranch-likely by default,
Not quite. sb1 has no-branch-likely. It actually does implement the instruction but the documentation clearly states that it should be avoided.
For the usual reasons - the CPU micro architects hate branch likely. It means having to cancel an instruction from the pipeline rather late, if the branch was not taken. So the deprecation just expresses the desparate wish of the CPU architects to get rid of the instruction. In practice that won't happen any time soon - we even still have useless crap like signed add and sub instructions in the MIPS ISA. And if the instructions actually were removed we still could software emulate them in the kernel.
However unlike the R10000 the SB1 and SB1 implement full blown branch prediction for branch likely so use it where you can for performance.
Ralf
Quoted 7 lines · Richard Sandiford
There are two ways we could handle this:
- Make -mfix-r10000 require -mbranch-likely. (It mustn't _imply_ -mbranch-likely. It should simply check that -mbranch-likely is already in effect.)
- Make -mfix-r10000 insert nops when -mbranch-likely is not in effect.
If I recall right, these is something special about the pipeline in this
context making the branch-likely instructions the only ones that work. Which would make the option you proposed first the only viable. I am not absolutely sure and I have no reference handy. Perhaps Ralf or someone at linux-mips will know?
Maciej
Quoted 6 lines · Maciej W. Rozycki
Quoted 6 lines
There are two ways we could handle this:
- Make -mfix-r10000 require -mbranch-likely. (It mustn't _imply_ -mbranch-likely. It should simply check that -mbranch-likely is already in effect.)
Quoted 7 lines
Quoted 1 line
- Make -mfix-r10000 insert nops when -mbranch-likely is not in effect.
If I recall right, these is something special about the pipeline in this
context making the branch-likely instructions the only ones that work. Which would make the option you proposed first the only viable. I am not absolutely sure and I have no reference handy. Perhaps Ralf or someone at linux-mips will know?
There are two possible workarounds. The other which IRIX and the Linux kernel are using is based on the branch-likely instruction. The way it works is that R10000 family processors have a fairly cheesy branch prediction for branch likely (unlike all MIPS32 and MIPS64 processors I know of!) which predicts branch likely instructions as always taken. So if a SC instruction succeeds the loop closure branch of the usual LL/SC loop will be miss-predicted and the pipeline restarted.
The alternative is to put enough NOPs (upto 28) after the loop closure brach to avoid a sequence of 4 problematic instructions being active in the pipeline at the same time.
SSNOP won't cut it btw. SSNOP don't have any influence on the predecode and reordering buffers - even assuming the R10000 actually honors SSNOP. Implementing the special treatment of SSNOP (which is encoded as SLL $0, $0, 1) just doesn't make sense for an R10000 calibre processor.
Is gcc capable of guaranteeing a certain minimum number of instructions between one LL and another LL instruction? Then this knowledge could be used to avoid the branch likely or cut down the padding NOPs.
Ralf
Quoted 2 lines · Kumba
Feedback would be welcome on any suggestions for improving this patch (please CC, as I'm not subscribed to the ML).
Please make it run-time selectable, aggregating all the workarounds for
the R10000 under -mfix-r10000, similarly to how it is done for other silicon errata. You probably want it on by default for -march=r10000 (but not necessarily -march=r12000). You need to supply a suitable documentation update too.
Maciej