This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH]: R10000 Needs LL/SC Workaround in Gcc
Kumba <kumba@gentoo.org> writes:
> Richard Sandiford wrote:
>> 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;
>> }
>>
>
> 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.
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.
> 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.
> 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.o
> build/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?
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.
> @@ -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. */
> + 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...
> @@ -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.)
> @@ -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:
> @@ -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