[PATCH] Cleanup expand_shift

[PATCH] Cleanup expand_shift

[Richard Guenther]

This is the promised followup to the expand_shift reorg.  The following
patch makes the worker take an RTL expanded shift amount and avoids
re-creating adjusted trees if it recurses for expanding rotates.  Most
of the scary code (involving the conversions) originated from the
fix for PR27861 - but I'm not sure if that bug was actually a side-effect
of using make_tree here.  I'm also not sure how much testing coverage
we have for this particular piece of code.

Anway, I'll bootstrap and test this on x86_64-unknown-linux-gnu
and I have verified PR27861 doesn't re-occur with a cross to
mipsel-linux-gnu.

Ok for trunk if it succeeds?

Thanks,
Richard.

2011-05-03  Richard Guenther  <rguenther@suse.de>

	* expmed.c (expand_variable_shift): Rename to ...
	(expand_shift_1): ... this.  Take an expanded shift amount.
	For rotates recurse directly not building trees for the shift amount.
	(expand_variable_shift): Wrap around expand_shift_1.
	(expand_shift): Adjust.

Index: gcc/expmed.c
===================================================================
*** gcc/expmed.c	(revision 173299)
--- gcc/expmed.c	(working copy)
*************** expand_dec (rtx target, rtx dec)
*** 2034,2047 ****
  \f
  /* Output a shift instruction for expression code CODE,
     with SHIFTED being the rtx for the value to shift,
!    and AMOUNT the tree for the amount to shift by.
     Store the result in the rtx TARGET, if that is convenient.
     If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
     Return the rtx for where the value is.  */
  
! rtx
! expand_variable_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
! 		       tree amount, rtx target, int unsignedp)
  {
    rtx op1, temp = 0;
    int left = (code == LSHIFT_EXPR || code == LROTATE_EXPR);
--- 2034,2047 ----
  \f
  /* Output a shift instruction for expression code CODE,
     with SHIFTED being the rtx for the value to shift,
!    and AMOUNT the rtx for the amount to shift by.
     Store the result in the rtx TARGET, if that is convenient.
     If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
     Return the rtx for where the value is.  */
  
! static rtx
! expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted,
! 		rtx amount, rtx target, int unsignedp)
  {
    rtx op1, temp = 0;
    int left = (code == LSHIFT_EXPR || code == LROTATE_EXPR);
*************** expand_variable_shift (enum tree_code co
*** 2055,2061 ****
    int attempt;
    bool speed = optimize_insn_for_speed_p ();
  
!   op1 = expand_normal (amount);
    op1_mode = GET_MODE (op1);
  
    /* Determine whether the shift/rotate amount is a vector, or scalar.  If the
--- 2055,2061 ----
    int attempt;
    bool speed = optimize_insn_for_speed_p ();
  
!   op1 = amount;
    op1_mode = GET_MODE (op1);
  
    /* Determine whether the shift/rotate amount is a vector, or scalar.  If the
*************** expand_variable_shift (enum tree_code co
*** 2140,2164 ****
  		 code below.  */
  
  	      rtx subtarget = target == shifted ? 0 : target;
! 	      tree new_amount, other_amount;
  	      rtx temp1;
! 	      tree type = TREE_TYPE (amount);
! 	      if (GET_MODE (op1) != TYPE_MODE (type)
! 		  && GET_MODE (op1) != VOIDmode)
! 		op1 = convert_to_mode (TYPE_MODE (type), op1, 1);
! 	      new_amount = make_tree (type, op1);
  	      other_amount
! 		= fold_build2 (MINUS_EXPR, type,
! 			       build_int_cst (type, GET_MODE_BITSIZE (mode)),
! 			       new_amount);
  
  	      shifted = force_reg (mode, shifted);
  
! 	      temp = expand_variable_shift (left ? LSHIFT_EXPR : RSHIFT_EXPR,
! 					    mode, shifted, new_amount, 0, 1);
! 	      temp1 = expand_variable_shift (left ? RSHIFT_EXPR : LSHIFT_EXPR,
! 					     mode, shifted, other_amount,
! 					     subtarget, 1);
  	      return expand_binop (mode, ior_optab, temp, temp1, target,
  				   unsignedp, methods);
  	    }
--- 2140,2161 ----
  		 code below.  */
  
  	      rtx subtarget = target == shifted ? 0 : target;
! 	      rtx new_amount, other_amount;
  	      rtx temp1;
! 
! 	      new_amount = amount;
  	      other_amount
! 		= simplify_gen_binary (MINUS, GET_MODE (amount),
! 				       GEN_INT (GET_MODE_BITSIZE (mode)),
! 				       amount);
  
  	      shifted = force_reg (mode, shifted);
  
! 	      temp = expand_shift_1 (left ? LSHIFT_EXPR : RSHIFT_EXPR,
! 				     mode, shifted, new_amount, 0, 1);
! 	      temp1 = expand_shift_1 (left ? RSHIFT_EXPR : LSHIFT_EXPR,
! 				      mode, shifted, other_amount,
! 				      subtarget, 1);
  	      return expand_binop (mode, ior_optab, temp, temp1, target,
  				   unsignedp, methods);
  	    }
