Re: [PATCH MIPS] Conditional traps for divide by zero checks...

[Richard Sandiford <rsandifo at redhat dot com>]

David Daney <ddaney@avtrex.com> writes:
> This patch lets the MIPS port generate a conditional trap instead of the
> two instruction conditional branch and break sequence for detecting
> division by zero.

This looks good to me.  The only thing I'm a little worried about is:

> The default is to use -mdivide-traps on platforms
> that have conditional traps.

For example, I don't know if *bsd folks are in the habit of building
for anything other than MIPS I.  If they are, will their OS cope with
the new trap insns?

On the other hand, I agree it's a good default if it doesn't cause
problems.  We can always change it later in the release cycle if
necessary, once people have had chance to test it, so my inclination
is to stick with it for now.

> 2004-09-02  David Daney  <ddaney@avtrex.com>
>
> 	* config.gcc: Added support for --with-divide=[breaks|traps] for
> 	mips targets.
> 	* config/mips/mips.h (MASK_DIVIDE_BREAKS): New target_flags bit.
> 	(TARGET_DIVIDE_TRAPS): New macro.
> 	(TARGET_SWITCHES): Added -mdivide-traps and -mdivide-breaks.
> 	(OPTION_DEFAULT_SPECS): Added --with-divide= support.
> 	* config/mips/mips.c (mips_idiv_insns): Generate proper count on
> 	GENERATE_DIVIDE_TRAPS.
> 	(mips_output_division): Emit conditional trap if
> 	GENERATE_DIVIDE_TRAPS is set.
> 	* doc/install.texi: Document --with-divide.
> 	* doc/invoke.texi: Document -mdivide-traps and -mdivide-breaks.

This is OK with me except for the minor comments below.  I'd like
Eric's buy-in as well though.

> Index: gcc/config/mips/mips.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/config/mips/mips.h,v
> retrieving revision 1.364
> diff -c -p -r1.364 mips.h
> *** gcc/config/mips/mips.h	29 Aug 2004 20:13:11 -0000	1.364
> --- gcc/config/mips/mips.h	2 Sep 2004 17:42:38 -0000
> *************** extern const struct mips_cpu_info *mips_
> *** 164,169 ****
> --- 164,172 ----
>   #define MASK_PAIRED_SINGLE 0x10000000   /* Support paired-single FPU.  */
>   #define MASK_MIPS3D        0x20000000   /* Support MIPS-3D instructions.  */
>   
> + #define MASK_DIVIDE_BREAKS 0x40000000   /* Divide by zero check uses
> +                                            break instead of trap. */
> + 
>   					/* Debug switches, not documented */
>   #define MASK_DEBUG	0		/* unused */
>   #define MASK_DEBUG_D	0		/* don't do define_split's */

0x08000000 is unused at the moment.  Would you mind using that instead?

No need to retest with that change.

> *************** configure for @samp{mipsel-elf} as a wor
> *** 3192,3197 ****
> --- 3203,3217 ----
>   @samp{mips*-*-linux*} target continues to use the MIPS II routines.  More
>   work on this is expected in future releases.
>   
> + MIPS systems check for division by zero (unless
> + @option{-mno-check-zero-division} is passed to the compiler) by
> + generating either a conditional trap or a break instruction.  Using
> + trap results in smaller code, but is only supported on MIPS II and
> + later.  Also, some versions of the Linux kernel have a bug that
> + prevents trap from generating the proper signal (SIGFPE).  To enable
> + the use of trap, use the @option{--with-divide=traps}
> + @command{configure} option when configuring GCC.
> + 
>   Cross-compilers for the Mips as target using the Mips assembler
>   currently do not work, because the auxiliary programs
>   @file{mips-tdump.c} and @file{mips-tfile.c} can't be compiled on

This is really good. ;)

> + @item -mdivide-traps
> + @itemx -mdivide-breaks
> + @opindex mdivide-traps
> + @opindex mdivide-breaks
> + Use conditional trap (break) to check for integer division by zero.
> + The default is @option{-mdivide-breaks} unless overridden by the the
> + @option{--with-divide=traps} @command{configure} option when
> + configuring GCC.
> + 

To me, this read like "-mdivide-breaks: Use conditional break" which
is a little confusing.  Also, shouldn't the default be -mdivide-traps?

Would you mind copying (ouch, it hurts to say that) the install notes
above and adapting them to mention the command line option?  Something
like:

    MIPS systems check for division by zero by generating either a
    conditional trap or a break instruction.  Using traps results in
    smaller code, but is only supported on MIPS II and later.
    Use @option{-mdivide-traps} to allow conditional traps on
    architectures that support them and @option{-mdivide-breaks}
    to force the use of breaks.

    The default is usually @option{-mdivide-traps}, but this can
    be overridden at configure time using @option{--with-divide=break}.
    Divide-by-zero checks can be completely disabled using
    @option{-mno-check-zero-division}.

Richard