Re: Bad choices by expand_mult_highpart

[Ulrich Weigand <weigand at i1 dot informatik dot uni-erlangen dot de>]

Jim Morrison wrote:

>  Have you filed a bug against this yet?  This looks really similar to what
> is causing my bootstrap failure on sparc-linux.

No, I haven't filed a bug.

However, it would appear that the problem is caused by trying to do
a signed multiply, which synth_mult doesn't actually support.  To fix
this requires an adjustment like the one performed by
expand_mult_highpart_adjust.

Also, we need to pass WIDER_MODE instead of MODE to 
choose_mult_variant, so that the test whether negate_variant is
allowed uses the correct mode size.

This in turn made the default multiplication cost computation
in choose_mult_variant result in too high values, so I simply
added another argument to pass in the cost bound instead.

The resulting patch is attached; it fixes my test case, but I haven't
completed full testing yet.  Could you try whether it fixes your
bootstrap problem on sparc?

Richard, do you think this is a correct solution?  Does is get
the cost calculations right for your targets?

Bye,
Ulrich



ChangeLog:

	* expmed.c (choose_mult_variant): Pass MULT_COST as argument instead
	of using register multiplication cost.
	(expand_mult): Adapt choose_mult_variant call.
	(expand_mult_highpart): Call choose_mult_variant with WIDER_MODE
	instead of MODE; pass appropriate cost bound.  Adjust result when
	performing signed multiplication by a negative constant.
	