*************** rtx
*** 2213,2224 ****
  expand_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
  	      int amount, rtx target, int unsignedp)
  {
!   /* ???  With re-writing expand_shift we could avoid going through a
!      tree for the shift amount and directly do GEN_INT (amount).  */
!   return expand_variable_shift (code, mode, shifted,
! 				build_int_cst (integer_type_node, amount),
! 				target, unsignedp);
  }
  \f
  /* Indicates the type of fixup needed after a constant multiplication.
     BASIC_VARIANT means no fixup is needed, NEGATE_VARIANT means that
--- 2210,2234 ----
  expand_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
  	      int amount, rtx target, int unsignedp)
  {
!   return expand_shift_1 (code, mode,
! 			 shifted, GEN_INT (amount), target, unsignedp);
  }
+ 
+ /* Output a shift instruction for expression code CODE,
+    with SHIFTED being the rtx for the value to shift,
+    and AMOUNT the tree for the amount to shift by.
+    Store the result in the rtx TARGET, if that is convenient.
+    If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
+    Return the rtx for where the value is.  */
+ 
+ rtx
+ expand_variable_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
+ 		       tree amount, rtx target, int unsignedp)
+ {
+   return expand_shift_1 (code, mode,
+ 			 shifted, expand_normal (amount), target, unsignedp);
+ }
+ 
  \f
  /* Indicates the type of fixup needed after a constant multiplication.
     BASIC_VARIANT means no fixup is needed, NEGATE_VARIANT means that

Re: [PATCH] Cleanup expand_shift

[Eric Botcazou]

> 2011-05-03  Richard Guenther  <rguenther@suse.de>
>
> 	* expmed.c (expand_variable_shift): Rename to ...
> 	(expand_shift_1): ... this.  Take an expanded shift amount.
> 	For rotates recurse directly not building trees for the shift amount.
> 	(expand_variable_shift): Wrap around expand_shift_1.
> 	(expand_shift): Adjust.

Looks OK on principle, but...

> --- 2140,2161 ----
>   		 code below.  */
>
>   	      rtx subtarget = target == shifted ? 0 : target;
> ! 	      rtx new_amount, other_amount;
>   	      rtx temp1;
> !
> ! 	      new_amount = amount;
>   	      other_amount
> ! 		= simplify_gen_binary (MINUS, GET_MODE (amount),
> ! 				       GEN_INT (GET_MODE_BITSIZE (mode)),
> ! 				       amount);

... why going back to AMOUNT?  The old code uses OP1, which can be different 
from AMOUNT if SHIFT_COUNT_TRUNCATED is nonzero.

