Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support

[Richard Sandiford <rdsandiford@googlemail.com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> Generally looks good.  It would have been better to submit the
>> ACE, m14kc and microMIPS support as three separate changes though.
>> Please can you do that for the follow-up?
>
>  I have done it now; the next version will comprise microMIPS ASE changes 
> only (the MCU ASE adds to both the MIPS ISA and the microMIPS ASE, so it 
> should be applied on top of the microMIPS change).

Thanks.  Thanks too for all the issues that you've addressed already,
and for the ones you said you'd sort out.  Sounds like good progress!

>  Stricly speaking some changes, while related, can be split off too (e.g. 
> some MIPS16 or testsuite changes), so I'll look into separating them too 
> -- perhaps that'll make the thing rolling sooner.

Yeah, thanks, that'd be a help if it's easy to do.  It doesn't matter
too much, though.  Most of the MIPS16 changes were consistent with the
microMIPS ones, so were easy to review as a unit.

>> From a usability perspective, it's a shame that:
>> 
>> 	.set	micromips
>> 	.ent	foo
>> foo:
>> 	b	1f
>> 	nop
>> 	.end	foo
>> 	.ent	bar
>> bar:
>> 1:	nop
>> 	.end	bar
>> 
>> disassembles as:
>> 
>> 00000000 <foo>:
>>    0:   cfff            b       0 <foo>
>>                         0: R_MICROMIPS_PC10_S1  .L11
>>    2:   0c00            nop
>>    4:   0c00            nop
>> 
>> 00000006 <bar>:
>>    6:   0c00            nop
>> 
>> leaving the poor user with no idea what .L11 is.
>
>  Indeed.  This is a general limitation of `objdump' it would seem.  This 
> is no different to what you get with:
>
> $ cat b.s
> 	.globl	baz
> 	.ent	foo
> foo:
> 	b	baz
> 	nop
> 	.end	foo
> 	.ent	bar
> baz:
> bar:
> 1:	nop
> 	.end	bar
> $ mips-sde-elf-objdump -dr b.o
>
> b.o:     file format elf32-tradbigmips
>
>
> Disassembly of section .text:
>
> 00000000 <foo>:
>    0:	1000ffff 	b	0 <foo>
> 			0: R_MIPS_PC16	baz
>    4:	00000000 	nop
>    8:	00000000 	nop
>
> 0000000c <bar>:
>    c:	00000000 	nop

Well, it's a little different.  The user has at least defined two
named symbols in this case, so they have a good chance of knowing
what "baz" means.  In the microMIPS case we've invented a label
and are using it in preference to the user-defined one.

> I'd just recommend peeking at the symbol table (back to the first 
> program):
>
> $ mips-sde-elf-objdump -t b.o
>
> b.o:     file format elf32-tradbigmips
>
> SYMBOL TABLE:
> 00000000 l    d  .text	00000000 .text
> 00000000 l    d  .data	00000000 .data
> 00000000 l    d  .bss	00000000 .bss
> 00000000 l    d  .reginfo	00000000 .reginfo
> 00000000 l    d  .pdr	00000000 .pdr
> 00000000 l     F .text	00000006 0x80 foo
> 00000006 l     F .text	00000002 0x80 bar
> 00000006 l       .text	00000000 0x80 .L1\x021

I suppose having a symbol with ^B in it is less than ideal too.
AIUI that name was chosen specifically because it wasn't supposed
to be written out.

It would be especially confusing if the user or compiler had a ".L11"
label (without the ^B).

>> The following:
>> 
>> 	.set	micromips
>> 	.ent	foo
>> foo:
>> 	ld	$10,0x1000($11)
>> 	.end	foo
>> 
>> generates an assertion failure:
>> 
>> Assertion failure in micromips_macro_build at gas/config/tc-mips.c line 19466.
>> Please report this bug.
>> 
>> on mipsisa64-elf with "-mips1 -mabi=32".
>
>  I can't reproduce it, sorry:
> [...]
>  Can you debug it and see what the relocation type is that's causing it?  
> I wonder if that might be related to the varargs issue you referring to 
> below and depend on the host architecture, hmm...

Yeah, you're right, sorry.  I forgot to mention that in the later reviews.
This was with a x86_64-linux-gnu host and a botched attempt at working
around the varags issue.  (I probably just added -Wno-error or something,
I can't remember now.)

I switched to a 32-bit host for parts 2 and 3 of the review, and yeah,
it doesn't reproduce there.

>> > gas/
>> > 2010-05-18  Chao-ying Fu  <fu@mips.com>
>> >             Maciej W. Rozycki  <macro@codesourcery.com>
>> >             Daniel Jacobowitz  <dan@codesourcery.com>
>> >
>> > 	* config/tc-mips.h (mips_segment_info): Add one bit for
>> > 	microMIPS.
>> > 	* config/tc-mips.c
>> 
>> How about having something like:
>> 
>>   #define OOD_TEXT_LABELS (mips_opts.mips16 || mips_opts.micromips)
>> 
>> ?
>
>  It sounds reasonable to me, except that condition is used in some other 
> contexts as well.  I have made it HAVE_CODE_COMPRESSION thus and took the 
> opportunity to optimise code around mips16_micromips_mark_labels() too.  
> Finally I have renamed the function to mips_compressed_mark_labels() for 
> as the other sounds too complicated to me.

All these changes sound good, thanks.

>> > 	(append_insn): Handle microMIPS.
>> 
>> +  if (mips_opts.micromips)
>> +    {
>> +      if ((prev_pinfo2 & INSN2_BRANCH_DELAY_16BIT)
>> +	  && !is_micromips_16bit_p (ip->insn_mo))
>> +	as_warn (_("instruction with wrong size in a branch delay slot that"
>> +		   " requires a 16-bit instruction"));
>> +      if ((prev_pinfo2 & INSN2_BRANCH_DELAY_32BIT)
>> +	  && !is_micromips_32bit_p (ip->insn_mo))
>> +	as_warn (_("instruction with wrong size in a branch delay slot that"
>> +		   " requires a 32-bit instruction"));
>> +    }
>> +  
>> 
>> Although not enforced as often as it should be, GAS convention is for
>> errors to start with a capital letter.
>
>  I'll be fixing these up as I come across them.  If to be effective, then 
> upstream HEAD should be fixed up or otherwise people have no way not to 
> get confused.  I didn't know of this rule for one.  That shouldn't be a 
> lot effort, should it?

Nope.  It's just a question of time and priorities. ;-)

>  Some have to stay for now actually, because .l testsuite patterns are 
> commonly shared between standard MIPS and microMIPS tests, and should be 
> fixed separately.

OK, that's fine.

>> +      if (pinfo & INSN_COP)
>> +	{
>> +	  /* We don't keep enough information to sort these cases out.
>> +	     The itbl support does keep this information however, although
>> +	     we currently don't support itbl fprmats as part of the cop
>> +	     instruction.  May want to add this support in the future.  */
>> +	}
>> 
>> Assert?
>
>  Well, that's no different to the standard MIPS variant.

OK, fair enough, but I suppose I was judging the new code on its merits
as new code.  Sticking to existing practice is an easier sell if the
code that implements it is being reused rather than copied...

>> +do_lsb:
>> 
>> Not properly indented.  A few other instances.
>
>  Like the respective originals in mips_ip().  I have fixed up the new 
> labels, but upstream HEAD code should be adjusted the same way.

Thanks.  Yeah, sorting out the new code is all that's needed.
As before, I was judging the code on its own merits rather than
checking whether each dubious bit was new or from cut-&-paste.

It'll be a while before I have chance to go through the update
properly, but it looks good at first glance.

Richard