public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 0/3] MSP430: Improve code-generation for shift instructions
@ 2020-07-21 18:17 Jozef Lawrynowicz
  2020-07-21 18:24 ` [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant Jozef Lawrynowicz
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Jozef Lawrynowicz @ 2020-07-21 18:17 UTC (permalink / raw)
  To: gcc-patches

The attached series of patches improve code-generation for MSP430 shift
instructions.
The first two patches are changes to generic areas of GCC, required for
the 3rd patch to have the desired effect.

Successfully regtested on trunk for x86_64-pc-linux-gnu (for the
generic changes) and msp430-elf.

Ok to apply?

Jozef Lawrynowicz (3):
  expr: Allow scalar_int_mode target mode when converting a constant
  expmed: Fix possible use of NULL_RTX return value from emit_store_flag
  MSP430: Simplify and extend shift instruction patterns

 gcc/config/msp430/constraints.md              |  10 +-
 gcc/config/msp430/msp430-protos.h             |   6 +-
 gcc/config/msp430/msp430.c                    | 272 +++++++++----
 gcc/config/msp430/msp430.md                   | 381 +++++-------------
 gcc/config/msp430/msp430.opt                  |   6 +
 gcc/config/msp430/predicates.md               |  13 +-
 gcc/doc/invoke.texi                           |  15 +-
 gcc/expmed.c                                  |  34 +-
 gcc/expr.c                                    |   2 +-
 .../gcc.target/msp430/emulate-srli.c          |   2 +-
 .../msp430/max-inline-shift-430-no-opt.c      |  52 +++
 .../gcc.target/msp430/max-inline-shift-430.c  |  50 +++
 .../gcc.target/msp430/max-inline-shift-430x.c |  48 +++
 libgcc/config/msp430/slli.S                   |  15 +
 libgcc/config/msp430/srai.S                   |  15 +
 libgcc/config/msp430/srli.S                   |  16 +
 16 files changed, 548 insertions(+), 389 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c

--
2.27.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant
  2020-07-21 18:17 [PATCH 0/3] MSP430: Improve code-generation for shift instructions Jozef Lawrynowicz
@ 2020-07-21 18:24 ` Jozef Lawrynowicz
  2020-07-22  8:33   ` Richard Sandiford
  2020-07-21 18:26 ` [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from emit_store_flag Jozef Lawrynowicz
  2020-07-21 18:29 ` [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns Jozef Lawrynowicz
  2 siblings, 1 reply; 10+ messages in thread
From: Jozef Lawrynowicz @ 2020-07-21 18:24 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

is_int_mode does not allow MODE_PARTIAL_INT modes, so convert_modes was
not allowing a constant value to be converted to a MODE_PARTIAL_INT for
use as operand 2 in patterns such as ashlpsi3. The constant had
to be copied into a register before it could be used, but now can be
used directly as an operand without any copying.

Successfully regtested on trunk for x86_64-pc-linux-gnu and msp430-elf.
Ok to apply?

[-- Attachment #2: 0001-expr-Allow-scalar_int_mode-target-mode-when-converti.patch --]
[-- Type: text/plain, Size: 1255 bytes --]

From 485feafad6ef0966ff0e1d2ae383cd2b6dbc5c96 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Wed, 15 Jul 2020 11:19:16 +0100
Subject: [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a
 constant

is_int_mode does not allow MODE_PARTIAL_INT modes, so convert_modes was
not allowing a constant value to be converted to a MODE_PARTIAL_INT for
use as operand 2 in patterns such as ashlpsi3. The constant had
to be copied into a register before it could be used, but now can be
used directly as an operand without any copying.

gcc/ChangeLog:

	* expr.c (convert_modes): Allow a constant integer to be converted to
	any scalar int mode.

---
 gcc/expr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/expr.c b/gcc/expr.c
index c7c3e9fd655..5a252f0935f 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -696,7 +696,7 @@ convert_modes (machine_mode mode, machine_mode oldmode, rtx x, int unsignedp)
     return x;
 
   if (CONST_SCALAR_INT_P (x)
-      && is_int_mode (mode, &int_mode))
+      && is_a <scalar_int_mode> (mode, &int_mode))
     {
       /* If the caller did not tell us the old mode, then there is not
 	 much to do with respect to canonicalization.  We have to
-- 
2.27.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from emit_store_flag
  2020-07-21 18:17 [PATCH 0/3] MSP430: Improve code-generation for shift instructions Jozef Lawrynowicz
  2020-07-21 18:24 ` [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant Jozef Lawrynowicz
@ 2020-07-21 18:26 ` Jozef Lawrynowicz
  2020-07-22  8:38   ` Richard Sandiford
  2020-07-21 18:29 ` [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns Jozef Lawrynowicz
  2 siblings, 1 reply; 10+ messages in thread
From: Jozef Lawrynowicz @ 2020-07-21 18:26 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 234 bytes --]

MSP430 does not support have any store-flag instructions, so
emit_store_flag can return NULL_RTX.  Catch the NULL_RTX in
expmed.c:expand_sdiv_pow2.

Successfully regtested on trunk for x86_64-pc-linux-gnu and msp430-elf.
Ok to apply?

[-- Attachment #2: 0002-expmed-Fix-possible-use-of-NULL_RTX-return-value-fro.patch --]
[-- Type: text/plain, Size: 2669 bytes --]

From cb0f8219ac90229c0336281d73f511c107c877d3 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Wed, 15 Jul 2020 11:19:31 +0100
Subject: [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from
 emit_store_flag

MSP430 does not support have any store-flag instructions, so
emit_store_flag can return NULL_RTX.  Catch the NULL_RTX in
expmed.c:expand_sdiv_pow2.

gcc/ChangeLog:

	* expmed.c (expand_sdiv_pow2): Check return value from emit_store_flag
	is not NULL_RTX before use.

---
 gcc/expmed.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/gcc/expmed.c b/gcc/expmed.c
index e7c03fbf92c..d3a1735d39e 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -4086,9 +4086,12 @@ expand_sdiv_pow2 (scalar_int_mode mode, rtx op0, HOST_WIDE_INT d)
     {
       temp = gen_reg_rtx (mode);
       temp = emit_store_flag (temp, LT, op0, const0_rtx, mode, 0, 1);
-      temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
-			   0, OPTAB_LIB_WIDEN);
-      return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
+      if (temp != NULL_RTX)
+	{
+	  temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
+			       0, OPTAB_LIB_WIDEN);
+	  return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
+	}
     }
 
   if (HAVE_conditional_move
@@ -4122,17 +4125,20 @@ expand_sdiv_pow2 (scalar_int_mode mode, rtx op0, HOST_WIDE_INT d)
 
       temp = gen_reg_rtx (mode);
       temp = emit_store_flag (temp, LT, op0, const0_rtx, mode, 0, -1);
-      if (GET_MODE_BITSIZE (mode) >= BITS_PER_WORD
-	  || shift_cost (optimize_insn_for_speed_p (), mode, ushift)
-	     > COSTS_N_INSNS (1))
-	temp = expand_binop (mode, and_optab, temp, gen_int_mode (d - 1, mode),
-			     NULL_RTX, 0, OPTAB_LIB_WIDEN);
-      else
-	temp = expand_shift (RSHIFT_EXPR, mode, temp,
-			     ushift, NULL_RTX, 1);
-      temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
-			   0, OPTAB_LIB_WIDEN);
-      return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
+      if (temp != NULL_RTX)
+	{
+	  if (GET_MODE_BITSIZE (mode) >= BITS_PER_WORD
+	      || shift_cost (optimize_insn_for_speed_p (), mode, ushift)
+	      > COSTS_N_INSNS (1))
+	    temp = expand_binop (mode, and_optab, temp, gen_int_mode (d - 1, mode),
+				 NULL_RTX, 0, OPTAB_LIB_WIDEN);
+	  else
+	    temp = expand_shift (RSHIFT_EXPR, mode, temp,
+				 ushift, NULL_RTX, 1);
+	  temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
+			       0, OPTAB_LIB_WIDEN);
+	  return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
+	}
     }
 
   label = gen_label_rtx ();
-- 
2.27.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns
  2020-07-21 18:17 [PATCH 0/3] MSP430: Improve code-generation for shift instructions Jozef Lawrynowicz
  2020-07-21 18:24 ` [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant Jozef Lawrynowicz
  2020-07-21 18:26 ` [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from emit_store_flag Jozef Lawrynowicz
@ 2020-07-21 18:29 ` Jozef Lawrynowicz
  2020-08-07 10:55   ` ping " Jozef Lawrynowicz
  2020-08-25 19:16   ` Jeff Law
  2 siblings, 2 replies; 10+ messages in thread
From: Jozef Lawrynowicz @ 2020-07-21 18:29 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

The implementation of define_expand and define_insn patterns to handle
shifts in the MSP430 backend is inconsistent, resulting in missed
opportunities to make best use of the architecture's features.

There's now a single define_expand used as the entry point for all valid
shifts, and the decision to either use a helper function to perform the
shift (often required for the 430 ISA), or fall through to the
define_insn patterns can be made from that expander function.

Shifts by a constant amount have been grouped into one define_insn for
each type of shift, instead of having different define_insn patterns for
shifts by different amounts.

A new target option "-mmax-inline-shift=" has been added to allow tuning
of the number of shift instructions to emit inline, instead of using
a library helper function.

Successfully regtested on trunk for msp430-elf, ok to apply?

[-- Attachment #2: 0003-MSP430-Simplify-and-extend-shift-instruction-pattern.patch --]
[-- Type: text/plain, Size: 43993 bytes --]

From a3c62c448c7f359bad85c86c35f712ca1fccf219 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Wed, 15 Jul 2020 11:43:36 +0100
Subject: [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns

The implementation of define_expand and define_insn patterns to handle
shifts in the MSP430 backend is inconsistent, resulting in missed
opportunities to make best use of the architecture's features.

There's now a single define_expand used as the entry point for all valid
shifts, and the decision to either use a helper function to perform the
shift (often required for the 430 ISA), or fall through to the
define_insn patterns can be made from that expander function.

Shifts by a constant amount have been grouped into one define_insn for
each type of shift, instead of having different define_insn patterns for
shifts by different amounts.

A new target option "-mmax-inline-shift=" has been added to allow tuning
of the number of shift instructions to emit inline, instead of using
a library helper function.

gcc/ChangeLog:

	* config/msp430/constraints.md (K): Change unused constraint to
	constraint to a const_int between 1 and 19.
	(P): New constraint.
	* config/msp430/msp430-protos.h (msp430x_logical_shift_right): Remove.
	(msp430_expand_shift): New.
	(msp430_output_asm_shift_insns): New.
	* config/msp430/msp430.c (msp430_rtx_costs): Remove shift costs.
	(CSH): Remove.
	(msp430_expand_helper): Remove hard-coded generation of some inline
	shift insns.
	(use_helper_for_const_shift): New.
	(msp430_expand_shift): New.
	(msp430_output_asm_shift_insns): New.
	(msp430_print_operand): Add new 'W' operand selector.
	(msp430x_logical_shift_right): Remove.
	* config/msp430/msp430.md (HPSI): New define_mode_iterator.
	(HDI): Likewise.
	(any_shift): New define_code_iterator.
	(shift_insn): New define_code_attr.
	Adjust unnamed insn patterns searched for by combine.
	(ashlhi3): Remove.
	(slli_1): Remove.
	(430x_shift_left): Remove.
	(slll_1): Remove.
	(slll_2): Remove.
	(ashlsi3): Remove.
	(ashldi3): Remove.
	(ashrhi3): Remove.
	(srai_1): Remove.
	(430x_arithmetic_shift_right): Remove.
	(srap_1): Remove.
	(srap_2): Remove.
	(sral_1): Remove.
	(sral_2): Remove.
	(ashrsi3): Remove.
	(ashrdi3): Remove.
	(lshrhi3): Remove.
	(srli_1): Remove.
	(430x_logical_shift_right): Remove.
	(srlp_1): Remove.
	(srll_1): Remove.
	(srll_2x): Remove.
	(lshrsi3): Remove.
	(lshrdi3): Remove.
	(<shift_insn><mode>3): New define_expand.
	(<shift_insn>hi3_430): New define_insn.
	(<shift_insn>si3_const): Likewise.
	(ashl<mode>3_430x): Likewise.
	(ashr<mode>3_430x): Likewise.
	(lshr<mode>3_430x): Likewise.
	(*bitbranch<mode>4_z): Replace renamed predicate msp430_bitpos with
	const_0_to_15_operand.
	* config/msp430/msp430.opt: New option -mmax-inline-shift=.
	* config/msp430/predicates.md (const_1_to_8_operand): New predicate.
	(const_0_to_15_operand): Rename msp430_bitpos predicate.
	(const_1_to_19_operand): New predicate.
	* doc/invoke.texi: Document -mmax-inline-shift=.

libgcc/ChangeLog:

	* config/msp430/slli.S (__gnu_mspabi_sllp): New.
	* config/msp430/srai.S (__gnu_mspabi_srap): New.
	* config/msp430/srli.S (__gnu_mspabi_srlp): New.

gcc/testsuite/ChangeLog:

	* gcc.target/msp430/emulate-srli.c: Fix expected assembler text.
	* gcc.target/msp430/max-inline-shift-430-no-opt.c: New test.
	* gcc.target/msp430/max-inline-shift-430.c: New test.
	* gcc.target/msp430/max-inline-shift-430x.c: New test.

---
 gcc/config/msp430/constraints.md              |  10 +-
 gcc/config/msp430/msp430-protos.h             |   6 +-
 gcc/config/msp430/msp430.c                    | 272 +++++++++----
 gcc/config/msp430/msp430.md                   | 381 +++++-------------
 gcc/config/msp430/msp430.opt                  |   6 +
 gcc/config/msp430/predicates.md               |  13 +-
 gcc/doc/invoke.texi                           |  15 +-
 .../gcc.target/msp430/emulate-srli.c          |   2 +-
 .../msp430/max-inline-shift-430-no-opt.c      |  52 +++
 .../gcc.target/msp430/max-inline-shift-430.c  |  50 +++
 .../gcc.target/msp430/max-inline-shift-430x.c |  48 +++
 libgcc/config/msp430/slli.S                   |  15 +
 libgcc/config/msp430/srai.S                   |  15 +
 libgcc/config/msp430/srli.S                   |  16 +
 14 files changed, 527 insertions(+), 374 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c

diff --git a/gcc/config/msp430/constraints.md b/gcc/config/msp430/constraints.md
index 14368f7be6d..b8f9674b2fe 100644
--- a/gcc/config/msp430/constraints.md
+++ b/gcc/config/msp430/constraints.md
@@ -25,15 +25,16 @@ (define_register_constraint "R13" "R13_REGS"
   "Register R13.")
 
 (define_constraint "K"
-  "Integer constant 1."
+  "Integer constant 1-19."
   (and (match_code "const_int")
-       (match_test "IN_RANGE (ival, 1, 1)")))
+       (match_test "IN_RANGE (ival, 1, 19)")))
 
 (define_constraint "L"
   "Integer constant -1^20..1^19."
   (and (match_code "const_int")
        (match_test "IN_RANGE (ival, HOST_WIDE_INT_M1U << 20, 1 << 19)")))
 
+;; Valid shift amount for RRUM, RRAM, RLAM, RRCM.
 (define_constraint "M"
   "Integer constant 1-4."
   (and (match_code "const_int")
@@ -49,6 +50,11 @@ (define_constraint "O"
   (and (match_code "const_int")
        (match_test "IN_RANGE (ival, 256, 65535)")))
 
+(define_constraint "P"
+  "Integer constant 1-16."
+  (and (match_code "const_int")
+       (match_test "IN_RANGE (ival, 1, 16)")))
+
 ;; We do not allow arbitrary constants, eg symbols or labels,
 ;; because their address may be above the 16-bit address limit
 ;; supported by the offset used in the MOVA instruction.
diff --git a/gcc/config/msp430/msp430-protos.h b/gcc/config/msp430/msp430-protos.h
index a13a94cb92c..0b4d9a42b41 100644
--- a/gcc/config/msp430/msp430-protos.h
+++ b/gcc/config/msp430/msp430-protos.h
@@ -35,7 +35,6 @@ rtx	msp430_incoming_return_addr_rtx (void);
 void	msp430_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree, int);
 int	msp430_initial_elimination_offset (int, int);
 bool    msp430_is_interrupt_func (void);
-const char * msp430x_logical_shift_right (rtx);
 const char * msp430_mcu_name (void);
 void    msp430_output_aligned_decl_common (FILE *, const tree, const char *,
 					   unsigned HOST_WIDE_INT, unsigned,
@@ -51,4 +50,9 @@ bool    msp430_use_f5_series_hwmult (void);
 bool	msp430_has_hwmult (void);
 bool msp430_op_not_in_high_mem (rtx op);
 
+#ifdef RTX_CODE
+int msp430_expand_shift (enum rtx_code code, machine_mode mode, rtx *operands);
+const char * msp430_output_asm_shift_insns (enum rtx_code code, machine_mode mode, rtx *operands);
+#endif
+
 #endif /* GCC_MSP430_PROTOS_H */
diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 455b4af3dad..c2b24974364 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -1064,15 +1064,6 @@ static bool msp430_rtx_costs (rtx	   x ATTRIBUTE_UNUSED,
 	  return true;
 	}
       break;
-    case ASHIFT:
-    case ASHIFTRT:
-    case LSHIFTRT:
-      if (!msp430x)
-	{
-	  *total = COSTS_N_INSNS (100);
-	  return true;
-	}
-      break;
     }
   return false;
 }
@@ -2674,32 +2665,6 @@ msp430_init_dwarf_reg_sizes_extra (tree address)
     }
 }
 
-/* This is a list of MD patterns that implement fixed-count shifts.  */
-static struct
-{
-  const char *name;
-  int count;
-  int need_430x;
-  rtx (*genfunc)(rtx,rtx);
-}
-const_shift_helpers[] =
-{
-#define CSH(N,C,X,G) { "__mspabi_" N, C, X, gen_##G }
-
-  CSH ("slli", 1, 1, slli_1),
-  CSH ("slll", 1, 1, slll_1),
-  CSH ("slll", 2, 1, slll_2),
-
-  CSH ("srai", 1, 0, srai_1),
-  CSH ("sral", 1, 0, sral_1),
-  CSH ("sral", 2, 0, sral_2),
-
-  CSH ("srll", 1, 0, srll_1),
-  CSH ("srll", 2, 1, srll_2x),
-  { 0, 0, 0, 0 }
-#undef CSH
-};
-
 /* The MSP430 ABI defines a number of helper functions that should be
    used for, for example, 32-bit shifts.  This function is called to
    emit such a function, using the table above to optimize some
@@ -2716,31 +2681,12 @@ msp430_expand_helper (rtx *operands, const char *helper_name,
   machine_mode arg0mode = GET_MODE (operands[0]);
   machine_mode arg1mode = GET_MODE (operands[1]);
   machine_mode arg2mode = GET_MODE (operands[2]);
-  int have_430x = msp430x ? 1 : 0;
   int expand_mpy = strncmp (helper_name, "__mspabi_mpy",
 			    sizeof ("__mspabi_mpy") - 1) == 0;
   /* This function has been used incorrectly if CONST_VARIANTS is TRUE for a
      hwmpy function.  */
   gcc_assert (!(expand_mpy && const_variants));
 
-  /* Emit size-optimal insns for small shifts we can easily do inline.  */
-  if (CONST_INT_P (operands[2]) && !expand_mpy)
-    {
-      int i;
-
-      for (i=0; const_shift_helpers[i].name; i++)
-	{
-	  if (const_shift_helpers[i].need_430x <= have_430x
-	      && strcmp (helper_name, const_shift_helpers[i].name) == 0
-	      && INTVAL (operands[2]) == const_shift_helpers[i].count)
-	    {
-	      emit_insn (const_shift_helpers[i].genfunc (operands[0],
-							 operands[1]));
-	      return;
-	    }
-	}
-    }
-
   if (arg1mode != VOIDmode && arg2mode != VOIDmode)
     /* Modes of arguments must be equal if not constants.  */
     gcc_assert (arg1mode == arg2mode);
@@ -2835,6 +2781,190 @@ msp430_expand_helper (rtx *operands, const char *helper_name,
 		  gen_rtx_REG (arg0mode, 12));
 }
 