-- 
Eric Botcazou

Re: [PATCH] Cleanup expand_shift

[Richard Guenther]

On Wed, 4 May 2011, Eric Botcazou wrote:

> > 2011-05-03  Richard Guenther  <rguenther@suse.de>
> >
> > 	* expmed.c (expand_variable_shift): Rename to ...
> > 	(expand_shift_1): ... this.  Take an expanded shift amount.
> > 	For rotates recurse directly not building trees for the shift amount.
> > 	(expand_variable_shift): Wrap around expand_shift_1.
> > 	(expand_shift): Adjust.
> 
> Looks OK on principle, but...
> 
> > --- 2140,2161 ----
> >   		 code below.  */
> >
> >   	      rtx subtarget = target == shifted ? 0 : target;
> > ! 	      rtx new_amount, other_amount;
> >   	      rtx temp1;
> > !
> > ! 	      new_amount = amount;
> >   	      other_amount
> > ! 		= simplify_gen_binary (MINUS, GET_MODE (amount),
> > ! 				       GEN_INT (GET_MODE_BITSIZE (mode)),
> > ! 				       amount);
> 
> ... why going back to AMOUNT?  The old code uses OP1, which can be different 
> from AMOUNT if SHIFT_COUNT_TRUNCATED is nonzero.

I think I did it that way because the old code tried to re-construct
the type of the original amount.  I can surely simply use op1 here
if that is preferred.

Btw, do you happen to know any target that would excercise this code
choosing from x86, ppc, s390 and ia64?

Thanks,
Richard.

Re: [PATCH] Cleanup expand_shift

[Eric Botcazou]

> I think I did it that way because the old code tried to re-construct
> the type of the original amount.  I can surely simply use op1 here
> if that is preferred.

Right, but it used the value of OP1 so I think the new code should as well.

> Btw, do you happen to know any target that would excercise this code
> choosing from x86, ppc, s390 and ia64?

All have rotate instructions so this seems to be a wash.

-- 
Eric Botcazou

Re: [PATCH] Cleanup expand_shift

[Richard Guenther]

On Wed, 4 May 2011, Eric Botcazou wrote:

> > I think I did it that way because the old code tried to re-construct
> > the type of the original amount.  I can surely simply use op1 here
> > if that is preferred.
> 
> Right, but it used the value of OP1 so I think the new code should as well.

Ok, I'll change it that way.

> > Btw, do you happen to know any target that would excercise this code
> > choosing from x86, ppc, s390 and ia64?
> 
> All have rotate instructions so this seems to be a wash.

Hm.  I guess people will scream if something breaks (I can't imagine
what though).

Richard.

Re: [PATCH] Cleanup expand_shift

[Richard Guenther]

On Wed, 4 May 2011, Richard Guenther wrote:

> On Wed, 4 May 2011, Eric Botcazou wrote:
> 
> > > I think I did it that way because the old code tried to re-construct
> > > the type of the original amount.  I can surely simply use op1 here
> > > if that is preferred.
> > 
> > Right, but it used the value of OP1 so I think the new code should as well.
> 
> Ok, I'll change it that way.
> 
> > > Btw, do you happen to know any target that would excercise this code
> > > choosing from x86, ppc, s390 and ia64?
> > 
> > All have rotate instructions so this seems to be a wash.
> 
> Hm.  I guess people will scream if something breaks (I can't imagine
> what though).

I have applied the following after re-bootstrapping and testing on
x86_64-unknown-linux-gnu and re-checking the mipsel cross testcase.

Richard.

2011-05-05  Richard Guenther  <rguenther@suse.de>

	* expmed.c (expand_variable_shift): Rename to ...
	(expand_shift_1): ... this.  Take an expanded shift amount.
	For rotates recurse directly not building trees for the shift amount.
	(expand_variable_shift): Wrap around expand_shift_1.
	(expand_shift): Adjust.

