This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] microMIPS/GAS: Correct the handling of hazard clearance NOPs

"Maciej W. Rozycki" <> 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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]