+/* Return TRUE if the helper function should be used and FALSE if the shifts
+   insns should be emitted inline.  */
+static bool
+use_helper_for_const_shift (enum rtx_code code, machine_mode mode,
+			    HOST_WIDE_INT amt)
+{
+  const int default_inline_shift = 4;
+  /* We initialize the option to 65 so we know if the user set it or not.  */
+  int user_set_max_inline = (msp430_max_inline_shift == 65 ? 0 : 1);
+  int max_inline = (user_set_max_inline ? msp430_max_inline_shift
+		    : default_inline_shift);
+  /* 32-bit shifts are roughly twice as costly as 16-bit shifts so we adjust
+     the heuristic accordingly.  */
+  int max_inline_32 = max_inline / 2;
+
+  /* Don't use helpers for these modes on 430X, when optimizing for speed, or
+     when emitting a small number of insns.  */
+  if ((mode == E_QImode || mode == E_HImode || mode == E_PSImode)
+      && (msp430x
+	  /* If the user set max_inline then we always obey that number.
+	     Otherwise we always emit the shifts inline at -O2 and above.  */
+	  || amt <= max_inline
+	  || (!user_set_max_inline
+	      && (optimize >= 2 && !optimize_size))))
+    return false;
+
+  /* 430 and 430X codegen for SImode shifts is the same.
+     Set a hard limit of 15 for the number of shifts that will be emitted
+     inline by default, even at -O2 and above, to prevent code size
+     explosion.  */
+  if (mode == E_SImode
+      && (amt <= max_inline_32
+	  || (!user_set_max_inline
+	      && (optimize >= 2 && !optimize_size)
+	      && amt <= 15)))
+    return false;
+
+  return true;
+}
+
+/* For shift operations which will use an mspabi helper function, setup the
+   call to msp430_expand helper.  Return 1 to indicate we have finished with
+   this insn and invoke "DONE".
+   Otherwise return 0 to indicate the insn should fallthrough.
+   Never FAIL.  */
+int
+msp430_expand_shift (enum rtx_code code, machine_mode mode, rtx *operands)
+{
+  /* Always use the helper function when the shift amount is not a
+     constant.  */
+  if (!CONST_INT_P (operands[2])
+      || mode == E_DImode
+      || use_helper_for_const_shift (code, mode, INTVAL (operands[2])))
+    {
+      const char *helper_name = NULL;
+      /* The const variants of mspabi shifts have significantly larger code
+	 size than the generic version, so use the generic version if
+	 optimizing for size.  */
+      bool const_variant = !optimize_size;
+      switch (mode)
+	{
+	case E_HImode:
+	  helper_name = (code == ASHIFT ? "__mspabi_slli" :
+			 (code == ASHIFTRT ? "__mspabi_srai" :
+			  (code == LSHIFTRT ? "__mspabi_srli" :
+			   NULL)));
+	  break;
+	case E_PSImode:
+	  helper_name = (code == ASHIFT ? "__gnu_mspabi_sllp" :
+			 (code == ASHIFTRT ? "__gnu_mspabi_srap" :
+			  (code == LSHIFTRT ? "__gnu_mspabi_srlp" :
+			   NULL)));
+	  /* No const variant for PSImode shifts FIXME.  */
+	  const_variant = false;
+	  break;
+	case E_SImode:
+	  helper_name = (code == ASHIFT ? "__mspabi_slll" :
+			 (code == ASHIFTRT ? "__mspabi_sral" :
+			  (code == LSHIFTRT ? "__mspabi_srll" :
+			   NULL)));
+	  break;
+	case E_DImode:
+	  helper_name = (code == ASHIFT ? "__mspabi_sllll" :
+			 (code == ASHIFTRT ? "__mspabi_srall" :
+			  (code == LSHIFTRT ? "__mspabi_srlll" :
+			   NULL)));
+	  /* No const variant for DImode shifts.  */
+	  const_variant = false;
+	  break;
+	default:
+	  gcc_unreachable ();
+	  break;
+	}
+      gcc_assert (helper_name);
+      msp430_expand_helper (operands, helper_name, const_variant);
+      return 1;
+    }
+  /* When returning 0, there must be an insn to match the RTL pattern
+     otherwise there will be an unrecognizeable insn.  */
+  return 0;
+}
+
+/* Helper function to emit a sequence of shift instructions.  The amount of
+   shift instructions to emit is in OPERANDS[2].
+   For 430 we output copies of identical inline shifts for all modes.
+   For 430X it is inneficient to do so for any modes except SI and DI, since we
+   can make use of R*M insns or RPT with 430X insns, so this function is only
+   used for SImode in that case.  */
+const char *
+msp430_output_asm_shift_insns (enum rtx_code code, machine_mode mode,
+			       rtx *operands)
+{
+  int i;
+  int amt;
+  int max_shift = GET_MODE_BITSIZE (mode) - 1;
+  gcc_assert (CONST_INT_P (operands[2]));
+  amt = INTVAL (operands[2]);
+
+  if (amt == 0 || amt > max_shift)
+    {
+      switch (code)
+	{
+	case ASHIFT:
+	  output_asm_insn ("# ignored undefined behaviour left shift "
+			   "of %1 by %2", operands);
+	  break;
+	case ASHIFTRT:
+	  output_asm_insn ("# ignored undefined behaviour arithmetic right "
+			   "shift of %1 by %2", operands);
+	  break;
+	case LSHIFTRT:
+	  output_asm_insn ("# ignored undefined behaviour logical right shift "
+			   "of %1 by %2", operands);
+	  break;
+	default:
+	  gcc_unreachable ();
+	}
+      return "";
+    }
+
+  if (code == ASHIFT)
+    {
+      if (!msp430x && mode == HImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("RLA.W\t%0", operands);
+      else if (mode == SImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("RLA%X0.W\t%L0 { RLC%X0.W\t%H0", operands);
+      else
+	/* Catch unhandled cases.  */
+	gcc_unreachable ();
+    }
+  else if (code == ASHIFTRT)
+    {
+      if (!msp430x && mode == HImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("RRA.W\t%0", operands);
+      else if (mode == SImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("RRA%X0.W\t%H0 { RRC%X0.W\t%L0", operands);
+      else
+	gcc_unreachable ();
+    }
+  else if (code == LSHIFTRT)
+    {
+      if (!msp430x && mode == HImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("CLRC { RRC.W\t%0", operands);
+      else if (mode == SImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("CLRC { RRC%X0.W\t%H0 { RRC%X0.W\t%L0", operands);
+      /* FIXME: Why doesn't "RRUX.W\t%H0 { RRC%X0.W\t%L0" work for msp430x?
+	 It causes execution timeouts e.g. pr41963.c.  */
+#if 0
+      else if (msp430x && mode == SImode)
+	for (i = 0; i < amt; i++)
+	  output_asm_insn ("RRUX.W\t%H0 { RRC%X0.W\t%L0", operands);
+#endif
+      else
+	gcc_unreachable ();
+    }
+  return "";
+}
+
 /* Called by cbranch<mode>4 to coerce operands into usable forms.  */
 void
 msp430_fixup_compare_operands (machine_mode my_mode, rtx * operands)
@@ -3368,6 +3498,7 @@ msp430_op_not_in_high_mem (rtx op)
    O   offset of the top of the stack
    Q   like X but generates an A postfix
    R   inverse of condition code, unsigned.
+   W   value - 16
    X   X instruction postfix in large mode
    Y   value - 4
    Z   value - 1
@@ -3394,6 +3525,11 @@ msp430_print_operand (FILE * file, rtx op, int letter)
       /* Print the constant value, less four.  */
       fprintf (file, "#%ld", INTVAL (op) - 4);
       return;
+    case 'W':
+      gcc_assert (CONST_INT_P (op));
+      /* Print the constant value, less 16.  */
+      fprintf (file, "#%ld", INTVAL (op) - 16);
+      return;
     case 'I':
       if (GET_CODE (op) == CONST_INT)
 	{
@@ -3711,34 +3847,6 @@ msp430x_extendhisi (rtx * operands)
   return "MOV.W\t%1, %L0 { MOV.W\t%1, %H0 { RPT\t#15 { RRAX.W\t%H0";
 }
 
-/* Likewise for logical right shifts.  */
-const char *
-msp430x_logical_shift_right (rtx amount)
-{
-  /* The MSP430X's logical right shift instruction - RRUM - does
-     not use an extension word, so we cannot encode a repeat count.
-     Try various alternatives to work around this.  If the count
-     is in a register we are stuck, hence the assert.  */
-  gcc_assert (CONST_INT_P (amount));
-
-  if (INTVAL (amount) <= 0
-      || INTVAL (amount) >= 16)
-    return "# nop logical shift.";
-
-  if (INTVAL (amount) > 0
-      && INTVAL (amount) < 5)
-    return "rrum.w\t%2, %0"; /* Two bytes.  */
-
-  if (INTVAL (amount) > 4
-      && INTVAL (amount) < 9)
-    return "rrum.w\t#4, %0 { rrum.w\t%Y2, %0 "; /* Four bytes.  */
-
-  /* First we logically shift right by one.  Now we know
-     that the top bit is zero and we can use the arithmetic
-     right shift instruction to perform the rest of the shift.  */
-  return "rrum.w\t#1, %0 { rpt\t%Z2 { rrax.w\t%0"; /* Six bytes.  */
-}
-
 /* Stop GCC from thinking that it can eliminate (SUBREG:PSI (SI)).  */
 
 #undef TARGET_CAN_CHANGE_MODE_CLASS
diff --git a/gcc/config/msp430/msp430.md b/gcc/config/msp430/msp430.md
index ed21eb02868..f70e61b97dd 100644
--- a/gcc/config/msp430/msp430.md
+++ b/gcc/config/msp430/msp430.md
@@ -65,6 +65,15 @@ (define_attr "length" "" (const_int 4))
 (include "constraints.md")
 
 (define_mode_iterator QHI [QI HI PSI])
+(define_mode_iterator HPSI [HI PSI])
+(define_mode_iterator HDI [HI PSI SI DI])
+
+;; Mapping of all shift operators
+(define_code_iterator any_shift [ashift ashiftrt lshiftrt])
+
+;; Base name for define_insn
+(define_code_attr shift_insn
+  [(ashift "ashl") (lshiftrt "lshr") (ashiftrt "ashr")])
 
 ;; There are two basic "family" tests we do here:
 ;;
@@ -689,31 +698,42 @@ (define_insn ""
    MOV%X1.B\t%1, %0"
 )
 
+;; The next three insns emit identical assembly code.
+;; They take a QImode and shift it in SImode.  Only shift counts <= 8
+;; are handled since that is the simple case where the high 16-bits (i.e. the
+;; high register) are always 0.
 (define_insn ""
-  [(set (match_operand:SI 0 "register_operand" "=r")
-	(ashift:SI (zero_extend:SI (match_operand:QI 1 "general_operand" "rm"))
-		   (match_operand:HI 2 "immediate_operand" "M")))]
+  [(set (match_operand:SI			     0 "register_operand" "=r,r,r")
+	(ashift:SI (zero_extend:SI (match_operand:QI 1 "general_operand" "0,rm,rm"))
+		   (match_operand:HI		     2 "const_1_to_8_operand" "M,M,i")))]
   "msp430x"
-  "MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0"
+  "@
+  RLAM.W %2, %L0 { CLR %H0
+  MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0
+  MOV%X1.B %1, %L0 { RPT %2 { RLAX.W %L0 { CLR %H0"
 )
 
-;; We are taking a char and shifting it and putting the result in 2 registers.
-;; the high register will always be for 0 shift counts < 8.
 (define_insn ""
-  [(set (match_operand:SI 0 "register_operand" "=r")
-	(ashift:SI (zero_extend:SI (subreg:HI (match_operand:QI 1 "general_operand" "rm") 0))
-		   (match_operand:HI 2 "immediate_operand" "M")))]
+  [(set (match_operand:SI			     		0 "register_operand" "=r,r,r")
+	(ashift:SI (zero_extend:SI (subreg:HI (match_operand:QI 1 "general_operand" "0,rm,rm") 0))
+		   (match_operand:HI		     		2 "const_1_to_8_operand" "M,M,i")))]
   "msp430x"
-  "MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0"
+  "@
+  RLAM.W %2, %L0 { CLR %H0
+  MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0
+  MOV%X1.B %1, %L0 { RPT %2 { RLAX.W %L0 { CLR %H0"
 )
 
 ;; Same as above but with a NOP sign_extend round the subreg
 (define_insn ""
-  [(set (match_operand:SI 0 "register_operand" "=r")
-	(ashift:SI (zero_extend:SI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "rm") 0)))
-		   (match_operand:HI 2 "immediate_operand" "M")))]
+  [(set (match_operand:SI			     				 0 "register_operand" "=r,r,r")
+	(ashift:SI (zero_extend:SI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "0,rm,rm") 0)))
+		   (match_operand:HI		     				 2 "const_1_to_8_operand" "M,M,i")))]
   "msp430x"
-  "MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0"
+  "@
+  RLAM.W %2, %L0 { CLR %H0
+  MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0
+  MOV%X1.B %1, %L0 { RPT %2 { RLAX.W %L0 { CLR %H0"
 )
 
 (define_insn ""
@@ -724,11 +744,14 @@ (define_insn ""
 )
 
 (define_insn ""
-  [(set (match_operand:PSI 0 "register_operand" "=r")
-	(ashift:PSI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "rm") 0))
-		    (match_operand:HI 2 "immediate_operand" "M")))]
+  [(set (match_operand:PSI					  0 "register_operand" "=r,r,r")
+	(ashift:PSI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "0,rm,rm") 0))
+		    (match_operand:HI				  2 "const_1_to_19_operand" "M,M,i")))]
   "msp430x"
-  "MOV%X1.B %1, %0 { RLAM.W %2, %0"
+  "@
+  RLAM.W %2, %0
+  MOV%X1.B %1, %0 { RLAM.W %2, %0
+  MOV%X1.B %1, %0 { RPT %2 { RLAX.A %0"
 )
 ;; END msp430 pointer manipulation combine insn patterns
 
@@ -840,287 +863,75 @@ (define_insn "truncsipsi2"
 ;; Note - we ignore shift counts of less than one or more than 15.
 ;; This is permitted by the ISO C99 standard as such shifts result
 ;; in "undefined" behavior.  [6.5.7 (3)]
+;;
+;; We avoid emitting insns in msp430_expand_shift, since we would have to handle
+;; many extra cases such as op0 != op1, or, op0 or op1 in memory.  Instead we
+;; let reload coerce op0 and op1 into the same register.
 
-;; signed A << C
-
-(define_expand "ashlhi3"
-  [(set (match_operand:HI	     0 "msp430_general_dst_nonv_operand")
-	(ashift:HI (match_operand:HI 1 "general_operand")
-		   (match_operand:HI 2 "general_operand")))]
+(define_expand "<shift_insn><mode>3"
+  [(set (match_operand:HDI		  0 "msp430_general_dst_nonv_operand")
+	(any_shift:HDI (match_operand:HDI 1 "general_operand")
+		       (match_operand:HDI 2 "general_operand")))]
   ""
   {
-    if ((GET_CODE (operands[1]) == SUBREG
-	 && REG_P (XEXP (operands[1], 0)))
-	|| MEM_P (operands[1]))
-      operands[1] = force_reg (HImode, operands[1]);
-    if (msp430x
-        && REG_P (operands[0])
-        && REG_P (operands[1])
-        && CONST_INT_P (operands[2]))
-      emit_insn (gen_430x_shift_left (operands[0], operands[1], operands[2]));
-    else if (CONST_INT_P (operands[2])
-	     && INTVAL (operands[2]) == 1)
-      emit_insn (gen_slli_1 (operands[0], operands[1]));
-    else		 
-      /* The const variants of mspabi shifts have larger code size than the
-	 generic version, so use the generic version if optimizing for
-	 size.  */
-      msp430_expand_helper (operands, \"__mspabi_slli\", !optimize_size);
-    DONE;
+    if (msp430_expand_shift (<CODE>, <MODE>mode, operands))
+      DONE;
+    /* Otherwise, fallthrough.  */
   }
 )
 
-(define_insn "slli_1"
-  [(set (match_operand:HI	     0 "msp430_general_dst_nonv_operand" "=rm")
-	(ashift:HI (match_operand:HI 1 "general_operand"       "0")
-		   (const_int 1)))]
-  ""
-  "RLA%X0.W\t%0" ;; Note - this is a macro for ADD
-)
-
-(define_insn "430x_shift_left"
-  [(set (match_operand:HI            0 "register_operand" "=r")
-	(ashift:HI (match_operand:HI 1 "register_operand"  "0")
-		   (match_operand    2 "immediate_operand" "n")))]
-  "msp430x"
-  "*
-  if (INTVAL (operands[2]) > 0 && INTVAL (operands[2]) < 5)
-    return \"RLAM.W\t%2, %0\";
-  else if (INTVAL (operands[2]) >= 5 && INTVAL (operands[2]) < 16)
-    return \"RPT\t%2 { RLAX.W\t%0\";
-  return \"# nop left shift\";
-  "
+;; All 430 HImode constant shifts
+(define_insn "<shift_insn>hi3_430"
+  [(set (match_operand:HI		0 "msp430_general_dst_nonv_operand" "=rm")
+	(any_shift:HI (match_operand:HI 1 "general_operand"       "0")
+		      (match_operand:HI 2 "const_int_operand"     "n")))]
+  "!msp430x"
+  "* return msp430_output_asm_shift_insns (<CODE>, HImode, operands);"
 )
 
-(define_insn "slll_1"
-  [(set (match_operand:SI	     0 "msp430_general_dst_nonv_operand" "=rm")
-	(ashift:SI (match_operand:SI 1 "general_operand"       "0")
-		   (const_int 1)))]
+;; All 430 and 430X SImode constant shifts
+(define_insn "<shift_insn>si3_const"
+  [(set (match_operand:SI		0 "msp430_general_dst_nonv_operand" "=rm")
+	(any_shift:SI (match_operand:SI 1 "general_operand"       "0")
+		      (match_operand:SI 2 "const_int_operand"     "n")))]
   ""
-  "RLA%X0.W\t%L0 { RLC%X0.W\t%H0"
-)
-
-(define_insn "slll_2"
-  [(set (match_operand:SI	     0 "msp430_general_dst_nonv_operand" "=rm")
-	(ashift:SI (match_operand:SI 1 "general_operand"       "0")
-		   (const_int 2)))]
-  ""
-  "RLA%X0.W\t%L0 { RLC%X0.W\t%H0 { RLA%X0.W\t%L0 { RLC%X0.W\t%H0"
-)
-
-(define_expand "ashlsi3"
-  [(set (match_operand:SI	     0 "msp430_general_dst_nonv_operand")
-	(ashift:SI (match_operand:SI 1 "general_operand")
-		   (match_operand:SI 2 "general_operand")))]
-  ""
-  "msp430_expand_helper (operands, \"__mspabi_slll\", !optimize_size);
-   DONE;"
-)
-
-(define_expand "ashldi3"
-  [(set (match_operand:DI	     0 "msp430_general_dst_nonv_operand")
-	(ashift:DI (match_operand:DI 1 "general_operand")
-		   (match_operand:DI 2 "general_operand")))]
-  ""
-  {
-    /* No const_variant for 64-bit shifts.  */
-    msp430_expand_helper (operands, \"__mspabi_sllll\", false);
-    DONE;
-  }
-)
-
-;;----------
-
-;; signed A >> C
-
-(define_expand "ashrhi3"
-  [(set (match_operand:HI	       0 "msp430_general_dst_nonv_operand")
-	(ashiftrt:HI (match_operand:HI 1 "general_operand")
-		     (match_operand:HI 2 "general_operand")))]
-  ""
-  {
-    if ((GET_CODE (operands[1]) == SUBREG
-	 && REG_P (XEXP (operands[1], 0)))
-	|| MEM_P (operands[1]))
-      operands[1] = force_reg (HImode, operands[1]);
-    if (msp430x
-        && REG_P (operands[0])
-        && REG_P (operands[1])
-        && CONST_INT_P (operands[2]))
-      emit_insn (gen_430x_arithmetic_shift_right (operands[0], operands[1], operands[2]));
-    else if (CONST_INT_P (operands[2])
-	     && INTVAL (operands[2]) == 1)
-      emit_insn (gen_srai_1 (operands[0], operands[1]));
-    else		 
-       msp430_expand_helper (operands, \"__mspabi_srai\", !optimize_size);
-   DONE;
-   }
-)
-
-(define_insn "srai_1"
-  [(set (match_operand:HI	       0 "msp430_general_dst_operand" "=rm")
-	(ashiftrt:HI (match_operand:HI 1 "msp430_general_operand"      "0")
-		     (const_int 1)))]
-  ""
-  "RRA%X0.W\t%0"
-)
-
-(define_insn "430x_arithmetic_shift_right"
-  [(set (match_operand:HI              0 "register_operand" "=r")
-	(ashiftrt:HI (match_operand:HI 1 "register_operand"  "0")
-		     (match_operand    2 "immediate_operand" "n")))]
-  "msp430x"
-  "*
-  if (INTVAL (operands[2]) > 0 && INTVAL (operands[2]) < 5)
-    return \"RRAM.W\t%2, %0\";
-  else if (INTVAL (operands[2]) >= 5 && INTVAL (operands[2]) < 16)
-    return \"RPT\t%2 { RRAX.W\t%0\";
-  return \"# nop arith right shift\";
-  "
-)
-
-(define_insn "srap_1"
-  [(set (match_operand:PSI              0 "register_operand" "=r")
-	(ashiftrt:PSI (match_operand:PSI 1 "general_operand" "0")
-		      (const_int 1)))]
-  "msp430x"
-  "RRAM.A #1,%0"
+  "* return msp430_output_asm_shift_insns (<CODE>, SImode, operands);"
 )
 
-(define_insn "srap_2"
-  [(set (match_operand:PSI              0 "register_operand" "=r")
-	(ashiftrt:PSI (match_operand:PSI 1 "general_operand" "0")
-		      (const_int 2)))]
+(define_insn "ashl<mode>3_430x"
+  [(set (match_operand:HPSI		 0 "msp430_general_dst_nonv_operand" "=r,r,r,r")
+	(ashift:HPSI (match_operand:HPSI 1 "general_operand" 		     "0 ,0,0,0")
+		     (match_operand:HPSI 2 "const_int_operand" 		     "M ,P,K,i")))]
   "msp430x"
-  "RRAM.A #2,%0"
-)
-
-(define_insn "sral_1"
-  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=rm")
-	(ashiftrt:SI (match_operand:SI 1 "general_operand"       "0")
-		     (const_int 1)))]
-  ""
-  "RRA%X0.W\t%H0 { RRC%X0.W\t%L0"
-)
-
-(define_insn "sral_2"
-  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=rm")
-	(ashiftrt:SI (match_operand:SI 1 "general_operand"       "0")
-		     (const_int 2)))]
-  ""
-  "RRA%X0.W\t%H0 { RRC%X0.W\t%L0 { RRA%X0.W\t%H0 { RRC%X0.W\t%L0"
-)
-
-(define_expand "ashrsi3"
-  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand")
-	(ashiftrt:SI (match_operand:SI 1 "general_operand")
-		     (match_operand:SI 2 "general_operand")))]
-  ""
-  "msp430_expand_helper (operands, \"__mspabi_sral\", !optimize_size);
-   DONE;"
-)
-
-(define_expand "ashrdi3"
-  [(set (match_operand:DI	     0 "msp430_general_dst_nonv_operand")
-	(ashift:DI (match_operand:DI 1 "general_operand")
-		   (match_operand:DI 2 "general_operand")))]
-  ""
-  {
-    /* No const_variant for 64-bit shifts.  */
-    msp430_expand_helper (operands, \"__mspabi_srall\", false);
-    DONE;
-  }
-)
-
-;;----------
-
-;; unsigned A >> C
-
-(define_expand "lshrhi3"
-  [(set (match_operand:HI	       0 "msp430_general_dst_nonv_operand")
-	(lshiftrt:HI (match_operand:HI 1 "general_operand")
-		     (match_operand:HI 2 "general_operand")))]
-  ""
-  {
-    if ((GET_CODE (operands[1]) == SUBREG
-	 && REG_P (XEXP (operands[1], 0)))
-	|| MEM_P (operands[1]))
-      operands[1] = force_reg (HImode, operands[1]);
-    if (msp430x
-        && REG_P (operands[0])
-        && REG_P (operands[1])
-        && CONST_INT_P (operands[2]))
-      emit_insn (gen_430x_logical_shift_right (operands[0], operands[1], operands[2]));
-    else if (CONST_INT_P (operands[2])
-	     && INTVAL (operands[2]) == 1)
-      emit_insn (gen_srli_1 (operands[0], operands[1]));
-    else		 
-      msp430_expand_helper (operands, \"__mspabi_srli\", !optimize_size);
-    DONE;
-  }
-)
-
-(define_insn "srli_1"
-  [(set (match_operand:HI	       0 "msp430_general_dst_nonv_operand" "=rm")
-	(lshiftrt:HI (match_operand:HI 1 "general_operand"       "0")
-		     (const_int 1)))]
-  ""
-  "CLRC { RRC%X0.W\t%0"
+  "@
+  RLAM%b0\t%2, %0
+  RPT\t%2 { RLAX%b0\t%0
+  RPT\t#16 { RLAX%b0\t%0 { RPT\t%W2 { RLAX%b0\t%0
+  # undefined behavior left shift of %1 by %2"
 )
 
-(define_insn "430x_logical_shift_right"
-  [(set (match_operand:HI              0 "register_operand" "=r")
-	(lshiftrt:HI (match_operand:HI 1 "register_operand"  "0")
-		     (match_operand    2 "immediate_operand" "n")))]
+(define_insn "ashr<mode>3_430x"
+  [(set (match_operand:HPSI		   0 "msp430_general_dst_nonv_operand" "=r,r,r,r")
+	(ashiftrt:HPSI (match_operand:HPSI 1 "general_operand"	  	     "0,0,0,0")
+		       (match_operand:HPSI 2 "const_int_operand" 	     "M,P,K,i")))]
   "msp430x"
-  {
-    return msp430x_logical_shift_right (operands[2]);
-  }
-)
-
-(define_insn "srlp_1"
-  [(set (match_operand:PSI              0 "register_operand" "=r")
-	(lshiftrt:PSI (match_operand:PSI 1 "general_operand" "0")
-		      (const_int 1)))]
-  ""
-  "RRUM.A #1,%0"
-)
-
-(define_insn "srll_1"
-  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=rm")
-	(lshiftrt:SI (match_operand:SI 1 "general_operand"       "0")
-		     (const_int 1)))]
-  ""
-  "CLRC { RRC%X0.W\t%H0 { RRC%X0.W\t%L0"
+  "@
+  RRAM%b0\t%2, %0
+  RPT\t%2 { RRAX%b0\t%0
+  RPT\t#16 { RRAX%b0\t%0 { RPT\t%W2 { RRAX%b0\t%0
+  # undefined behavior arithmetic right shift of %1 by %2"
 )
 
-(define_insn "srll_2x"
-  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=r")
-	(lshiftrt:SI (match_operand:SI 1 "general_operand"       "0")
-		     (const_int 2)))]
+(define_insn "lshr<mode>3_430x"
+  [(set (match_operand:HPSI		   0 "msp430_general_dst_nonv_operand" "=r,r,r,r")
+	(lshiftrt:HPSI (match_operand:HPSI 1 "general_operand"	  	     "0,0,0,0")
+		       (match_operand:HPSI 2 "const_int_operand" 	     "M,P,K,i")))]
   "msp430x"
-  "RRUX.W\t%H0 { RRC.W\t%L0 { RRUX.W\t%H0 { RRC.W\t%L0"
-)
-
-(define_expand "lshrsi3"
-  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand")
-	(lshiftrt:SI (match_operand:SI 1 "general_operand")
-		     (match_operand:SI 2 "general_operand")))]
-  ""
-  "msp430_expand_helper (operands, \"__mspabi_srll\", !optimize_size);
-   DONE;"
-)
-
-(define_expand "lshrdi3"
-  [(set (match_operand:DI	     0 "msp430_general_dst_nonv_operand")
-	(ashift:DI (match_operand:DI 1 "general_operand")
-		   (match_operand:DI 2 "general_operand")))]
-  ""
-  {
-    /* No const_variant for 64-bit shifts.  */
-    msp430_expand_helper (operands, \"__mspabi_srlll\", false);
-    DONE;
-  }
+  "@
+  RRUM%b0\t%2, %0
+  RPT\t%2 { RRUX%b0\t%0
+  RPT\t#16 { RRUX%b0\t%0 { RPT\t%W2 { RRUX%b0\t%0
+  # undefined behavior logical right shift of %1 by %2"
 )
 
 ;;------------------------------------------------------------
@@ -1427,7 +1238,7 @@ (define_insn "*bitbranch<mode>4_z"
   [(set (pc) (if_then_else
 	      (ne (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rYs,rm")
 				    (const_int 1)
-				    (match_operand 1 "msp430_bitpos" "i,i"))
+				    (match_operand 1 "const_0_to_15_operand" "i,i"))
 		  (const_int 0))
               (label_ref (match_operand 2 "" ""))
 	      (pc)))
@@ -1443,7 +1254,7 @@ (define_insn "*bitbranch<mode>4_z"
   [(set (pc) (if_then_else
 	      (eq (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rm")
 				   (const_int 1)
-				   (match_operand 1 "msp430_bitpos" "i"))
+				   (match_operand 1 "const_0_to_15_operand" "i"))
 		  (const_int 0))
               (label_ref (match_operand 2 "" ""))
 	      (pc)))
@@ -1457,7 +1268,7 @@ (define_insn "*bitbranch<mode>4_z"
   [(set (pc) (if_then_else
 	      (eq (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rm")
 				   (const_int 1)
-				   (match_operand 1 "msp430_bitpos" "i"))
+				   (match_operand 1 "const_0_to_15_operand" "i"))
 		  (const_int 0))
               (pc)
 	      (label_ref (match_operand 2 "" ""))))
@@ -1471,7 +1282,7 @@ (define_insn "*bitbranch<mode>4_z"
   [(set (pc) (if_then_else
 	      (ne (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rm")
 				   (const_int 1)
-				   (match_operand 1 "msp430_bitpos" "i"))
+				   (match_operand 1 "const_0_to_15_operand" "i"))
 		  (const_int 0))
               (pc)
 	      (label_ref (match_operand 2 "" ""))))
diff --git a/gcc/config/msp430/msp430.opt b/gcc/config/msp430/msp430.opt
index b451174c3d1..8134ca7ac95 100644
--- a/gcc/config/msp430/msp430.opt
+++ b/gcc/config/msp430/msp430.opt
@@ -109,3 +109,9 @@ mdevices-csv-loc=
 Target Joined Var(msp430_devices_csv_loc) RejectNegative Report
 The path to devices.csv.  The GCC driver can normally locate devices.csv itself
 and pass this option to the compiler, so the user shouldn't need to pass this.
+
+mmax-inline-shift=
+Target RejectNegative Joined UInteger IntegerRange(0,65) Var(msp430_max_inline_shift) Init(65) Report
+For shift operations by a constant amount, which require an individual instruction to shift by one
+position, set the maximum number of inline shift instructions (maximum value 64) to emit instead of using the corresponding __mspabi helper function.
+The default value is 4.
diff --git a/gcc/config/msp430/predicates.md b/gcc/config/msp430/predicates.md
index 408d56f610a..4bfa0c0f2d5 100644
--- a/gcc/config/msp430/predicates.md
+++ b/gcc/config/msp430/predicates.md
@@ -113,12 +113,21 @@ (define_predicate "msp430_nonsubregnonpostinc_or_imm_operand"
        (ior (match_code "reg,mem")
 	    (match_operand 0 "immediate_operand"))))
 
-; TRUE for constants which are bit positions for zero_extract
-(define_predicate "msp430_bitpos"
+(define_predicate "const_1_to_8_operand"
+  (and (match_code "const_int")
+       (match_test ("   INTVAL (op) >= 1
+		     && INTVAL (op) <= 8 "))))
+
+(define_predicate "const_0_to_15_operand"
   (and (match_code "const_int")
        (match_test ("   INTVAL (op) >= 0
 		     && INTVAL (op) <= 15 "))))
 
+(define_predicate "const_1_to_19_operand"
+  (and (match_code "const_int")
+       (match_test ("   INTVAL (op) >= 1
+		     && INTVAL (op) <= 19 "))))
+
 (define_predicate "msp430_symbol_operand"
   (match_code "symbol_ref")
 )
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 09bcc5b0f78..885c7aae3a5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -1066,7 +1066,7 @@ Objective-C and Objective-C++ Dialects}.
 -mwarn-mcu @gol
 -mcode-region=  -mdata-region= @gol
 -msilicon-errata=  -msilicon-errata-warn= @gol
--mhwmult=  -minrt  -mtiny-printf}
+-mhwmult=  -minrt  -mtiny-printf  -mmax-inline-shift=}
 
 @emph{NDS32 Options}
 @gccoptlist{-mbig-endian  -mlittle-endian @gol
@@ -24728,6 +24728,19 @@ buffered before it is sent to write.
 This option requires Newlib Nano IO, so GCC must be configured with
 @samp{--enable-newlib-nano-formatted-io}.
 
+@item -mmax-inline-shift=
+@opindex mmax-inline-shift=
+This option takes an integer between 0 and 64 inclusive, and sets
+the maximum number of inline shift instructions which should be emitted to
+perform a shift operation by a constant amount.  When this value needs to be
+exceeded, an mspabi helper function is used instead.  The default value is 4.
+
+This only affects cases where a shift by multiple positions cannot be
+completed with a single instruction (e.g. all shifts >1 on the 430 ISA).
+
+Shifts of a 32-bit value are at least twice as costly, so the value passed for
+this option is divided by 2 and the resulting value used instead.
+
 @item -mcode-region=
 @itemx -mdata-region=
 @opindex mcode-region
diff --git a/gcc/testsuite/gcc.target/msp430/emulate-srli.c b/gcc/testsuite/gcc.target/msp430/emulate-srli.c
index f870d13f86b..35207b7c458 100644
--- a/gcc/testsuite/gcc.target/msp430/emulate-srli.c
+++ b/gcc/testsuite/gcc.target/msp430/emulate-srli.c
@@ -2,7 +2,7 @@
 /* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
 /* { dg-options "-Os" } */
 /* { dg-final { scan-assembler-not "mspabi_srli" } } */
-/* { dg-final { scan-assembler "rrum" } } */
+/* { dg-final { scan-assembler "RRUM" } } */
 
 /* Ensure that HImode shifts with source operand in memory are emulated with a
    rotate instructions.  */
diff --git a/gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
new file mode 100644
index 00000000000..c795f7570d7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
@@ -0,0 +1,52 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430x" "-mlarge" } { "" } } */
+/* { dg-options "-mcpu=msp430" } */
+/* { dg-final { scan-assembler-not "__mspabi_slli_4" } } */
+/* { dg-final { scan-assembler-not "__mspabi_sral_2" } } */
+/* { dg-final { scan-assembler "__mspabi_slli_5" } } */
+/* { dg-final { scan-assembler "__mspabi_sral_3" } } */
+
+/* Test the default value of 4 for -mmax-inline-shift has been observed.  */
+
+volatile int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15;
+volatile long l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15;
+
+void
+ashift (void)
+{
+  a1 <<= 1;
+  a2 <<= 2;
+  a3 <<= 3;
+  a4 <<= 4;
+  a5 <<= 5;
+  a6 <<= 6;
+  a7 <<= 7;
+  a8 <<= 8;
+  a9 <<= 9;
+  a10 <<= 10;
+  a11 <<= 11;
+  a12 <<= 12;
+  a13 <<= 13;
+  a14 <<= 14;
+  a15 <<= 15;
+}
+
+void
+ashiftrt (void)
+{
+  l1  >>= 1;
+  l2  >>= 2;
+  l3  >>= 3;
+  l4  >>= 4;
+  l5  >>= 5;
+  l6  >>= 6;
+  l7  >>= 7;
+  l8  >>= 8;
+  l9  >>= 9;
+  l10 >>= 10;
+  l11 >>= 11;
+  l12 >>= 12;
+  l13 >>= 13;
+  l14 >>= 14;
+  l15 >>= 15;
+}
diff --git a/gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
new file mode 100644
index 00000000000..7a519eec663
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
@@ -0,0 +1,50 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430x" "-mlarge" } { "" } } */
+/* { dg-options "-mcpu=msp430 -mmax-inline-shift=10" } */
+/* { dg-final { scan-assembler-not "__mspabi_slli_10" } } */
+/* { dg-final { scan-assembler-not "__mspabi_sral_5" } } */
+/* { dg-final { scan-assembler "__mspabi_slli_11" } } */
+/* { dg-final { scan-assembler "__mspabi_sral_6" } } */
+
+volatile int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15;
+volatile long l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15;
+
+void
+ashift (void)
+{
+  a1 <<= 1;
+  a2 <<= 2;
+  a3 <<= 3;
+  a4 <<= 4;
+  a5 <<= 5;
+  a6 <<= 6;
+  a7 <<= 7;
+  a8 <<= 8;
+  a9 <<= 9;
+  a10 <<= 10;
+  a11 <<= 11;
+  a12 <<= 12;
+  a13 <<= 13;
+  a14 <<= 14;
+  a15 <<= 15;
+}
+
+void
+ashiftrt (void)
+{
+  l1  >>= 1;
+  l2  >>= 2;
+  l3  >>= 3;
+  l4  >>= 4;
+  l5  >>= 5;
+  l6  >>= 6;
+  l7  >>= 7;
+  l8  >>= 8;
+  l9  >>= 9;
+  l10 >>= 10;
+  l11 >>= 11;
+  l12 >>= 12;
+  l13 >>= 13;
+  l14 >>= 14;
+  l15 >>= 15;
+}
diff --git a/gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c
new file mode 100644
index 00000000000..074b3af5539
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
+/* { dg-options "-mcpu=msp430x -mmax-inline-shift=10" } */
+/* { dg-final { scan-assembler-not "__mspabi_slli" } } */
+/* { dg-final { scan-assembler "__mspabi_sral_6" } } */
+
+volatile int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15;
+volatile long l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15;
+
+void
+ashift (void)
+{
+  a1 <<= 1;
+  a2 <<= 2;
+  a3 <<= 3;
+  a4 <<= 4;
+  a5 <<= 5;
+  a6 <<= 6;
+  a7 <<= 7;
+  a8 <<= 8;
+  a9 <<= 9;
+  a10 <<= 10;
+  a11 <<= 11;
+  a12 <<= 12;
+  a13 <<= 13;
+  a14 <<= 14;
+  a15 <<= 15;
+}
+
+void
+ashiftrt (void)
+{
+  l1  >>= 1;
+  l2  >>= 2;
+  l3  >>= 3;
+  l4  >>= 4;
+  l5  >>= 5;
+  l6  >>= 6;
+  l7  >>= 7;
+  l8  >>= 8;
+  l9  >>= 9;
+  l10 >>= 10;
+  l11 >>= 11;
+  l12 >>= 12;
+  l13 >>= 13;
+  l14 >>= 14;
+  l15 >>= 15;
+}
diff --git a/libgcc/config/msp430/slli.S b/libgcc/config/msp430/slli.S
index c31e2d5db9b..b22622e0bf5 100644
--- a/libgcc/config/msp430/slli.S
+++ b/libgcc/config/msp430/slli.S
@@ -65,6 +65,21 @@ __mspabi_slli:
 	RET
 #endif
 
+#ifdef __MSP430X__
+	.section	.text.__gnu_mspabi_sllp
+1:	ADDA	#-1,R13
+	ADDA	R12,R12
+	.global	__gnu_mspabi_sllp
+__gnu_mspabi_sllp:
+	CMP	#0,R13
+	JNZ	1b
+#ifdef __MSP430X_LARGE__
+	RETA
+#else
+	RET
+#endif /* __MSP430X_LARGE__ */
+#endif /* __MSP430X__ */
+
 /* Logical Left Shift - R12:R13 -> R12:R13.  */
 
 	.section	.text.__mspabi_slll_n
diff --git a/libgcc/config/msp430/srai.S b/libgcc/config/msp430/srai.S
index d4a47fa985a..0100a368365 100644
--- a/libgcc/config/msp430/srai.S
+++ b/libgcc/config/msp430/srai.S
@@ -64,6 +64,21 @@ __mspabi_srai:
 	RET
 #endif
 
+#ifdef __MSP430X__
+	.section	.text.__gnu_mspabi_srap
+1:	ADDA	#-1,R13
+	RRAX.A	R12,R12
+	.global	__gnu_mspabi_srap
+__gnu_mspabi_srap:
+	CMP	#0,R13
+	JNZ	1b
+#ifdef __MSP430X_LARGE__
+	RETA
+#else
+	RET
+#endif /* __MSP430X_LARGE__ */
+#endif /* __MSP430X__ */
+
 /* Arithmetic Right Shift - R12:R13 -> R12:R13.  */
 
 	.section	.text.__mspabi_sral_n
diff --git a/libgcc/config/msp430/srli.S b/libgcc/config/msp430/srli.S
index 838c4bc0617..50db47c9938 100644
--- a/libgcc/config/msp430/srli.S
+++ b/libgcc/config/msp430/srli.S
@@ -66,6 +66,22 @@ __mspabi_srli:
 	RET
 #endif
 
+#ifdef __MSP430X__
+	.section	.text.__gnu_mspabi_srlp
+1:	ADDA	#-1,R13
+	CLRC
+	RRCX.A	R12,R12
+	.global	__gnu_mspabi_srlp
+__gnu_mspabi_srlp:
+	CMP	#0,R13
+	JNZ	1b
+#ifdef __MSP430X_LARGE__
+	RETA
+#else
+	RET
+#endif /* __MSP430X_LARGE__ */
+#endif /* __MSP430X__ */
+
 /* Logical Right Shift - R12:R13 -> R12:R13.  */
 
 	.section	.text.__mspabi_srll_n
-- 
2.27.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant
  2020-07-21 18:24 ` [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant Jozef Lawrynowicz
@ 2020-07-22  8:33   ` Richard Sandiford
  2020-07-22  9:48     ` Jozef Lawrynowicz
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2020-07-22  8:33 UTC (permalink / raw)
  To: gcc-patches

Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> is_int_mode does not allow MODE_PARTIAL_INT modes, so convert_modes was
> not allowing a constant value to be converted to a MODE_PARTIAL_INT for
> use as operand 2 in patterns such as ashlpsi3. The constant had
> to be copied into a register before it could be used, but now can be
> used directly as an operand without any copying.

Yeah.  I guess this dates back to when MODE_PARTIAL_INTs didn't have
a known precision.

> diff --git a/gcc/expr.c b/gcc/expr.c
> index c7c3e9fd655..5a252f0935f 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -696,7 +696,7 @@ convert_modes (machine_mode mode, machine_mode oldmode, rtx x, int unsignedp)
>      return x;
>  
>    if (CONST_SCALAR_INT_P (x)
> -      && is_int_mode (mode, &int_mode))
> +      && is_a <scalar_int_mode> (mode, &int_mode))
>      {
>        /* If the caller did not tell us the old mode, then there is not
>  	 much to do with respect to canonicalization.  We have to

I think we also need to change the condition in:

      /* If the caller did not tell us the old mode, then there is not
	 much to do with respect to canonicalization.  We have to
	 assume that all the bits are significant.  */
      if (GET_MODE_CLASS (oldmode) != MODE_INT)

to is_a <scalar_int_mode> (old_mode)

OK with that, thanks.

Richard

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from emit_store_flag
  2020-07-21 18:26 ` [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from emit_store_flag Jozef Lawrynowicz
@ 2020-07-22  8:38   ` Richard Sandiford
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Sandiford @ 2020-07-22  8:38 UTC (permalink / raw)
  To: gcc-patches

Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> diff --git a/gcc/expmed.c b/gcc/expmed.c
> index e7c03fbf92c..d3a1735d39e 100644
> --- a/gcc/expmed.c
> +++ b/gcc/expmed.c
> @@ -4086,9 +4086,12 @@ expand_sdiv_pow2 (scalar_int_mode mode, rtx op0, HOST_WIDE_INT d)
>      {
>        temp = gen_reg_rtx (mode);
>        temp = emit_store_flag (temp, LT, op0, const0_rtx, mode, 0, 1);
> -      temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
> -			   0, OPTAB_LIB_WIDEN);
> -      return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
> +      if (temp != NULL_RTX)
> +	{
> +	  temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
> +			       0, OPTAB_LIB_WIDEN);
> +	  return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
> +	}
>      }
>  
>    if (HAVE_conditional_move
> @@ -4122,17 +4125,20 @@ expand_sdiv_pow2 (scalar_int_mode mode, rtx op0, HOST_WIDE_INT d)
>  
>        temp = gen_reg_rtx (mode);
>        temp = emit_store_flag (temp, LT, op0, const0_rtx, mode, 0, -1);
> -      if (GET_MODE_BITSIZE (mode) >= BITS_PER_WORD
> -	  || shift_cost (optimize_insn_for_speed_p (), mode, ushift)
> -	     > COSTS_N_INSNS (1))
> -	temp = expand_binop (mode, and_optab, temp, gen_int_mode (d - 1, mode),
> -			     NULL_RTX, 0, OPTAB_LIB_WIDEN);
> -      else
> -	temp = expand_shift (RSHIFT_EXPR, mode, temp,
> -			     ushift, NULL_RTX, 1);
> -      temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
> -			   0, OPTAB_LIB_WIDEN);
> -      return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
> +      if (temp != NULL_RTX)
> +	{
> +	  if (GET_MODE_BITSIZE (mode) >= BITS_PER_WORD
> +	      || shift_cost (optimize_insn_for_speed_p (), mode, ushift)
> +	      > COSTS_N_INSNS (1))
> +	    temp = expand_binop (mode, and_optab, temp, gen_int_mode (d - 1, mode),

Long line.

OK otherwise, thanks.  I guess these failed attempts will leave
a few unused temporary registers around (from the gen_reg_rtxes)
but it's going to be hard to avoid that in a clean way.

Richard

> +				 NULL_RTX, 0, OPTAB_LIB_WIDEN);
> +	  else
> +	    temp = expand_shift (RSHIFT_EXPR, mode, temp,
> +				 ushift, NULL_RTX, 1);
> +	  temp = expand_binop (mode, add_optab, temp, op0, NULL_RTX,
> +			       0, OPTAB_LIB_WIDEN);
> +	  return expand_shift (RSHIFT_EXPR, mode, temp, logd, NULL_RTX, 0);
> +	}
>      }
>  
>    label = gen_label_rtx ();

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant
  2020-07-22  8:33   ` Richard Sandiford
@ 2020-07-22  9:48     ` Jozef Lawrynowicz
  2020-07-24 14:14       ` Richard Sandiford
  0 siblings, 1 reply; 10+ messages in thread
From: Jozef Lawrynowicz @ 2020-07-22  9:48 UTC (permalink / raw)
  To: gcc-patches, richard.sandiford

On Wed, Jul 22, 2020 at 09:33:47AM +0100, Richard Sandiford wrote:
> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> > is_int_mode does not allow MODE_PARTIAL_INT modes, so convert_modes was
> > not allowing a constant value to be converted to a MODE_PARTIAL_INT for
> > use as operand 2 in patterns such as ashlpsi3. The constant had
> > to be copied into a register before it could be used, but now can be
> > used directly as an operand without any copying.
> 
> Yeah.  I guess this dates back to when MODE_PARTIAL_INTs didn't have
> a known precision.

Is that what the section on MODE_PARTIAL_INT in the description for the
"subreg" RTX refers to?  From "14.8 Registers and Memory" of gccint:

  A MODE_PARTIAL_INT mode behaves as if it were as wide as the corresponding
  MODE_INT mode, except that it has an unknown number of undefined bits.

If so, that whole section seems out of date. I can work on getting it
fixed up.

> 
> > diff --git a/gcc/expr.c b/gcc/expr.c
> > index c7c3e9fd655..5a252f0935f 100644
> > --- a/gcc/expr.c
> > +++ b/gcc/expr.c
> > @@ -696,7 +696,7 @@ convert_modes (machine_mode mode, machine_mode oldmode, rtx x, int unsignedp)
> >      return x;
> >  
> >    if (CONST_SCALAR_INT_P (x)
> > -      && is_int_mode (mode, &int_mode))
> > +      && is_a <scalar_int_mode> (mode, &int_mode))
> >      {
> >        /* If the caller did not tell us the old mode, then there is not
> >  	 much to do with respect to canonicalization.  We have to
> 
> I think we also need to change the condition in:
> 
>       /* If the caller did not tell us the old mode, then there is not
> 	 much to do with respect to canonicalization.  We have to
> 	 assume that all the bits are significant.  */
>       if (GET_MODE_CLASS (oldmode) != MODE_INT)
> 
> to is_a <scalar_int_mode> (old_mode)
> 
> OK with that, thanks.

Thanks, I'll regtest that change and apply if all looks good.

Jozef
> 
> Richard

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant
  2020-07-22  9:48     ` Jozef Lawrynowicz
@ 2020-07-24 14:14       ` Richard Sandiford
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Sandiford @ 2020-07-24 14:14 UTC (permalink / raw)
  To: gcc-patches

Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> On Wed, Jul 22, 2020 at 09:33:47AM +0100, Richard Sandiford wrote:
>> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
>> > is_int_mode does not allow MODE_PARTIAL_INT modes, so convert_modes was
>> > not allowing a constant value to be converted to a MODE_PARTIAL_INT for
>> > use as operand 2 in patterns such as ashlpsi3. The constant had
>> > to be copied into a register before it could be used, but now can be
>> > used directly as an operand without any copying.
>> 
>> Yeah.  I guess this dates back to when MODE_PARTIAL_INTs didn't have
>> a known precision.
>
> Is that what the section on MODE_PARTIAL_INT in the description for the
> "subreg" RTX refers to?  From "14.8 Registers and Memory" of gccint:
>
>   A MODE_PARTIAL_INT mode behaves as if it were as wide as the corresponding
>   MODE_INT mode, except that it has an unknown number of undefined bits.

Yeah.  Before d8487c949ad5 the number of significant bits in a partial
mode was hidden from target-independent code.

> If so, that whole section seems out of date. I can work on getting it
> fixed up.

Sounds good :-)

Thanks,
Richard

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ping [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns
  2020-07-21 18:29 ` [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns Jozef Lawrynowicz
@ 2020-08-07 10:55   ` Jozef Lawrynowicz
  2020-08-25 19:16   ` Jeff Law
  1 sibling, 0 replies; 10+ messages in thread
From: Jozef Lawrynowicz @ 2020-08-07 10:55 UTC (permalink / raw)
  To: gcc-patches

Pinging for this patch.

Thanks,
Jozef

On Tue, Jul 21, 2020 at 07:29:53PM +0100, Jozef Lawrynowicz wrote:
> The implementation of define_expand and define_insn patterns to handle
> shifts in the MSP430 backend is inconsistent, resulting in missed
> opportunities to make best use of the architecture's features.
> 
> There's now a single define_expand used as the entry point for all valid
> shifts, and the decision to either use a helper function to perform the
> shift (often required for the 430 ISA), or fall through to the
> define_insn patterns can be made from that expander function.
> 
> Shifts by a constant amount have been grouped into one define_insn for
> each type of shift, instead of having different define_insn patterns for
> shifts by different amounts.
> 
> A new target option "-mmax-inline-shift=" has been added to allow tuning
> of the number of shift instructions to emit inline, instead of using
> a library helper function.
> 
> Successfully regtested on trunk for msp430-elf, ok to apply?

> From a3c62c448c7f359bad85c86c35f712ca1fccf219 Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> Date: Wed, 15 Jul 2020 11:43:36 +0100
> Subject: [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns
> 
> The implementation of define_expand and define_insn patterns to handle
> shifts in the MSP430 backend is inconsistent, resulting in missed
> opportunities to make best use of the architecture's features.
> 
> There's now a single define_expand used as the entry point for all valid
> shifts, and the decision to either use a helper function to perform the
> shift (often required for the 430 ISA), or fall through to the
> define_insn patterns can be made from that expander function.
> 
> Shifts by a constant amount have been grouped into one define_insn for
> each type of shift, instead of having different define_insn patterns for
> shifts by different amounts.
> 
> A new target option "-mmax-inline-shift=" has been added to allow tuning
> of the number of shift instructions to emit inline, instead of using
> a library helper function.
> 
> gcc/ChangeLog:
> 
> 	* config/msp430/constraints.md (K): Change unused constraint to
> 	constraint to a const_int between 1 and 19.
> 	(P): New constraint.
> 	* config/msp430/msp430-protos.h (msp430x_logical_shift_right): Remove.
> 	(msp430_expand_shift): New.
> 	(msp430_output_asm_shift_insns): New.
> 	* config/msp430/msp430.c (msp430_rtx_costs): Remove shift costs.
> 	(CSH): Remove.
> 	(msp430_expand_helper): Remove hard-coded generation of some inline
> 	shift insns.
> 	(use_helper_for_const_shift): New.
> 	(msp430_expand_shift): New.
> 	(msp430_output_asm_shift_insns): New.
> 	(msp430_print_operand): Add new 'W' operand selector.
> 	(msp430x_logical_shift_right): Remove.
> 	* config/msp430/msp430.md (HPSI): New define_mode_iterator.
> 	(HDI): Likewise.
> 	(any_shift): New define_code_iterator.
> 	(shift_insn): New define_code_attr.
> 	Adjust unnamed insn patterns searched for by combine.
> 	(ashlhi3): Remove.
> 	(slli_1): Remove.
> 	(430x_shift_left): Remove.
> 	(slll_1): Remove.
> 	(slll_2): Remove.
> 	(ashlsi3): Remove.
> 	(ashldi3): Remove.
> 	(ashrhi3): Remove.
> 	(srai_1): Remove.
> 	(430x_arithmetic_shift_right): Remove.
> 	(srap_1): Remove.
> 	(srap_2): Remove.
> 	(sral_1): Remove.
> 	(sral_2): Remove.
> 	(ashrsi3): Remove.
> 	(ashrdi3): Remove.
> 	(lshrhi3): Remove.
> 	(srli_1): Remove.
> 	(430x_logical_shift_right): Remove.
> 	(srlp_1): Remove.
> 	(srll_1): Remove.
> 	(srll_2x): Remove.
> 	(lshrsi3): Remove.
> 	(lshrdi3): Remove.
> 	(<shift_insn><mode>3): New define_expand.
> 	(<shift_insn>hi3_430): New define_insn.
> 	(<shift_insn>si3_const): Likewise.
> 	(ashl<mode>3_430x): Likewise.
> 	(ashr<mode>3_430x): Likewise.
> 	(lshr<mode>3_430x): Likewise.
> 	(*bitbranch<mode>4_z): Replace renamed predicate msp430_bitpos with
> 	const_0_to_15_operand.
> 	* config/msp430/msp430.opt: New option -mmax-inline-shift=.
> 	* config/msp430/predicates.md (const_1_to_8_operand): New predicate.
> 	(const_0_to_15_operand): Rename msp430_bitpos predicate.
> 	(const_1_to_19_operand): New predicate.
> 	* doc/invoke.texi: Document -mmax-inline-shift=.
> 
> libgcc/ChangeLog:
> 
> 	* config/msp430/slli.S (__gnu_mspabi_sllp): New.
> 	* config/msp430/srai.S (__gnu_mspabi_srap): New.
> 	* config/msp430/srli.S (__gnu_mspabi_srlp): New.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/msp430/emulate-srli.c: Fix expected assembler text.
> 	* gcc.target/msp430/max-inline-shift-430-no-opt.c: New test.
> 	* gcc.target/msp430/max-inline-shift-430.c: New test.
> 	* gcc.target/msp430/max-inline-shift-430x.c: New test.
> 
> ---
>  gcc/config/msp430/constraints.md              |  10 +-
>  gcc/config/msp430/msp430-protos.h             |   6 +-
>  gcc/config/msp430/msp430.c                    | 272 +++++++++----
>  gcc/config/msp430/msp430.md                   | 381 +++++-------------
>  gcc/config/msp430/msp430.opt                  |   6 +
>  gcc/config/msp430/predicates.md               |  13 +-
>  gcc/doc/invoke.texi                           |  15 +-
>  .../gcc.target/msp430/emulate-srli.c          |   2 +-
>  .../msp430/max-inline-shift-430-no-opt.c      |  52 +++
>  .../gcc.target/msp430/max-inline-shift-430.c  |  50 +++
>  .../gcc.target/msp430/max-inline-shift-430x.c |  48 +++
>  libgcc/config/msp430/slli.S                   |  15 +
>  libgcc/config/msp430/srai.S                   |  15 +
>  libgcc/config/msp430/srli.S                   |  16 +
>  14 files changed, 527 insertions(+), 374 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
>  create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
>  create mode 100644 gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c
> 
> diff --git a/gcc/config/msp430/constraints.md b/gcc/config/msp430/constraints.md
> index 14368f7be6d..b8f9674b2fe 100644
> --- a/gcc/config/msp430/constraints.md
> +++ b/gcc/config/msp430/constraints.md
> @@ -25,15 +25,16 @@ (define_register_constraint "R13" "R13_REGS"
>    "Register R13.")
>  
>  (define_constraint "K"
> -  "Integer constant 1."
> +  "Integer constant 1-19."
>    (and (match_code "const_int")
> -       (match_test "IN_RANGE (ival, 1, 1)")))
> +       (match_test "IN_RANGE (ival, 1, 19)")))
>  
>  (define_constraint "L"
>    "Integer constant -1^20..1^19."
>    (and (match_code "const_int")
>         (match_test "IN_RANGE (ival, HOST_WIDE_INT_M1U << 20, 1 << 19)")))
>  
> +;; Valid shift amount for RRUM, RRAM, RLAM, RRCM.
>  (define_constraint "M"
>    "Integer constant 1-4."
>    (and (match_code "const_int")
> @@ -49,6 +50,11 @@ (define_constraint "O"
>    (and (match_code "const_int")
>         (match_test "IN_RANGE (ival, 256, 65535)")))
>  
> +(define_constraint "P"
> +  "Integer constant 1-16."
> +  (and (match_code "const_int")
> +       (match_test "IN_RANGE (ival, 1, 16)")))
> +
>  ;; We do not allow arbitrary constants, eg symbols or labels,
>  ;; because their address may be above the 16-bit address limit
>  ;; supported by the offset used in the MOVA instruction.
> diff --git a/gcc/config/msp430/msp430-protos.h b/gcc/config/msp430/msp430-protos.h
> index a13a94cb92c..0b4d9a42b41 100644
> --- a/gcc/config/msp430/msp430-protos.h
> +++ b/gcc/config/msp430/msp430-protos.h
> @@ -35,7 +35,6 @@ rtx	msp430_incoming_return_addr_rtx (void);
>  void	msp430_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree, int);
>  int	msp430_initial_elimination_offset (int, int);
>  bool    msp430_is_interrupt_func (void);
> -const char * msp430x_logical_shift_right (rtx);
>  const char * msp430_mcu_name (void);
>  void    msp430_output_aligned_decl_common (FILE *, const tree, const char *,
>  					   unsigned HOST_WIDE_INT, unsigned,
> @@ -51,4 +50,9 @@ bool    msp430_use_f5_series_hwmult (void);
>  bool	msp430_has_hwmult (void);
>  bool msp430_op_not_in_high_mem (rtx op);
>  
> +#ifdef RTX_CODE
> +int msp430_expand_shift (enum rtx_code code, machine_mode mode, rtx *operands);
> +const char * msp430_output_asm_shift_insns (enum rtx_code code, machine_mode mode, rtx *operands);
> +#endif
> +
>  #endif /* GCC_MSP430_PROTOS_H */
> diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
> index 455b4af3dad..c2b24974364 100644
> --- a/gcc/config/msp430/msp430.c
> +++ b/gcc/config/msp430/msp430.c
> @@ -1064,15 +1064,6 @@ static bool msp430_rtx_costs (rtx	   x ATTRIBUTE_UNUSED,
>  	  return true;
>  	}
>        break;
> -    case ASHIFT:
> -    case ASHIFTRT:
> -    case LSHIFTRT:
> -      if (!msp430x)
> -	{
> -	  *total = COSTS_N_INSNS (100);
> -	  return true;
> -	}
> -      break;
>      }
>    return false;
>  }
> @@ -2674,32 +2665,6 @@ msp430_init_dwarf_reg_sizes_extra (tree address)
>      }
>  }
>  
> -/* This is a list of MD patterns that implement fixed-count shifts.  */
> -static struct
> -{
> -  const char *name;
> -  int count;
> -  int need_430x;
> -  rtx (*genfunc)(rtx,rtx);
> -}
> -const_shift_helpers[] =
> -{
> -#define CSH(N,C,X,G) { "__mspabi_" N, C, X, gen_##G }
> -
> -  CSH ("slli", 1, 1, slli_1),
> -  CSH ("slll", 1, 1, slll_1),
> -  CSH ("slll", 2, 1, slll_2),
> -
> -  CSH ("srai", 1, 0, srai_1),
> -  CSH ("sral", 1, 0, sral_1),
> -  CSH ("sral", 2, 0, sral_2),
> -
> -  CSH ("srll", 1, 0, srll_1),
> -  CSH ("srll", 2, 1, srll_2x),
> -  { 0, 0, 0, 0 }
> -#undef CSH
> -};
> -
>  /* The MSP430 ABI defines a number of helper functions that should be
>     used for, for example, 32-bit shifts.  This function is called to
>     emit such a function, using the table above to optimize some
> @@ -2716,31 +2681,12 @@ msp430_expand_helper (rtx *operands, const char *helper_name,
>    machine_mode arg0mode = GET_MODE (operands[0]);
>    machine_mode arg1mode = GET_MODE (operands[1]);
>    machine_mode arg2mode = GET_MODE (operands[2]);
> -  int have_430x = msp430x ? 1 : 0;
>    int expand_mpy = strncmp (helper_name, "__mspabi_mpy",
>  			    sizeof ("__mspabi_mpy") - 1) == 0;
>    /* This function has been used incorrectly if CONST_VARIANTS is TRUE for a
>       hwmpy function.  */
>    gcc_assert (!(expand_mpy && const_variants));
>  
> -  /* Emit size-optimal insns for small shifts we can easily do inline.  */
> -  if (CONST_INT_P (operands[2]) && !expand_mpy)
> -    {
> -      int i;
> -
> -      for (i=0; const_shift_helpers[i].name; i++)
> -	{
> -	  if (const_shift_helpers[i].need_430x <= have_430x
> -	      && strcmp (helper_name, const_shift_helpers[i].name) == 0
> -	      && INTVAL (operands[2]) == const_shift_helpers[i].count)
> -	    {
> -	      emit_insn (const_shift_helpers[i].genfunc (operands[0],
> -							 operands[1]));
> -	      return;
> -	    }
> -	}
> -    }
> -
>    if (arg1mode != VOIDmode && arg2mode != VOIDmode)
>      /* Modes of arguments must be equal if not constants.  */
>      gcc_assert (arg1mode == arg2mode);
> @@ -2835,6 +2781,190 @@ msp430_expand_helper (rtx *operands, const char *helper_name,
>  		  gen_rtx_REG (arg0mode, 12));
>  }
>  
> +/* Return TRUE if the helper function should be used and FALSE if the shifts
> +   insns should be emitted inline.  */
> +static bool
> +use_helper_for_const_shift (enum rtx_code code, machine_mode mode,
> +			    HOST_WIDE_INT amt)
> +{
> +  const int default_inline_shift = 4;
> +  /* We initialize the option to 65 so we know if the user set it or not.  */
> +  int user_set_max_inline = (msp430_max_inline_shift == 65 ? 0 : 1);
> +  int max_inline = (user_set_max_inline ? msp430_max_inline_shift
> +		    : default_inline_shift);
> +  /* 32-bit shifts are roughly twice as costly as 16-bit shifts so we adjust
> +     the heuristic accordingly.  */
> +  int max_inline_32 = max_inline / 2;
> +
> +  /* Don't use helpers for these modes on 430X, when optimizing for speed, or
> +     when emitting a small number of insns.  */
> +  if ((mode == E_QImode || mode == E_HImode || mode == E_PSImode)
> +      && (msp430x
> +	  /* If the user set max_inline then we always obey that number.
> +	     Otherwise we always emit the shifts inline at -O2 and above.  */
> +	  || amt <= max_inline
> +	  || (!user_set_max_inline
> +	      && (optimize >= 2 && !optimize_size))))
> +    return false;
> +
> +  /* 430 and 430X codegen for SImode shifts is the same.
> +     Set a hard limit of 15 for the number of shifts that will be emitted
> +     inline by default, even at -O2 and above, to prevent code size
> +     explosion.  */
> +  if (mode == E_SImode
> +      && (amt <= max_inline_32
> +	  || (!user_set_max_inline
> +	      && (optimize >= 2 && !optimize_size)
> +	      && amt <= 15)))
> +    return false;
> +
> +  return true;
> +}
> +
> +/* For shift operations which will use an mspabi helper function, setup the
> +   call to msp430_expand helper.  Return 1 to indicate we have finished with
> +   this insn and invoke "DONE".
> +   Otherwise return 0 to indicate the insn should fallthrough.
> +   Never FAIL.  */
> +int
> +msp430_expand_shift (enum rtx_code code, machine_mode mode, rtx *operands)
> +{
> +  /* Always use the helper function when the shift amount is not a
> +     constant.  */
> +  if (!CONST_INT_P (operands[2])
> +      || mode == E_DImode
> +      || use_helper_for_const_shift (code, mode, INTVAL (operands[2])))
> +    {
> +      const char *helper_name = NULL;
> +      /* The const variants of mspabi shifts have significantly larger code
> +	 size than the generic version, so use the generic version if
> +	 optimizing for size.  */
> +      bool const_variant = !optimize_size;
> +      switch (mode)
> +	{
> +	case E_HImode:
> +	  helper_name = (code == ASHIFT ? "__mspabi_slli" :
> +			 (code == ASHIFTRT ? "__mspabi_srai" :
> +			  (code == LSHIFTRT ? "__mspabi_srli" :
> +			   NULL)));
> +	  break;
> +	case E_PSImode:
> +	  helper_name = (code == ASHIFT ? "__gnu_mspabi_sllp" :
> +			 (code == ASHIFTRT ? "__gnu_mspabi_srap" :
> +			  (code == LSHIFTRT ? "__gnu_mspabi_srlp" :
> +			   NULL)));
> +	  /* No const variant for PSImode shifts FIXME.  */
> +	  const_variant = false;
> +	  break;
> +	case E_SImode:
> +	  helper_name = (code == ASHIFT ? "__mspabi_slll" :
> +			 (code == ASHIFTRT ? "__mspabi_sral" :
> +			  (code == LSHIFTRT ? "__mspabi_srll" :
> +			   NULL)));
> +	  break;
> +	case E_DImode:
> +	  helper_name = (code == ASHIFT ? "__mspabi_sllll" :
> +			 (code == ASHIFTRT ? "__mspabi_srall" :
> +			  (code == LSHIFTRT ? "__mspabi_srlll" :
> +			   NULL)));
> +	  /* No const variant for DImode shifts.  */
> +	  const_variant = false;
> +	  break;
> +	default:
> +	  gcc_unreachable ();
> +	  break;
> +	}
> +      gcc_assert (helper_name);
> +      msp430_expand_helper (operands, helper_name, const_variant);
> +      return 1;
> +    }
> +  /* When returning 0, there must be an insn to match the RTL pattern
> +     otherwise there will be an unrecognizeable insn.  */
> +  return 0;
> +}
> +
> +/* Helper function to emit a sequence of shift instructions.  The amount of
> +   shift instructions to emit is in OPERANDS[2].
> +   For 430 we output copies of identical inline shifts for all modes.
> +   For 430X it is inneficient to do so for any modes except SI and DI, since we
> +   can make use of R*M insns or RPT with 430X insns, so this function is only
> +   used for SImode in that case.  */
> +const char *
> +msp430_output_asm_shift_insns (enum rtx_code code, machine_mode mode,
> +			       rtx *operands)
> +{
> +  int i;
> +  int amt;
> +  int max_shift = GET_MODE_BITSIZE (mode) - 1;
> +  gcc_assert (CONST_INT_P (operands[2]));
> +  amt = INTVAL (operands[2]);
> +
> +  if (amt == 0 || amt > max_shift)
> +    {
> +      switch (code)
> +	{
> +	case ASHIFT:
> +	  output_asm_insn ("# ignored undefined behaviour left shift "
> +			   "of %1 by %2", operands);
> +	  break;
> +	case ASHIFTRT:
> +	  output_asm_insn ("# ignored undefined behaviour arithmetic right "
> +			   "shift of %1 by %2", operands);
> +	  break;
> +	case LSHIFTRT:
> +	  output_asm_insn ("# ignored undefined behaviour logical right shift "
> +			   "of %1 by %2", operands);
> +	  break;
> +	default:
> +	  gcc_unreachable ();
> +	}
> +      return "";
> +    }
> +
> +  if (code == ASHIFT)
> +    {
> +      if (!msp430x && mode == HImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("RLA.W\t%0", operands);
> +      else if (mode == SImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("RLA%X0.W\t%L0 { RLC%X0.W\t%H0", operands);
> +      else
> +	/* Catch unhandled cases.  */
> +	gcc_unreachable ();
> +    }
> +  else if (code == ASHIFTRT)
> +    {
> +      if (!msp430x && mode == HImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("RRA.W\t%0", operands);
> +      else if (mode == SImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("RRA%X0.W\t%H0 { RRC%X0.W\t%L0", operands);
> +      else
> +	gcc_unreachable ();
> +    }
> +  else if (code == LSHIFTRT)
> +    {
> +      if (!msp430x && mode == HImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("CLRC { RRC.W\t%0", operands);
> +      else if (mode == SImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("CLRC { RRC%X0.W\t%H0 { RRC%X0.W\t%L0", operands);
> +      /* FIXME: Why doesn't "RRUX.W\t%H0 { RRC%X0.W\t%L0" work for msp430x?
> +	 It causes execution timeouts e.g. pr41963.c.  */
> +#if 0
> +      else if (msp430x && mode == SImode)
> +	for (i = 0; i < amt; i++)
> +	  output_asm_insn ("RRUX.W\t%H0 { RRC%X0.W\t%L0", operands);
> +#endif
> +      else
> +	gcc_unreachable ();
> +    }
> +  return "";
> +}
> +
>  /* Called by cbranch<mode>4 to coerce operands into usable forms.  */
>  void
>  msp430_fixup_compare_operands (machine_mode my_mode, rtx * operands)
> @@ -3368,6 +3498,7 @@ msp430_op_not_in_high_mem (rtx op)
>     O   offset of the top of the stack
>     Q   like X but generates an A postfix
>     R   inverse of condition code, unsigned.
> +   W   value - 16
>     X   X instruction postfix in large mode
>     Y   value - 4
>     Z   value - 1
> @@ -3394,6 +3525,11 @@ msp430_print_operand (FILE * file, rtx op, int letter)
>        /* Print the constant value, less four.  */
>        fprintf (file, "#%ld", INTVAL (op) - 4);
>        return;
> +    case 'W':
> +      gcc_assert (CONST_INT_P (op));
> +      /* Print the constant value, less 16.  */
> +      fprintf (file, "#%ld", INTVAL (op) - 16);
> +      return;
>      case 'I':
>        if (GET_CODE (op) == CONST_INT)
>  	{
> @@ -3711,34 +3847,6 @@ msp430x_extendhisi (rtx * operands)
>    return "MOV.W\t%1, %L0 { MOV.W\t%1, %H0 { RPT\t#15 { RRAX.W\t%H0";
>  }
>  
> -/* Likewise for logical right shifts.  */
> -const char *
> -msp430x_logical_shift_right (rtx amount)
> -{
> -  /* The MSP430X's logical right shift instruction - RRUM - does
> -     not use an extension word, so we cannot encode a repeat count.
> -     Try various alternatives to work around this.  If the count
> -     is in a register we are stuck, hence the assert.  */
> -  gcc_assert (CONST_INT_P (amount));
> -
> -  if (INTVAL (amount) <= 0
> -      || INTVAL (amount) >= 16)
> -    return "# nop logical shift.";
> -
> -  if (INTVAL (amount) > 0
> -      && INTVAL (amount) < 5)
> -    return "rrum.w\t%2, %0"; /* Two bytes.  */
> -
> -  if (INTVAL (amount) > 4
> -      && INTVAL (amount) < 9)
> -    return "rrum.w\t#4, %0 { rrum.w\t%Y2, %0 "; /* Four bytes.  */
> -
> -  /* First we logically shift right by one.  Now we know
> -     that the top bit is zero and we can use the arithmetic
> -     right shift instruction to perform the rest of the shift.  */
> -  return "rrum.w\t#1, %0 { rpt\t%Z2 { rrax.w\t%0"; /* Six bytes.  */
> -}
> -
>  /* Stop GCC from thinking that it can eliminate (SUBREG:PSI (SI)).  */
>  
>  #undef TARGET_CAN_CHANGE_MODE_CLASS
> diff --git a/gcc/config/msp430/msp430.md b/gcc/config/msp430/msp430.md
> index ed21eb02868..f70e61b97dd 100644
> --- a/gcc/config/msp430/msp430.md
> +++ b/gcc/config/msp430/msp430.md
> @@ -65,6 +65,15 @@ (define_attr "length" "" (const_int 4))
>  (include "constraints.md")
>  
>  (define_mode_iterator QHI [QI HI PSI])
> +(define_mode_iterator HPSI [HI PSI])
> +(define_mode_iterator HDI [HI PSI SI DI])
> +
> +;; Mapping of all shift operators
> +(define_code_iterator any_shift [ashift ashiftrt lshiftrt])
> +
> +;; Base name for define_insn
> +(define_code_attr shift_insn
> +  [(ashift "ashl") (lshiftrt "lshr") (ashiftrt "ashr")])
>  
>  ;; There are two basic "family" tests we do here:
>  ;;
> @@ -689,31 +698,42 @@ (define_insn ""
>     MOV%X1.B\t%1, %0"
>  )
>  
> +;; The next three insns emit identical assembly code.
> +;; They take a QImode and shift it in SImode.  Only shift counts <= 8
> +;; are handled since that is the simple case where the high 16-bits (i.e. the
> +;; high register) are always 0.
>  (define_insn ""
> -  [(set (match_operand:SI 0 "register_operand" "=r")
> -	(ashift:SI (zero_extend:SI (match_operand:QI 1 "general_operand" "rm"))
> -		   (match_operand:HI 2 "immediate_operand" "M")))]
> +  [(set (match_operand:SI			     0 "register_operand" "=r,r,r")
> +	(ashift:SI (zero_extend:SI (match_operand:QI 1 "general_operand" "0,rm,rm"))
> +		   (match_operand:HI		     2 "const_1_to_8_operand" "M,M,i")))]
>    "msp430x"
> -  "MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0"
> +  "@
> +  RLAM.W %2, %L0 { CLR %H0
> +  MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0
> +  MOV%X1.B %1, %L0 { RPT %2 { RLAX.W %L0 { CLR %H0"
>  )
>  
> -;; We are taking a char and shifting it and putting the result in 2 registers.
> -;; the high register will always be for 0 shift counts < 8.
>  (define_insn ""
> -  [(set (match_operand:SI 0 "register_operand" "=r")
> -	(ashift:SI (zero_extend:SI (subreg:HI (match_operand:QI 1 "general_operand" "rm") 0))
> -		   (match_operand:HI 2 "immediate_operand" "M")))]
> +  [(set (match_operand:SI			     		0 "register_operand" "=r,r,r")
> +	(ashift:SI (zero_extend:SI (subreg:HI (match_operand:QI 1 "general_operand" "0,rm,rm") 0))
> +		   (match_operand:HI		     		2 "const_1_to_8_operand" "M,M,i")))]
>    "msp430x"
> -  "MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0"
> +  "@
> +  RLAM.W %2, %L0 { CLR %H0
> +  MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0
> +  MOV%X1.B %1, %L0 { RPT %2 { RLAX.W %L0 { CLR %H0"
>  )
>  
>  ;; Same as above but with a NOP sign_extend round the subreg
>  (define_insn ""
> -  [(set (match_operand:SI 0 "register_operand" "=r")
> -	(ashift:SI (zero_extend:SI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "rm") 0)))
> -		   (match_operand:HI 2 "immediate_operand" "M")))]
> +  [(set (match_operand:SI			     				 0 "register_operand" "=r,r,r")
> +	(ashift:SI (zero_extend:SI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "0,rm,rm") 0)))
> +		   (match_operand:HI		     				 2 "const_1_to_8_operand" "M,M,i")))]
>    "msp430x"
> -  "MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0"
> +  "@
> +  RLAM.W %2, %L0 { CLR %H0
> +  MOV%X1.B %1, %L0 { RLAM.W %2, %L0 { CLR %H0
> +  MOV%X1.B %1, %L0 { RPT %2 { RLAX.W %L0 { CLR %H0"
>  )
>  
>  (define_insn ""
> @@ -724,11 +744,14 @@ (define_insn ""
>  )
>  
>  (define_insn ""
> -  [(set (match_operand:PSI 0 "register_operand" "=r")
> -	(ashift:PSI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "rm") 0))
> -		    (match_operand:HI 2 "immediate_operand" "M")))]
> +  [(set (match_operand:PSI					  0 "register_operand" "=r,r,r")
> +	(ashift:PSI (sign_extend:PSI (subreg:HI (match_operand:QI 1 "general_operand" "0,rm,rm") 0))
> +		    (match_operand:HI				  2 "const_1_to_19_operand" "M,M,i")))]
>    "msp430x"
> -  "MOV%X1.B %1, %0 { RLAM.W %2, %0"
> +  "@
> +  RLAM.W %2, %0
> +  MOV%X1.B %1, %0 { RLAM.W %2, %0
> +  MOV%X1.B %1, %0 { RPT %2 { RLAX.A %0"
>  )
>  ;; END msp430 pointer manipulation combine insn patterns
>  
> @@ -840,287 +863,75 @@ (define_insn "truncsipsi2"
>  ;; Note - we ignore shift counts of less than one or more than 15.
>  ;; This is permitted by the ISO C99 standard as such shifts result
>  ;; in "undefined" behavior.  [6.5.7 (3)]
> +;;
> +;; We avoid emitting insns in msp430_expand_shift, since we would have to handle
> +;; many extra cases such as op0 != op1, or, op0 or op1 in memory.  Instead we
> +;; let reload coerce op0 and op1 into the same register.
>  
> -;; signed A << C
> -
> -(define_expand "ashlhi3"
> -  [(set (match_operand:HI	     0 "msp430_general_dst_nonv_operand")
> -	(ashift:HI (match_operand:HI 1 "general_operand")
> -		   (match_operand:HI 2 "general_operand")))]
> +(define_expand "<shift_insn><mode>3"
> +  [(set (match_operand:HDI		  0 "msp430_general_dst_nonv_operand")
> +	(any_shift:HDI (match_operand:HDI 1 "general_operand")
> +		       (match_operand:HDI 2 "general_operand")))]
>    ""
>    {
> -    if ((GET_CODE (operands[1]) == SUBREG
> -	 && REG_P (XEXP (operands[1], 0)))
> -	|| MEM_P (operands[1]))
> -      operands[1] = force_reg (HImode, operands[1]);
> -    if (msp430x
> -        && REG_P (operands[0])
> -        && REG_P (operands[1])
> -        && CONST_INT_P (operands[2]))
> -      emit_insn (gen_430x_shift_left (operands[0], operands[1], operands[2]));
> -    else if (CONST_INT_P (operands[2])
> -	     && INTVAL (operands[2]) == 1)
> -      emit_insn (gen_slli_1 (operands[0], operands[1]));
> -    else		 
> -      /* The const variants of mspabi shifts have larger code size than the
> -	 generic version, so use the generic version if optimizing for
> -	 size.  */
> -      msp430_expand_helper (operands, \"__mspabi_slli\", !optimize_size);
> -    DONE;
> +    if (msp430_expand_shift (<CODE>, <MODE>mode, operands))
> +      DONE;
> +    /* Otherwise, fallthrough.  */
>    }
>  )
>  
> -(define_insn "slli_1"
> -  [(set (match_operand:HI	     0 "msp430_general_dst_nonv_operand" "=rm")
> -	(ashift:HI (match_operand:HI 1 "general_operand"       "0")
> -		   (const_int 1)))]
> -  ""
> -  "RLA%X0.W\t%0" ;; Note - this is a macro for ADD
> -)
> -
> -(define_insn "430x_shift_left"
> -  [(set (match_operand:HI            0 "register_operand" "=r")
> -	(ashift:HI (match_operand:HI 1 "register_operand"  "0")
> -		   (match_operand    2 "immediate_operand" "n")))]
> -  "msp430x"
> -  "*
> -  if (INTVAL (operands[2]) > 0 && INTVAL (operands[2]) < 5)
> -    return \"RLAM.W\t%2, %0\";
> -  else if (INTVAL (operands[2]) >= 5 && INTVAL (operands[2]) < 16)
> -    return \"RPT\t%2 { RLAX.W\t%0\";
> -  return \"# nop left shift\";
> -  "
> +;; All 430 HImode constant shifts
> +(define_insn "<shift_insn>hi3_430"
> +  [(set (match_operand:HI		0 "msp430_general_dst_nonv_operand" "=rm")
> +	(any_shift:HI (match_operand:HI 1 "general_operand"       "0")
> +		      (match_operand:HI 2 "const_int_operand"     "n")))]
> +  "!msp430x"
> +  "* return msp430_output_asm_shift_insns (<CODE>, HImode, operands);"
>  )
>  
> -(define_insn "slll_1"
> -  [(set (match_operand:SI	     0 "msp430_general_dst_nonv_operand" "=rm")
> -	(ashift:SI (match_operand:SI 1 "general_operand"       "0")
> -		   (const_int 1)))]
> +;; All 430 and 430X SImode constant shifts
> +(define_insn "<shift_insn>si3_const"
> +  [(set (match_operand:SI		0 "msp430_general_dst_nonv_operand" "=rm")
> +	(any_shift:SI (match_operand:SI 1 "general_operand"       "0")
> +		      (match_operand:SI 2 "const_int_operand"     "n")))]
>    ""
> -  "RLA%X0.W\t%L0 { RLC%X0.W\t%H0"
> -)
> -
> -(define_insn "slll_2"
> -  [(set (match_operand:SI	     0 "msp430_general_dst_nonv_operand" "=rm")
> -	(ashift:SI (match_operand:SI 1 "general_operand"       "0")
> -		   (const_int 2)))]
> -  ""
> -  "RLA%X0.W\t%L0 { RLC%X0.W\t%H0 { RLA%X0.W\t%L0 { RLC%X0.W\t%H0"
> -)
> -
> -(define_expand "ashlsi3"
> -  [(set (match_operand:SI	     0 "msp430_general_dst_nonv_operand")
> -	(ashift:SI (match_operand:SI 1 "general_operand")
> -		   (match_operand:SI 2 "general_operand")))]
> -  ""
> -  "msp430_expand_helper (operands, \"__mspabi_slll\", !optimize_size);
> -   DONE;"
> -)
> -
> -(define_expand "ashldi3"
> -  [(set (match_operand:DI	     0 "msp430_general_dst_nonv_operand")
> -	(ashift:DI (match_operand:DI 1 "general_operand")
> -		   (match_operand:DI 2 "general_operand")))]
> -  ""
> -  {
> -    /* No const_variant for 64-bit shifts.  */
> -    msp430_expand_helper (operands, \"__mspabi_sllll\", false);
> -    DONE;
> -  }
> -)
> -
> -;;----------
> -
> -;; signed A >> C
> -
> -(define_expand "ashrhi3"
> -  [(set (match_operand:HI	       0 "msp430_general_dst_nonv_operand")
> -	(ashiftrt:HI (match_operand:HI 1 "general_operand")
> -		     (match_operand:HI 2 "general_operand")))]
> -  ""
> -  {
> -    if ((GET_CODE (operands[1]) == SUBREG
> -	 && REG_P (XEXP (operands[1], 0)))
> -	|| MEM_P (operands[1]))
> -      operands[1] = force_reg (HImode, operands[1]);
> -    if (msp430x
> -        && REG_P (operands[0])
> -        && REG_P (operands[1])
> -        && CONST_INT_P (operands[2]))
> -      emit_insn (gen_430x_arithmetic_shift_right (operands[0], operands[1], operands[2]));
> -    else if (CONST_INT_P (operands[2])
> -	     && INTVAL (operands[2]) == 1)
> -      emit_insn (gen_srai_1 (operands[0], operands[1]));
> -    else		 
> -       msp430_expand_helper (operands, \"__mspabi_srai\", !optimize_size);
> -   DONE;
> -   }
> -)
> -
> -(define_insn "srai_1"
> -  [(set (match_operand:HI	       0 "msp430_general_dst_operand" "=rm")
> -	(ashiftrt:HI (match_operand:HI 1 "msp430_general_operand"      "0")
> -		     (const_int 1)))]
> -  ""
> -  "RRA%X0.W\t%0"
> -)
> -
> -(define_insn "430x_arithmetic_shift_right"
> -  [(set (match_operand:HI              0 "register_operand" "=r")
> -	(ashiftrt:HI (match_operand:HI 1 "register_operand"  "0")
> -		     (match_operand    2 "immediate_operand" "n")))]
> -  "msp430x"
> -  "*
> -  if (INTVAL (operands[2]) > 0 && INTVAL (operands[2]) < 5)
> -    return \"RRAM.W\t%2, %0\";
> -  else if (INTVAL (operands[2]) >= 5 && INTVAL (operands[2]) < 16)
> -    return \"RPT\t%2 { RRAX.W\t%0\";
> -  return \"# nop arith right shift\";
> -  "
> -)
> -
> -(define_insn "srap_1"
> -  [(set (match_operand:PSI              0 "register_operand" "=r")
> -	(ashiftrt:PSI (match_operand:PSI 1 "general_operand" "0")
> -		      (const_int 1)))]
> -  "msp430x"
> -  "RRAM.A #1,%0"
> +  "* return msp430_output_asm_shift_insns (<CODE>, SImode, operands);"
>  )
>  
> -(define_insn "srap_2"
> -  [(set (match_operand:PSI              0 "register_operand" "=r")
> -	(ashiftrt:PSI (match_operand:PSI 1 "general_operand" "0")
> -		      (const_int 2)))]
> +(define_insn "ashl<mode>3_430x"
> +  [(set (match_operand:HPSI		 0 "msp430_general_dst_nonv_operand" "=r,r,r,r")
> +	(ashift:HPSI (match_operand:HPSI 1 "general_operand" 		     "0 ,0,0,0")
> +		     (match_operand:HPSI 2 "const_int_operand" 		     "M ,P,K,i")))]
>    "msp430x"
> -  "RRAM.A #2,%0"
> -)
> -
> -(define_insn "sral_1"
> -  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=rm")
> -	(ashiftrt:SI (match_operand:SI 1 "general_operand"       "0")
> -		     (const_int 1)))]
> -  ""
> -  "RRA%X0.W\t%H0 { RRC%X0.W\t%L0"
> -)
> -
> -(define_insn "sral_2"
> -  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=rm")
> -	(ashiftrt:SI (match_operand:SI 1 "general_operand"       "0")
> -		     (const_int 2)))]
> -  ""
> -  "RRA%X0.W\t%H0 { RRC%X0.W\t%L0 { RRA%X0.W\t%H0 { RRC%X0.W\t%L0"
> -)
> -
> -(define_expand "ashrsi3"
> -  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand")
> -	(ashiftrt:SI (match_operand:SI 1 "general_operand")
> -		     (match_operand:SI 2 "general_operand")))]
> -  ""
> -  "msp430_expand_helper (operands, \"__mspabi_sral\", !optimize_size);
> -   DONE;"
> -)
> -
> -(define_expand "ashrdi3"
> -  [(set (match_operand:DI	     0 "msp430_general_dst_nonv_operand")
> -	(ashift:DI (match_operand:DI 1 "general_operand")
> -		   (match_operand:DI 2 "general_operand")))]
> -  ""
> -  {
> -    /* No const_variant for 64-bit shifts.  */
> -    msp430_expand_helper (operands, \"__mspabi_srall\", false);
> -    DONE;
> -  }
> -)
> -
> -;;----------
> -
> -;; unsigned A >> C
> -
> -(define_expand "lshrhi3"
> -  [(set (match_operand:HI	       0 "msp430_general_dst_nonv_operand")
> -	(lshiftrt:HI (match_operand:HI 1 "general_operand")
> -		     (match_operand:HI 2 "general_operand")))]
> -  ""
> -  {
> -    if ((GET_CODE (operands[1]) == SUBREG
> -	 && REG_P (XEXP (operands[1], 0)))
> -	|| MEM_P (operands[1]))
> -      operands[1] = force_reg (HImode, operands[1]);
> -    if (msp430x
> -        && REG_P (operands[0])
> -        && REG_P (operands[1])
> -        && CONST_INT_P (operands[2]))
> -      emit_insn (gen_430x_logical_shift_right (operands[0], operands[1], operands[2]));
> -    else if (CONST_INT_P (operands[2])
> -	     && INTVAL (operands[2]) == 1)
> -      emit_insn (gen_srli_1 (operands[0], operands[1]));
> -    else		 
> -      msp430_expand_helper (operands, \"__mspabi_srli\", !optimize_size);
> -    DONE;
> -  }
> -)
> -
> -(define_insn "srli_1"
> -  [(set (match_operand:HI	       0 "msp430_general_dst_nonv_operand" "=rm")
> -	(lshiftrt:HI (match_operand:HI 1 "general_operand"       "0")
> -		     (const_int 1)))]
> -  ""
> -  "CLRC { RRC%X0.W\t%0"
> +  "@
> +  RLAM%b0\t%2, %0
> +  RPT\t%2 { RLAX%b0\t%0
> +  RPT\t#16 { RLAX%b0\t%0 { RPT\t%W2 { RLAX%b0\t%0
> +  # undefined behavior left shift of %1 by %2"
>  )
>  
> -(define_insn "430x_logical_shift_right"
> -  [(set (match_operand:HI              0 "register_operand" "=r")
> -	(lshiftrt:HI (match_operand:HI 1 "register_operand"  "0")
> -		     (match_operand    2 "immediate_operand" "n")))]
> +(define_insn "ashr<mode>3_430x"
> +  [(set (match_operand:HPSI		   0 "msp430_general_dst_nonv_operand" "=r,r,r,r")
> +	(ashiftrt:HPSI (match_operand:HPSI 1 "general_operand"	  	     "0,0,0,0")
> +		       (match_operand:HPSI 2 "const_int_operand" 	     "M,P,K,i")))]
>    "msp430x"
> -  {
> -    return msp430x_logical_shift_right (operands[2]);
> -  }
> -)
> -
> -(define_insn "srlp_1"
> -  [(set (match_operand:PSI              0 "register_operand" "=r")
> -	(lshiftrt:PSI (match_operand:PSI 1 "general_operand" "0")
> -		      (const_int 1)))]
> -  ""
> -  "RRUM.A #1,%0"
> -)
> -
> -(define_insn "srll_1"
> -  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=rm")
> -	(lshiftrt:SI (match_operand:SI 1 "general_operand"       "0")
> -		     (const_int 1)))]
> -  ""
> -  "CLRC { RRC%X0.W\t%H0 { RRC%X0.W\t%L0"
> +  "@
> +  RRAM%b0\t%2, %0
> +  RPT\t%2 { RRAX%b0\t%0
> +  RPT\t#16 { RRAX%b0\t%0 { RPT\t%W2 { RRAX%b0\t%0
> +  # undefined behavior arithmetic right shift of %1 by %2"
>  )
>  
> -(define_insn "srll_2x"
> -  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand" "=r")
> -	(lshiftrt:SI (match_operand:SI 1 "general_operand"       "0")
> -		     (const_int 2)))]
> +(define_insn "lshr<mode>3_430x"
> +  [(set (match_operand:HPSI		   0 "msp430_general_dst_nonv_operand" "=r,r,r,r")
> +	(lshiftrt:HPSI (match_operand:HPSI 1 "general_operand"	  	     "0,0,0,0")
> +		       (match_operand:HPSI 2 "const_int_operand" 	     "M,P,K,i")))]
>    "msp430x"
> -  "RRUX.W\t%H0 { RRC.W\t%L0 { RRUX.W\t%H0 { RRC.W\t%L0"
> -)
> -
> -(define_expand "lshrsi3"
> -  [(set (match_operand:SI	       0 "msp430_general_dst_nonv_operand")
> -	(lshiftrt:SI (match_operand:SI 1 "general_operand")
> -		     (match_operand:SI 2 "general_operand")))]
> -  ""
> -  "msp430_expand_helper (operands, \"__mspabi_srll\", !optimize_size);
> -   DONE;"
> -)
> -
> -(define_expand "lshrdi3"
> -  [(set (match_operand:DI	     0 "msp430_general_dst_nonv_operand")
> -	(ashift:DI (match_operand:DI 1 "general_operand")
> -		   (match_operand:DI 2 "general_operand")))]
> -  ""
> -  {
> -    /* No const_variant for 64-bit shifts.  */
> -    msp430_expand_helper (operands, \"__mspabi_srlll\", false);
> -    DONE;
> -  }
> +  "@
> +  RRUM%b0\t%2, %0
> +  RPT\t%2 { RRUX%b0\t%0
> +  RPT\t#16 { RRUX%b0\t%0 { RPT\t%W2 { RRUX%b0\t%0
> +  # undefined behavior logical right shift of %1 by %2"
>  )
>  
>  ;;------------------------------------------------------------
> @@ -1427,7 +1238,7 @@ (define_insn "*bitbranch<mode>4_z"
>    [(set (pc) (if_then_else
>  	      (ne (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rYs,rm")
>  				    (const_int 1)
> -				    (match_operand 1 "msp430_bitpos" "i,i"))
> +				    (match_operand 1 "const_0_to_15_operand" "i,i"))
>  		  (const_int 0))
>                (label_ref (match_operand 2 "" ""))
>  	      (pc)))
> @@ -1443,7 +1254,7 @@ (define_insn "*bitbranch<mode>4_z"
>    [(set (pc) (if_then_else
>  	      (eq (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rm")
>  				   (const_int 1)
> -				   (match_operand 1 "msp430_bitpos" "i"))
> +				   (match_operand 1 "const_0_to_15_operand" "i"))
>  		  (const_int 0))
>                (label_ref (match_operand 2 "" ""))
>  	      (pc)))
> @@ -1457,7 +1268,7 @@ (define_insn "*bitbranch<mode>4_z"
>    [(set (pc) (if_then_else
>  	      (eq (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rm")
>  				   (const_int 1)
> -				   (match_operand 1 "msp430_bitpos" "i"))
> +				   (match_operand 1 "const_0_to_15_operand" "i"))
>  		  (const_int 0))
>                (pc)
>  	      (label_ref (match_operand 2 "" ""))))
> @@ -1471,7 +1282,7 @@ (define_insn "*bitbranch<mode>4_z"
>    [(set (pc) (if_then_else
>  	      (ne (zero_extract:HI (match_operand:QHI 0 "msp430_general_dst_operand" "rm")
>  				   (const_int 1)
> -				   (match_operand 1 "msp430_bitpos" "i"))
> +				   (match_operand 1 "const_0_to_15_operand" "i"))
>  		  (const_int 0))
>                (pc)
>  	      (label_ref (match_operand 2 "" ""))))
> diff --git a/gcc/config/msp430/msp430.opt b/gcc/config/msp430/msp430.opt
> index b451174c3d1..8134ca7ac95 100644
> --- a/gcc/config/msp430/msp430.opt
> +++ b/gcc/config/msp430/msp430.opt
> @@ -109,3 +109,9 @@ mdevices-csv-loc=
>  Target Joined Var(msp430_devices_csv_loc) RejectNegative Report
>  The path to devices.csv.  The GCC driver can normally locate devices.csv itself
>  and pass this option to the compiler, so the user shouldn't need to pass this.
> +
> +mmax-inline-shift=
> +Target RejectNegative Joined UInteger IntegerRange(0,65) Var(msp430_max_inline_shift) Init(65) Report
> +For shift operations by a constant amount, which require an individual instruction to shift by one
> +position, set the maximum number of inline shift instructions (maximum value 64) to emit instead of using the corresponding __mspabi helper function.
> +The default value is 4.
> diff --git a/gcc/config/msp430/predicates.md b/gcc/config/msp430/predicates.md
> index 408d56f610a..4bfa0c0f2d5 100644
> --- a/gcc/config/msp430/predicates.md
> +++ b/gcc/config/msp430/predicates.md
> @@ -113,12 +113,21 @@ (define_predicate "msp430_nonsubregnonpostinc_or_imm_operand"
>         (ior (match_code "reg,mem")
>  	    (match_operand 0 "immediate_operand"))))
>  
> -; TRUE for constants which are bit positions for zero_extract
> -(define_predicate "msp430_bitpos"
> +(define_predicate "const_1_to_8_operand"
> +  (and (match_code "const_int")
> +       (match_test ("   INTVAL (op) >= 1
> +		     && INTVAL (op) <= 8 "))))
> +
> +(define_predicate "const_0_to_15_operand"
>    (and (match_code "const_int")
>         (match_test ("   INTVAL (op) >= 0
>  		     && INTVAL (op) <= 15 "))))
>  
> +(define_predicate "const_1_to_19_operand"
> +  (and (match_code "const_int")
> +       (match_test ("   INTVAL (op) >= 1
> +		     && INTVAL (op) <= 19 "))))
> +
>  (define_predicate "msp430_symbol_operand"
>    (match_code "symbol_ref")
>  )
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 09bcc5b0f78..885c7aae3a5 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1066,7 +1066,7 @@ Objective-C and Objective-C++ Dialects}.
>  -mwarn-mcu @gol
>  -mcode-region=  -mdata-region= @gol
>  -msilicon-errata=  -msilicon-errata-warn= @gol
> --mhwmult=  -minrt  -mtiny-printf}
> +-mhwmult=  -minrt  -mtiny-printf  -mmax-inline-shift=}
>  
>  @emph{NDS32 Options}
>  @gccoptlist{-mbig-endian  -mlittle-endian @gol
> @@ -24728,6 +24728,19 @@ buffered before it is sent to write.
>  This option requires Newlib Nano IO, so GCC must be configured with
>  @samp{--enable-newlib-nano-formatted-io}.
>  
> +@item -mmax-inline-shift=
> +@opindex mmax-inline-shift=
> +This option takes an integer between 0 and 64 inclusive, and sets
> +the maximum number of inline shift instructions which should be emitted to
> +perform a shift operation by a constant amount.  When this value needs to be
> +exceeded, an mspabi helper function is used instead.  The default value is 4.
> +
> +This only affects cases where a shift by multiple positions cannot be
> +completed with a single instruction (e.g. all shifts >1 on the 430 ISA).
> +
> +Shifts of a 32-bit value are at least twice as costly, so the value passed for
> +this option is divided by 2 and the resulting value used instead.
> +
>  @item -mcode-region=
>  @itemx -mdata-region=
>  @opindex mcode-region
> diff --git a/gcc/testsuite/gcc.target/msp430/emulate-srli.c b/gcc/testsuite/gcc.target/msp430/emulate-srli.c
> index f870d13f86b..35207b7c458 100644
> --- a/gcc/testsuite/gcc.target/msp430/emulate-srli.c
> +++ b/gcc/testsuite/gcc.target/msp430/emulate-srli.c
> @@ -2,7 +2,7 @@
>  /* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
>  /* { dg-options "-Os" } */
>  /* { dg-final { scan-assembler-not "mspabi_srli" } } */
> -/* { dg-final { scan-assembler "rrum" } } */
> +/* { dg-final { scan-assembler "RRUM" } } */
>  
>  /* Ensure that HImode shifts with source operand in memory are emulated with a
>     rotate instructions.  */
> diff --git a/gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
> new file mode 100644
> index 00000000000..c795f7570d7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430-no-opt.c
> @@ -0,0 +1,52 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430x" "-mlarge" } { "" } } */
> +/* { dg-options "-mcpu=msp430" } */
> +/* { dg-final { scan-assembler-not "__mspabi_slli_4" } } */
> +/* { dg-final { scan-assembler-not "__mspabi_sral_2" } } */
> +/* { dg-final { scan-assembler "__mspabi_slli_5" } } */
> +/* { dg-final { scan-assembler "__mspabi_sral_3" } } */
> +
> +/* Test the default value of 4 for -mmax-inline-shift has been observed.  */
> +
> +volatile int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15;
> +volatile long l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15;
> +
> +void
> +ashift (void)
> +{
> +  a1 <<= 1;
> +  a2 <<= 2;
> +  a3 <<= 3;
> +  a4 <<= 4;
> +  a5 <<= 5;
> +  a6 <<= 6;
> +  a7 <<= 7;
> +  a8 <<= 8;
> +  a9 <<= 9;
> +  a10 <<= 10;
> +  a11 <<= 11;
> +  a12 <<= 12;
> +  a13 <<= 13;
> +  a14 <<= 14;
> +  a15 <<= 15;
> +}
> +
> +void
> +ashiftrt (void)
> +{
> +  l1  >>= 1;
> +  l2  >>= 2;
> +  l3  >>= 3;
> +  l4  >>= 4;
> +  l5  >>= 5;
> +  l6  >>= 6;
> +  l7  >>= 7;
> +  l8  >>= 8;
> +  l9  >>= 9;
> +  l10 >>= 10;
> +  l11 >>= 11;
> +  l12 >>= 12;
> +  l13 >>= 13;
> +  l14 >>= 14;
> +  l15 >>= 15;
> +}
> diff --git a/gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
> new file mode 100644
> index 00000000000..7a519eec663
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430.c
> @@ -0,0 +1,50 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430x" "-mlarge" } { "" } } */
> +/* { dg-options "-mcpu=msp430 -mmax-inline-shift=10" } */
> +/* { dg-final { scan-assembler-not "__mspabi_slli_10" } } */
> +/* { dg-final { scan-assembler-not "__mspabi_sral_5" } } */
> +/* { dg-final { scan-assembler "__mspabi_slli_11" } } */
> +/* { dg-final { scan-assembler "__mspabi_sral_6" } } */
> +
> +volatile int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15;
> +volatile long l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15;
> +
> +void
> +ashift (void)
> +{
> +  a1 <<= 1;
> +  a2 <<= 2;
> +  a3 <<= 3;
> +  a4 <<= 4;
> +  a5 <<= 5;
> +  a6 <<= 6;
> +  a7 <<= 7;
> +  a8 <<= 8;
> +  a9 <<= 9;
> +  a10 <<= 10;
> +  a11 <<= 11;
> +  a12 <<= 12;
> +  a13 <<= 13;
> +  a14 <<= 14;
> +  a15 <<= 15;
> +}
> +
> +void
> +ashiftrt (void)
> +{
> +  l1  >>= 1;
> +  l2  >>= 2;
> +  l3  >>= 3;
> +  l4  >>= 4;
> +  l5  >>= 5;
> +  l6  >>= 6;
> +  l7  >>= 7;
> +  l8  >>= 8;
> +  l9  >>= 9;
> +  l10 >>= 10;
> +  l11 >>= 11;
> +  l12 >>= 12;
> +  l13 >>= 13;
> +  l14 >>= 14;
> +  l15 >>= 15;
> +}
> diff --git a/gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c
> new file mode 100644
> index 00000000000..074b3af5539
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/msp430/max-inline-shift-430x.c
> @@ -0,0 +1,48 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
> +/* { dg-options "-mcpu=msp430x -mmax-inline-shift=10" } */
> +/* { dg-final { scan-assembler-not "__mspabi_slli" } } */
> +/* { dg-final { scan-assembler "__mspabi_sral_6" } } */
> +
> +volatile int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15;
> +volatile long l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15;
> +
> +void
> +ashift (void)
> +{
> +  a1 <<= 1;
> +  a2 <<= 2;
> +  a3 <<= 3;
> +  a4 <<= 4;
> +  a5 <<= 5;
> +  a6 <<= 6;
> +  a7 <<= 7;
> +  a8 <<= 8;
> +  a9 <<= 9;
> +  a10 <<= 10;
> +  a11 <<= 11;
> +  a12 <<= 12;
> +  a13 <<= 13;
> +  a14 <<= 14;
> +  a15 <<= 15;
> +}
> +
> +void
> +ashiftrt (void)
> +{
> +  l1  >>= 1;
> +  l2  >>= 2;
> +  l3  >>= 3;
> +  l4  >>= 4;
> +  l5  >>= 5;
> +  l6  >>= 6;
> +  l7  >>= 7;
> +  l8  >>= 8;
> +  l9  >>= 9;
> +  l10 >>= 10;
> +  l11 >>= 11;
> +  l12 >>= 12;
> +  l13 >>= 13;
> +  l14 >>= 14;
> +  l15 >>= 15;
> +}
> diff --git a/libgcc/config/msp430/slli.S b/libgcc/config/msp430/slli.S
> index c31e2d5db9b..b22622e0bf5 100644
> --- a/libgcc/config/msp430/slli.S
> +++ b/libgcc/config/msp430/slli.S
> @@ -65,6 +65,21 @@ __mspabi_slli:
>  	RET
>  #endif
>  
> +#ifdef __MSP430X__
> +	.section	.text.__gnu_mspabi_sllp
> +1:	ADDA	#-1,R13
> +	ADDA	R12,R12
> +	.global	__gnu_mspabi_sllp
> +__gnu_mspabi_sllp:
> +	CMP	#0,R13
> +	JNZ	1b
> +#ifdef __MSP430X_LARGE__
> +	RETA
> +#else
> +	RET
> +#endif /* __MSP430X_LARGE__ */
> +#endif /* __MSP430X__ */
> +
>  /* Logical Left Shift - R12:R13 -> R12:R13.  */
>  
>  	.section	.text.__mspabi_slll_n
> diff --git a/libgcc/config/msp430/srai.S b/libgcc/config/msp430/srai.S
> index d4a47fa985a..0100a368365 100644
> --- a/libgcc/config/msp430/srai.S
> +++ b/libgcc/config/msp430/srai.S
> @@ -64,6 +64,21 @@ __mspabi_srai:
>  	RET
>  #endif
>  
> +#ifdef __MSP430X__
> +	.section	.text.__gnu_mspabi_srap
> +1:	ADDA	#-1,R13
> +	RRAX.A	R12,R12
> +	.global	__gnu_mspabi_srap
> +__gnu_mspabi_srap:
> +	CMP	#0,R13
> +	JNZ	1b
> +#ifdef __MSP430X_LARGE__
> +	RETA
> +#else
> +	RET
> +#endif /* __MSP430X_LARGE__ */
> +#endif /* __MSP430X__ */
> +
>  /* Arithmetic Right Shift - R12:R13 -> R12:R13.  */
>  
>  	.section	.text.__mspabi_sral_n
> diff --git a/libgcc/config/msp430/srli.S b/libgcc/config/msp430/srli.S
> index 838c4bc0617..50db47c9938 100644
> --- a/libgcc/config/msp430/srli.S
> +++ b/libgcc/config/msp430/srli.S
> @@ -66,6 +66,22 @@ __mspabi_srli:
>  	RET
>  #endif
>  
> +#ifdef __MSP430X__
> +	.section	.text.__gnu_mspabi_srlp
> +1:	ADDA	#-1,R13
> +	CLRC
> +	RRCX.A	R12,R12
> +	.global	__gnu_mspabi_srlp
> +__gnu_mspabi_srlp:
> +	CMP	#0,R13
> +	JNZ	1b
> +#ifdef __MSP430X_LARGE__
> +	RETA
> +#else
> +	RET
> +#endif /* __MSP430X_LARGE__ */
> +#endif /* __MSP430X__ */
> +
>  /* Logical Right Shift - R12:R13 -> R12:R13.  */
>  
>  	.section	.text.__mspabi_srll_n
> -- 
> 2.27.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns
  2020-07-21 18:29 ` [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns Jozef Lawrynowicz
  2020-08-07 10:55   ` ping " Jozef Lawrynowicz
@ 2020-08-25 19:16   ` Jeff Law
  1 sibling, 0 replies; 10+ messages in thread
From: Jeff Law @ 2020-08-25 19:16 UTC (permalink / raw)
  To: Jozef Lawrynowicz, gcc-patches

On Tue, 2020-07-21 at 19:29 +0100, Jozef Lawrynowicz wrote:
> The implementation of define_expand and define_insn patterns to handle
> shifts in the MSP430 backend is inconsistent, resulting in missed
> opportunities to make best use of the architecture's features.
> 
> There's now a single define_expand used as the entry point for all valid
> shifts, and the decision to either use a helper function to perform the
> shift (often required for the 430 ISA), or fall through to the
> define_insn patterns can be made from that expander function.
> 
> Shifts by a constant amount have been grouped into one define_insn for
> each type of shift, instead of having different define_insn patterns for
> shifts by different amounts.
> 
> A new target option "-mmax-inline-shift=" has been added to allow tuning
> of the number of shift instructions to emit inline, instead of using
> a library helper function.
> 
> Successfully regtested on trunk for msp430-elf, ok to apply?
OK
jeff


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-08-25 19:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 18:17 [PATCH 0/3] MSP430: Improve code-generation for shift instructions Jozef Lawrynowicz
2020-07-21 18:24 ` [PATCH 1/3] expr: Allow scalar_int_mode target mode when converting a constant Jozef Lawrynowicz
2020-07-22  8:33   ` Richard Sandiford
2020-07-22  9:48     ` Jozef Lawrynowicz
2020-07-24 14:14       ` Richard Sandiford
2020-07-21 18:26 ` [PATCH 2/3] expmed: Fix possible use of NULL_RTX return value from emit_store_flag Jozef Lawrynowicz
2020-07-22  8:38   ` Richard Sandiford
2020-07-21 18:29 ` [PATCH 3/3] MSP430: Simplify and extend shift instruction patterns Jozef Lawrynowicz
2020-08-07 10:55   ` ping " Jozef Lawrynowicz
2020-08-25 19:16   ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).