Index: gcc/expmed.c
===================================================================
*** gcc/expmed.c.orig	2011-05-04 11:07:08.000000000 +0200
--- gcc/expmed.c	2011-05-04 17:54:27.000000000 +0200
*************** expand_dec (rtx target, rtx dec)
*** 2032,2045 ****
  \f
  /* Output a shift instruction for expression code CODE,
     with SHIFTED being the rtx for the value to shift,
!    and AMOUNT the tree for the amount to shift by.
     Store the result in the rtx TARGET, if that is convenient.
     If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
     Return the rtx for where the value is.  */
  
! rtx
! expand_variable_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
! 		       tree amount, rtx target, int unsignedp)
  {
    rtx op1, temp = 0;
    int left = (code == LSHIFT_EXPR || code == LROTATE_EXPR);
--- 2032,2045 ----
  \f
  /* Output a shift instruction for expression code CODE,
     with SHIFTED being the rtx for the value to shift,
!    and AMOUNT the rtx for the amount to shift by.
     Store the result in the rtx TARGET, if that is convenient.
     If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
     Return the rtx for where the value is.  */
  
! static rtx
! expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted,
! 		rtx amount, rtx target, int unsignedp)
  {
    rtx op1, temp = 0;
    int left = (code == LSHIFT_EXPR || code == LROTATE_EXPR);
*************** expand_variable_shift (enum tree_code co
*** 2053,2059 ****
    int attempt;
    bool speed = optimize_insn_for_speed_p ();
  
!   op1 = expand_normal (amount);
    op1_mode = GET_MODE (op1);
  
    /* Determine whether the shift/rotate amount is a vector, or scalar.  If the
--- 2053,2059 ----
    int attempt;
    bool speed = optimize_insn_for_speed_p ();
  
!   op1 = amount;
    op1_mode = GET_MODE (op1);
  
    /* Determine whether the shift/rotate amount is a vector, or scalar.  If the
*************** expand_variable_shift (enum tree_code co
*** 2138,2162 ****
  		 code below.  */
  
  	      rtx subtarget = target == shifted ? 0 : target;
! 	      tree new_amount, other_amount;
  	      rtx temp1;
! 	      tree type = TREE_TYPE (amount);
! 	      if (GET_MODE (op1) != TYPE_MODE (type)
! 		  && GET_MODE (op1) != VOIDmode)
! 		op1 = convert_to_mode (TYPE_MODE (type), op1, 1);
! 	      new_amount = make_tree (type, op1);
  	      other_amount
! 		= fold_build2 (MINUS_EXPR, type,
! 			       build_int_cst (type, GET_MODE_BITSIZE (mode)),
! 			       new_amount);
  
  	      shifted = force_reg (mode, shifted);
  
! 	      temp = expand_variable_shift (left ? LSHIFT_EXPR : RSHIFT_EXPR,
! 					    mode, shifted, new_amount, 0, 1);
! 	      temp1 = expand_variable_shift (left ? RSHIFT_EXPR : LSHIFT_EXPR,
! 					     mode, shifted, other_amount,
! 					     subtarget, 1);
  	      return expand_binop (mode, ior_optab, temp, temp1, target,
  				   unsignedp, methods);
  	    }
--- 2138,2159 ----
  		 code below.  */
  
  	      rtx subtarget = target == shifted ? 0 : target;
! 	      rtx new_amount, other_amount;
  	      rtx temp1;
! 
! 	      new_amount = op1;
  	      other_amount
! 		= simplify_gen_binary (MINUS, GET_MODE (op1),
! 				       GEN_INT (GET_MODE_BITSIZE (mode)),
! 				       op1);
  
  	      shifted = force_reg (mode, shifted);
  
! 	      temp = expand_shift_1 (left ? LSHIFT_EXPR : RSHIFT_EXPR,
! 				     mode, shifted, new_amount, 0, 1);
! 	      temp1 = expand_shift_1 (left ? RSHIFT_EXPR : LSHIFT_EXPR,
! 				      mode, shifted, other_amount,
! 				      subtarget, 1);
  	      return expand_binop (mode, ior_optab, temp, temp1, target,
  				   unsignedp, methods);
  	    }