Index: gcc/expmed.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expmed.c,v
retrieving revision 1.152
diff -c -p -r1.152 expmed.c
*** gcc/expmed.c	19 Mar 2004 09:59:00 -0000	1.152
--- gcc/expmed.c	21 Mar 2004 02:12:50 -0000
*************** enum mult_variant {basic_variant, negate
*** 2157,2163 ****
  
  static void synth_mult (struct algorithm *, unsigned HOST_WIDE_INT, int);
  static bool choose_mult_variant (enum machine_mode, HOST_WIDE_INT,
! 				 struct algorithm *, enum mult_variant *);
  static rtx expand_mult_const (enum machine_mode, rtx, HOST_WIDE_INT, rtx,
  			      const struct algorithm *, enum mult_variant);
  static unsigned HOST_WIDE_INT choose_multiplier (unsigned HOST_WIDE_INT, int,
--- 2157,2163 ----
  
  static void synth_mult (struct algorithm *, unsigned HOST_WIDE_INT, int);
  static bool choose_mult_variant (enum machine_mode, HOST_WIDE_INT,
! 				 struct algorithm *, enum mult_variant *, int);
  static rtx expand_mult_const (enum machine_mode, rtx, HOST_WIDE_INT, rtx,
  			      const struct algorithm *, enum mult_variant);
  static unsigned HOST_WIDE_INT choose_multiplier (unsigned HOST_WIDE_INT, int,
*************** synth_mult (struct algorithm *alg_out, u
*** 2416,2436 ****
         - a shift/add sequence based on -VAL, followed by a negation
         - a shift/add sequence based on VAL - 1, followed by an addition.
  
!    Return true if the cheapest of these is better than register
!    multiplication, describing the algorithm in *ALG and final
!    fixup in *VARIANT.  */
  
  static bool
  choose_mult_variant (enum machine_mode mode, HOST_WIDE_INT val,
! 		     struct algorithm *alg, enum mult_variant *variant)
  {
-   int mult_cost;
    struct algorithm alg2;
-   rtx reg;
- 
-   reg = gen_rtx_REG (mode, FIRST_PSEUDO_REGISTER);
-   mult_cost = rtx_cost (gen_rtx_MULT (mode, reg, GEN_INT (val)), SET);
-   mult_cost = MIN (12 * add_cost, mult_cost);
  
    *variant = basic_variant;
    synth_mult (alg, val, mult_cost);
--- 2416,2430 ----
         - a shift/add sequence based on -VAL, followed by a negation
         - a shift/add sequence based on VAL - 1, followed by an addition.
  
!    Return true if the cheapest of these cost less than MULT_COST,
!    describing the algorithm in *ALG and final fixup in *VARIANT.  */
  
  static bool
  choose_mult_variant (enum machine_mode mode, HOST_WIDE_INT val,
! 		     struct algorithm *alg, enum mult_variant *variant,
! 		     int mult_cost)
  {
    struct algorithm alg2;
  
    *variant = basic_variant;
    synth_mult (alg, val, mult_cost);
*************** expand_mult (enum machine_mode mode, rtx
*** 2642,2651 ****
       that it seems better to use synth_mult always.  */
  
    if (const_op1 && GET_CODE (const_op1) == CONST_INT
!       && (unsignedp || !flag_trapv)
!       && choose_mult_variant (mode, INTVAL (const_op1), &algorithm, &variant))
!     return expand_mult_const (mode, op0, INTVAL (const_op1), target,
! 			      &algorithm, variant);
  
    if (GET_CODE (op0) == CONST_DOUBLE)
      {
--- 2636,2651 ----
       that it seems better to use synth_mult always.  */
  
    if (const_op1 && GET_CODE (const_op1) == CONST_INT
!       && (unsignedp || !flag_trapv))
!     {
!       int mult_cost = rtx_cost (gen_rtx_MULT (mode, op0, op1), SET);
!       mult_cost = MIN (12 * add_cost, mult_cost);
! 
!       if (choose_mult_variant (mode, INTVAL (const_op1), &algorithm, &variant,
! 			       mult_cost))
! 	return expand_mult_const (mode, op0, INTVAL (const_op1), target,
! 				  &algorithm, variant);
!     }
  
    if (GET_CODE (op0) == CONST_DOUBLE)
      {
*************** expand_mult_highpart (enum machine_mode 
*** 2976,2982 ****
  		      unsigned HOST_WIDE_INT cnst1, rtx target,
  		      int unsignedp, int max_cost)
  {
!   enum machine_mode wider_mode;
    enum mult_variant variant;
    struct algorithm alg;
    rtx op1, tem;
--- 2976,2984 ----
  		      unsigned HOST_WIDE_INT cnst1, rtx target,
  		      int unsignedp, int max_cost)
  {
!   enum machine_mode wider_mode = GET_MODE_WIDER_MODE (mode);
!   int extra_cost = shift_cost[GET_MODE_BITSIZE (mode) - 1];
!   bool sign_adjust = false;
    enum mult_variant variant;
    struct algorithm alg;
    rtx op1, tem;
*************** expand_mult_highpart (enum machine_mode 
*** 2987,3007 ****
  
    op1 = gen_int_mode (cnst1, mode);
  
    /* See whether shift/add multiplication is cheap enough.  */
!   if (choose_mult_variant (mode, cnst1, &alg, &variant)
!       && (alg.cost += shift_cost[GET_MODE_BITSIZE (mode) - 1]) < max_cost)
      {
        /* See whether the specialized multiplication optabs are
  	 cheaper than the shift/add version.  */
        tem = expand_mult_highpart_optab (mode, op0, op1, target,
! 					unsignedp, alg.cost);
        if (tem)
  	return tem;
  
!       wider_mode = GET_MODE_WIDER_MODE (mode);
!       op0 = convert_to_mode (wider_mode, op0, unsignedp);
!       tem = expand_mult_const (wider_mode, op0, cnst1, 0, &alg, variant);
!       return extract_high_half (mode, tem);
      }
    return expand_mult_highpart_optab (mode, op0, op1, target,
  				     unsignedp, max_cost);
--- 2989,3021 ----
  
    op1 = gen_int_mode (cnst1, mode);
  
+   /* Check whether we try to multiply by a negative constant.  */
+   if (!unsignedp && ((cnst1 >> (GET_MODE_BITSIZE (mode) - 1)) & 1))
+     {
+       sign_adjust = true;
+       extra_cost += add_cost;
+     }
+ 
    /* See whether shift/add multiplication is cheap enough.  */
!   if (choose_mult_variant (wider_mode, cnst1, &alg, &variant,
! 			   max_cost - extra_cost))
      {
        /* See whether the specialized multiplication optabs are
  	 cheaper than the shift/add version.  */
        tem = expand_mult_highpart_optab (mode, op0, op1, target,
! 					unsignedp, alg.cost + extra_cost);
        if (tem)
  	return tem;
  
!       tem = convert_to_mode (wider_mode, op0, unsignedp);
!       tem = expand_mult_const (wider_mode, tem, cnst1, 0, &alg, variant);
!       tem = extract_high_half (mode, tem);
! 
!       /* Adjust result for signedness. */
!       if (sign_adjust)
! 	tem = force_operand (gen_rtx_MINUS (mode, tem, op0), tem);
! 
!       return tem;
      }
    return expand_mult_highpart_optab (mode, op0, op1, target,
  				     unsignedp, max_cost);

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de