This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] microMIPS/GAS: Correct the handling of hazard clearance NOPs
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: "Maciej W. Rozycki" <macro at codesourcery dot com>
- Cc: binutils at sourceware dot org, Chao-ying Fu <fu at mips dot com>, Rich Fuhler <rich at mips dot com>, David Lau <davidlau at mips dot com>, Kevin Mills <kevinm at mips dot com>, Ilie Garbacea <ilie at mips dot com>, Catherine Moore <clm at codesourcery dot com>, Nathan Sidwell <nathan at codesourcery dot com>, Joseph Myers <joseph at codesourcery dot com>
- Date: Tue, 09 Aug 2011 12:35:26 +0100
- Subject: Re: [PATCH] microMIPS/GAS: Correct the handling of hazard clearance NOPs
- References: <alpine.DEB.1.10.1108021611410.4083@tp.orcam.me.uk> <877h6qrh69.fsf@firetop.home> <alpine.DEB.1.10.1108090017080.4083@tp.orcam.me.uk>
"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> On Sat, 6 Aug 2011, Richard Sandiford wrote:
>> These sorts of test are becoming a maintenance and readability problem,
>> so it seemed like a good time to abstract them away. Perhaps the only
>> noteworthy part is that the microMIPS relaxation code tested:
>>
>> || (pinfo2 & ~INSN2_ALIAS) == INSN2_UNCOND_BRANCH
>>
>> instead of plain:
>>
>> || (pinfo2 & INSN2_UNCOND_BRANCH)
>>
>> The first form excludes JRC and JRADDIUSP, which should of course not be
>> relaxed. But those instructions wouldn't have address expressions or
>> fixups anyway, so both conditions ought to be equivalent.
>
> Yes, fair enough as it stands, but things may change and then someone
> will scratch their head ten years on or suchlike -- as we do about 1990s
> stuff these days sometimes.
>
> How about adding a comment to the effect of your explanation above where
> you've removed the ~INSN2_ALIAS mask -- ...
I think it's the other way round. The INSN2_ALIAS check was far from
obvious. When I first saw it, I wondered why we were excluding branches
that read from registers (since we have plenty of INSN2_READ_*s). Then I
realised that we weren't trying to exclude that as such; we were trying
to exclude indirect jumps. But JR is INSN2_UNCOND_BRANCH_DELAY for all
of normal MIPS, MIPS16 and microMIPS, and has never had (or needed)
special checks in the relaxation code. AFAIK, noone has every been
confused by that. So...
>> @@ -4198,16 +4224,12 @@ append_insn (struct mips_cl_insn *ip, ex
>> && address_expr
>> && ((relax32 && *reloc_type == BFD_RELOC_16_PCREL_S2)
>> || *reloc_type > BFD_RELOC_UNUSED)
>> - && (pinfo & INSN_UNCOND_BRANCH_DELAY
>> - || pinfo & INSN_COND_BRANCH_DELAY
>> - || (pinfo2 & ~INSN2_ALIAS) == INSN2_UNCOND_BRANCH
>> - || pinfo2 & INSN2_COND_BRANCH))
>> + && (delayed_branch_p (ip) || compact_branch_p (ip)))
>> {
>> bfd_boolean relax16 = *reloc_type > BFD_RELOC_UNUSED;
>> int type = relax16 ? *reloc_type - BFD_RELOC_UNUSED : 0;
>
> ... specifically here?
no, I don't think that's a good idea. The new form brings the microMIPS
code in line with the normal MIPS and MIPS16 code, and is IMO what a
reader is likely to expect.
Richard