*************** rtx
*** 2211,2222 ****
  expand_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
  	      int amount, rtx target, int unsignedp)
  {
!   /* ???  With re-writing expand_shift we could avoid going through a
!      tree for the shift amount and directly do GEN_INT (amount).  */
!   return expand_variable_shift (code, mode, shifted,
! 				build_int_cst (integer_type_node, amount),
! 				target, unsignedp);
  }
  \f
  /* Indicates the type of fixup needed after a constant multiplication.
     BASIC_VARIANT means no fixup is needed, NEGATE_VARIANT means that
--- 2208,2232 ----
  expand_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
  	      int amount, rtx target, int unsignedp)
  {
!   return expand_shift_1 (code, mode,
! 			 shifted, GEN_INT (amount), target, unsignedp);
  }
+ 
+ /* Output a shift instruction for expression code CODE,
+    with SHIFTED being the rtx for the value to shift,
+    and AMOUNT the tree for the amount to shift by.
+    Store the result in the rtx TARGET, if that is convenient.
+    If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
+    Return the rtx for where the value is.  */
+ 
+ rtx
+ expand_variable_shift (enum tree_code code, enum machine_mode mode, rtx shifted,
+ 		       tree amount, rtx target, int unsignedp)
+ {
+   return expand_shift_1 (code, mode,
+ 			 shifted, expand_normal (amount), target, unsignedp);
+ }
+ 
  \f
  /* Indicates the type of fixup needed after a constant multiplication.
     BASIC_VARIANT means no fixup is needed, NEGATE_VARIANT means that

Re: [PATCH] Cleanup expand_shift

[Hans-Peter Nilsson]

On Thu, 5 May 2011, Richard Guenther wrote:
> On Wed, 4 May 2011, Richard Guenther wrote:
> > On Wed, 4 May 2011, Eric Botcazou wrote:
> > Hm.  I guess people will scream if something breaks (I can't imagine
> > what though).

AAAaaarghh!  Building cris-elf is now broken.

> I have applied the following after re-bootstrapping and testing on
> x86_64-unknown-linux-gnu and re-checking the mipsel cross testcase.
>
> Richard.
>
> 2011-05-05  Richard Guenther  <rguenther@suse.de>
>
> 	* expmed.c (expand_variable_shift): Rename to ...
> 	(expand_shift_1): ... this.  Take an expanded shift amount.
> 	For rotates recurse directly not building trees for the shift amount.
> 	(expand_variable_shift): Wrap around expand_shift_1.
> 	(expand_shift): Adjust.

PR 48908.

brgds, H-P

Re: [PATCH] Cleanup expand_shift

[Richard Guenther]

On Thu, 5 May 2011, Hans-Peter Nilsson wrote:

> On Thu, 5 May 2011, Richard Guenther wrote:
> > On Wed, 4 May 2011, Richard Guenther wrote:
> > > On Wed, 4 May 2011, Eric Botcazou wrote:
> > > Hm.  I guess people will scream if something breaks (I can't imagine
> > > what though).
> 
> AAAaaarghh!  Building cris-elf is now broken.
> 
> > I have applied the following after re-bootstrapping and testing on
> > x86_64-unknown-linux-gnu and re-checking the mipsel cross testcase.
> >
> > Richard.
> >
> > 2011-05-05  Richard Guenther  <rguenther@suse.de>
> >
> > 	* expmed.c (expand_variable_shift): Rename to ...
> > 	(expand_shift_1): ... this.  Take an expanded shift amount.
> > 	For rotates recurse directly not building trees for the shift amount.
> > 	(expand_variable_shift): Wrap around expand_shift_1.
> > 	(expand_shift): Adjust.
> 
> PR 48908.

Ok, it seems simplify_gen_binary doesn't like VOIDmode.  The following
side-steps the issue of choosing an appropriate mode for a constant
shift amount and instead computes it in HWI.  Similar to the
SHIFT_COUNT_TRUNCATED path we don't bother about a CONST_DOUBLE shift
amount.

I'm going to bootstrap & regtest this on x86_64-unknown-linux-gnu
(with again zero testing coverage ...).  The patch fixes the
reported ICE with a cross to cris-elf, more testing is appreciated
(though I guess autotesters will pick it up).

Does it look sane?

Thanks,
Richard.

2011-05-06  Richard Guenther  <rguenther@suse.de>

	PR middle-end/48908
	* expmed.c (expand_shift_1): Compute adjusted constant shift
	amount manually.

Index: gcc/expmed.c
===================================================================
*** gcc/expmed.c	(revision 173473)
--- gcc/expmed.c	(working copy)
*************** expand_shift_1 (enum tree_code code, enu
*** 2141,2151 ****
  	      rtx new_amount, other_amount;
  	      rtx temp1;
  
  	      new_amount = op1;
! 	      other_amount
! 		= simplify_gen_binary (MINUS, GET_MODE (op1),
! 				       GEN_INT (GET_MODE_BITSIZE (mode)),
! 				       op1);
  
  	      shifted = force_reg (mode, shifted);
  
--- 2141,2156 ----
  	      rtx new_amount, other_amount;
  	      rtx temp1;
  
+ 	      op1_mode = GET_MODE (op1);
  	      new_amount = op1;
! 	      if (op1_mode == VOIDmode)
! 		other_amount = GEN_INT (GET_MODE_BITSIZE (mode)
! 					- INTVAL (op1));
! 	      else
! 		other_amount
! 		  = simplify_gen_binary (MINUS, op1_mode,
! 					 GEN_INT (GET_MODE_BITSIZE (mode)),
! 					 op1);
  
  	      shifted = force_reg (mode, shifted);
  

Re: [PATCH] Cleanup expand_shift

[Eric Botcazou]

> I'm going to bootstrap & regtest this on x86_64-unknown-linux-gnu
> (with again zero testing coverage ...).  The patch fixes the
> reported ICE with a cross to cris-elf, more testing is appreciated
> (though I guess autotesters will pick it up).
>
> Does it look sane?

Yes, I think so, but...

> Index: gcc/expmed.c
> ===================================================================
> *** gcc/expmed.c	(revision 173473)
> --- gcc/expmed.c	(working copy)
> *************** expand_shift_1 (enum tree_code code, enu
> *** 2141,2151 ****
>   	      rtx new_amount, other_amount;
>   	      rtx temp1;
>
>   	      new_amount = op1;
> ! 	      other_amount
> ! 		= simplify_gen_binary (MINUS, GET_MODE (op1),
> ! 				       GEN_INT (GET_MODE_BITSIZE (mode)),
> ! 				       op1);
>
>   	      shifted = force_reg (mode, shifted);
>
> --- 2141,2156 ----
>   	      rtx new_amount, other_amount;
>   	      rtx temp1;
>
> + 	      op1_mode = GET_MODE (op1);
>   	      new_amount = op1;
> ! 	      if (op1_mode == VOIDmode)
> ! 		other_amount = GEN_INT (GET_MODE_BITSIZE (mode)
> ! 					- INTVAL (op1));
> ! 	      else
> ! 		other_amount
> ! 		  = simplify_gen_binary (MINUS, op1_mode,
> ! 					 GEN_INT (GET_MODE_BITSIZE (mode)),
> ! 					 op1);
>
>   	      shifted = force_reg (mode, shifted);

... I'd test CONST_INT_P (op1) instead of op1_mode == VOIDmode since you are 
accessing INTVAL in the branch.

-- 
Eric Botcazou