Re: [PATCH] MIPS: microMIPS ASE support

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> I think we're getting close, so could you post any updates as patches
>> relative to this one, rather than reposting the whole thing?
>
>  Yes, I think it will make it easier for us both to keep track of what has 
> been addressed and what not.  I have no technical problem with including 
> further changes in a separate patch.

Thanks.  For avoidance of doubt, please make any updates (with the FIXMEs
fixed, for instance), relative to the original.

>> Formatting nit:
>> 
>>       /* If the output section is the PLT section,
>>          then the target is not microMIPS.  */
>>       target_is_micromips_code_p = (htab->splt != sec
>> 				    && ELF_ST_IS_MICROMIPS (h->root.other));
>> 
>> More importantly, the comment isn't any help.  When do we create
>> statically-resolved relocations against .plt?

Oops, ignore this, I was thinking "sec" was the section that contained
the relocation.

>> >    /* Calls from 16-bit code to 32-bit code and vice versa require the
>> > -     mode change.  */
>> > -  *cross_mode_jump_p = !info->relocatable
>> > -		       && ((r_type == R_MIPS16_26 && !target_is_16_bit_code_p)
>> > -			   || ((r_type == R_MIPS_26 || r_type == R_MIPS_JALR)
>> > -			       && target_is_16_bit_code_p));
>> > +     mode change.  This is not required for calls to undefined weak
>> > +     symbols, which should never be executed at runtime.  */
>> 
>> But why do we need to go out of our way to check for them?  I'm sure
>> there's a good reason, but the comment doesn't give much clue what it is.
>
>  Undefined weak symbols are, well, undefined, so they have resolved to nil 
> and are meant never to be jumped to, so we don't want to error out on them 
> just because they do not have the ISA bit set and a JALX therefore 
> required could not be used for some reason, like the invocation being a 
> sibling call or because it would not satisfy the fixed delay slot 
> dependency.

OK, makes sense.  Please update the comment to something like:

  /* Calls from 16-bit code to 32-bit code and vice versa require the
     mode change.  However, we can ignore calls to undefined weak symbols,
     which should never be executed at runtime.  This exception is important
     because the assembly writer may have "known" that any definition of the
     symbol would be 16-bit code, and that direct jumps were therefore
     acceptable.  */

>> At least, that's what the code seems to do, since it returns 2 for
>> things like:
>> 
>> > +  { /* "b(g|l)(e|t)z", "s,p",	*/ 0x40000000, 0xff200000 },
>> 
>> which as far as I can tell from the opcodes patch allows both 16-bit
>> and 32-bit delay slots.  (IV-g doesn't seem to be public yet, so I can't
>> check the spec.)  But from the way the function is used, it looks
>> like 0 really means "no size constraints", so why doesn't the function
>> return 0 for instructions like these?
>
>  Only link variations of branches and jumps have a fixed-size delay slot 
> -- that's because the link register is set to a fixed offset from the 
> delay-slot instruction (either four as with JAL or two as with JALS).  Of 
> all such jumps and branches only JALX does not have a JALXS counterpart 
> (regrettably, as it would have made life of software much, much easier).
>
>  I've explained the meaning of 0 below -- it's unsafe to return this value 
> for a variable-size delay slot.

Hmm, I was thinking of the case where there was no branch _after_
the LUI, and where the instruction after the LUI could then become
the delay slot for a variable-length branch before the (deleted) LUI.
But yeah, I can see that 0 isn't correct if there is a branch immediately
after the LUI.

>> I'm not sure whether I like the idea of making this function quadratic
>> simply to decide whether the preceding instruction is a branch,
>> but there's probably not much we can do about that.
>
>  Hmm, we could do this in two passes over the reloc table (still O(n)) and 
> first make a fast auxiliary data structure (e.g. a hash) to keep addresses 
> of branch and jump relocations.  Given linker relaxation is off by default 
> I'd vote for this as a future improvement.

Yeah, I'd wondered about that, but then we'd need to measure whether that
made things better or not.  I'm OK with leaving it.

Richard