Re: [Patch] MIPS: mips16e machine patterns - zeb/zeh seb/seh

[Richard Sandiford <rsandifo at nildram dot co dot uk>]

David Ung <davidu@mips.com> writes:
> This patch adds to the compiler machine patterns for the mips16e zero
> extend (byte/half) and sign extend (byte/half) instructions.
> The mips16e extensions are available for mips32 and above.  There are
> other instructions such as save/restore which we will submit in future
> patches.
> I've added the command line option of -mips16e to turn on the generation
> of these instructions.
> A regression test case is also includes.

I suppose there's the question of whether this should be handled by a
separate switch (-mips16e, like you've done) or whether the availability
of MIPS16e should be inferred from the combination of -mips16 and the
-march setting.

I've no strong opinion either way, but I'd like to make sure that the
possibility has been considered.

> 	* config/mips/mips.opt: Add new command line option mips16e.
> 	* config/mips/mips.h (TARGET_MIPS16E): New definition.
> 	(TARGET_CPU_CPP_BUILTINS): Define __mips16e.
> 	(ASM_SPEC): Pass through -mips16e.
> 	* config/mips/mips.c (override_options): Warn if -mips16e and
> 	-mno-mips16 are used together.  Enable -mips16, when -mips16e is
> 	used. 
> 	(mips_file_start): Print ".set mips16e" for -mips16e.
> 	(build_mips16_function_stub): Similarly.
> 	(build_mips16_call_stub): Similarly.
> 	* config/mips/mips.md (zero_extend<SHORT:mode><GPR:mode>2):
> 	Changed expand condition to exclude generating of "and" if
> 	TARGET_MIPS16E is true.
> 	(*zero_extend<SHORT:mode><GPR:mode>2_mips16e): New pattern for
> 	matching mips16e zeb/zeh.
> 	(*extend<SHORT:mode><GPR:mode>2_mips16e): New pattern for matching
> 	mips16e seb/seh. 
> 	(*extend<SHORT:mode><GPR:mode>2): Disable this pattern for
> 	TARGET_MIPS16E. 

You don't document the new option.  Tsk tsk ;)

> Index: gcc/testsuite/gcc.target/mips/mips.exp
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/testsuite/gcc.target/mips/mips.exp,v
> retrieving revision 1.2
> diff -c -p -b -r1.2 mips.exp
> *** gcc/testsuite/gcc.target/mips/mips.exp	18 Apr 2005 20:34:36 -0000	1.2
> --- gcc/testsuite/gcc.target/mips/mips.exp	7 Jun 2005 13:42:46 -0000
> *************** proc dg-mips-options {args} {
> *** 152,159 ****
>   	    if {$mips_mips16 || ($arch != $mips_arch && $mips_forced_isa)} {
>   		set matches 0
>   	    }
> ! 	} elseif {[regexp -- {^-mips(.*)} $flag dummy isa] && $isa != 16} {
> ! 	    if {$mips_mips16 || ($isa != $mips_isa && $mips_forced_isa)} {
>   		set matches 0
>   	    }
>   	} elseif {[regexp -- {^-m(hard|soft)-float} $flag dummy float]} {
> --- 152,164 ----
>   	    if {$mips_mips16 || ($arch != $mips_arch && $mips_forced_isa)} {
>   		set matches 0
>   	    }
> ! 	} elseif {[regexp -- {^-mips(.*)} $flag dummy isa]} {
> ! 	    if {$isa == "16e"} {
> ! 	        if {$mips_isa < 32} {
> ! 		    set matches 0 
> !                 }            
> ! 	    } elseif {$isa != 16 && ($mips_mips16 
> !                       || ($isa != $mips_isa && $mips_forced_isa))} {
>   		set matches 0
>   	    }
>   	} elseif {[regexp -- {^-m(hard|soft)-float} $flag dummy float]} {

Why is this necessary?  You've coded -mips16e so that it works for any
-march setting, so I would have expected it to be case of extending
"isa != 16" to something like "![string match 16* $isa]".

(Note that the script doesn't cope with -mips16 for dg-do run tests,
but we don't have any of those, and your new one is just a compile
test too.)

Looks good otherwise.

Richard