Re: [PATCH v2] PR target/30315 i386 missed optimization detecting overflows

[Uros Bizjak <ubizjak at gmail dot com>]

Rask Ingemann Lambertsen wrote:

> 2007-08-10  Rask Ingemann Lambertsen  <rask@sygehus.dk>
>
> 	PR target/30315
> 	* config/i386/i386.md (SWI)(addsub): New.
> 	(*<optab><mode>3_cc_overflow1): New.
> 	(*<optab><mode>3_cc_overflow2): New.
> 	(*<optab><mode>3_cconly_overflow1): New.
> 	(*<optab><mode>3_cconly_overflow2): New.
> 	(*<optab>si3_zext_cc_overflow1): New.
> 	(*<optab>si3_zext_cc_overflow2): New.
> 	* config/i386/predicates.md (fcmov_comparison_operator): Accept
> 	CCCmode for LTU, GTU, LEU and GEU.
> 	(ix86_comparison_operator): Likewise.
> 	(ix86_carry_flag_operator): Carry flag is set if LTU or GTU in CCCmode.
> 	* gcc/config/i386/i386.c (put_condition_code): Support CCCmode.
> 	(ix86_cc_mode): Use CCCmode when testing for overflow of PLUS
> 	or MINUS expressions.
>
> testsuite/
>
> 2007-08-10  Rask Ingemann Lambertsen  <rask@sygehus.dk>
>
> 	PR target/30315
> 	* gcc.target/i386/pr30315.c: New.
>
>   

This is OK for mainline, but please consider a few minor remarks below.

Thanks,
Uros.

> Index: gcc/config/i386/i386.md
> ===================================================================
> --- gcc/config/i386/i386.md	(revision 127179)
> +++ gcc/config/i386/i386.md	(working copy)
> @@ -488,6 +488,36 @@
>    [(set_attr "length" "128")
>     (set_attr "type" "multi")])
>  
> +(define_code_macro addsub [plus minus])
> +
>   

Perhaps a better name for a code macro would be something like 
"plusminus" ...
> +;; Base name to use for define_insn and such.
> +(define_code_attr optab [(plus "add") (minus "sub")])
>   

... leaving "addsub" keyword for a more descriptive define_insn 
macroized pattern name.
> +
> +;; Mark commutative operators as such in constraints.
> +(define_code_attr pct [(plus "%") (minus "")])
>   

Perhaps "comm" instead of "pct"?

> +
> +;; Instruction mnemonic for output templates.
> +(define_code_attr mnemonic [(plus "add") (minus "sub")])
>   

The definition above is not needed, "addsub" keyword can also be used 
instead.

> +
> +;; All single word integer modes.
> +(define_mode_macro SWI [QI HI SI (DI "TARGET_64BIT")])
>