[PATCH v2, i386]: Introduce x86_maybe_negate_const_int

[Uros Bizjak <ubizjak at gmail dot com>]

On 02/12/2010 03:15 PM, Nathan Froyd wrote:
> On Fri, Feb 12, 2010 at 02:27:05PM +0100, Uros Bizjak wrote:
>    
> > Attached patch introduces x86_should_negate_const_int_p predicate in
> > places that negate const ints in order to generate smaller or
> > "cleaner" (i.e. sub XX, %reg instead of add -XX, %reg) asm code. The
> > predicate is also enhanced to perform checks for overflow when trying
> > to negate 0x80...0 to all modes.
> >
> > @@ -6769,16 +6691,12 @@
> >
> >       default:
> >         gcc_assert (rtx_equal_p (operands[0], operands[1]));
> > -      /* Make things pretty and `subl $4,%eax' rather than `addl $-4,%eax'.
> > -	 Exceptions: -128 encodes smaller than 128, so swap sign and op.  */
> > -      if ((INTVAL (operands[2]) == -128
> > -	   || (INTVAL (operands[2])>  0
> > -	&&  INTVAL (operands[2]) != 128))
> > -	  /* Avoid overflows.  */
> > -	&&  ((INTVAL (operands[2])&  ((((unsigned int) 1)<<  31) - 1))))
> > -	return "sub{q}\t{%2, %0|%0, %2}";
> > -      operands[2] = GEN_INT (-INTVAL (operands[2]));
> > -      return "add{q}\t{%2, %0|%0, %2}";
> > +      if (x86_should_negate_const_int_p (operands[2], DImode))
> > +	{
> > +	  operands[2] = GEN_INT (-INTVAL (operands[2]));
> > +	  return "add{q}\t{%2, %0|%0, %2}";
> > +	}
> > +      return "sub{q}\t{%2, %0|%0, %2}";
> >       }
> >   }
> >      
> It seems like every place you use x86_should_negate_const_int_p, you're
> doing this pattern of if/then negate/else.  Perhaps you should go even
> farther and have:
>
> const char *
> x86_addsub_with_maybe_negated_operand (rtx *loc, enum machine_mode mode,
>                                         const char *addstr, const char *substr)
> {
>     /* Most of the logic from x86_should_negate_const_int_p...  */
>
>     /* Modify *LOC if necessary.  */
>
>     /* Return ADDSTR or SUBSTR as appropriate.  */
> }
>
> And then all the affected places just return the result of calling that
> function.
>    

Thanks for your suggestion!

I think that your idea of conditionally modifying *LOC cleans and 
simplifies the code even further. OTOH, passing addstr and substr seems 
like an overkill to me, since we can return a boolean to signal if the 
negation of the operand was performed and choose between two strings (or 
do some other action) at the caller site.

Attached is new version of the patch (it also adds one more spot where 
x86_maybe_negate_const_int can be used):

2010-02-26  Uros Bizjak <ubizjak@gmail.com>

    * config/i386/i386.c (x86_maybe_negate_const_int): New.
    (x86_output_mi_thunk): Use x86_maybe_negate_const_int.
    * config/i386/i386-protos.h: Declare x86_maybe_negate_const_int.
    * config/i386/i386.md (*add<mode>_1, *addsi_1_zext, *addhi_1,
    *addhi_1_lea, *addqi_1, *addqi_1_lea, *addqi_1_slp, *add<mode>_2,
    *addsi_2_zext, *addhi_2, *addqi_2, *add<mode>_3, *addsi_3_zext,
    *addhi_3, *addqi_3,*add<mode>_5, *addhi_5, *addqi_5):
    Use x86_maybe_negate_const_int to output insn mnemonic.
    (*adddi_4, *addsi_4, *addhi_4, *addqi_4): Ditto.  Remove overflow
    check from instruction predicate.  Update comments.
    * config/i386/sync.md (sync_add<mode>): Use
    x86_maybe_negate_const_int to output insn mnemonic.

Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu 
{,-m32}. This patch will be committed to 4.6.

Uros.

Attachment: p.diff.txt
Description: Text document