[RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen Lin]

Hi all,

define_insn_and_split should avoid to use empty split condition
if the condition for define_insn isn't empty, otherwise it can
sometimes result in unexpected consequence, since the split
will always be done even if the insn condition doesn't hold.

To avoid forgetting to add "&& 1" onto split condition, as
Segher suggested in thread[1], this series is to add the check
and raise an error if it catches the unexpected cases.  With
this new check, we have to fix up some existing
define_insn_and_split which are detected as error.  I hope all
these places are not intentional to be kept as blank.

Any comments are highly appreciated.

BR,
Kewen

[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566970.html

Kewen Lin (11):
  gen: Emit error msg for empty split condition
  arc: Update unexpected empty split condition
  arm: Update unexpected empty split condition
  cris: Update unexpected empty split condition
  h8300: Update unexpected empty split condition
  i386: Update unexpected empty split condition
  m68k: Update unexpected empty split condition
  mips: Update unexpected empty split condition
  or1k: Update unexpected empty split condition
  sh: Update unexpected empty split condition
  sparc: Update unexpected empty split condition

 gcc/config/arc/arc.md        |  2 +-
 gcc/config/arm/vfp.md        |  4 ++--
 gcc/config/cris/cris.md      |  2 +-
 gcc/config/h8300/combiner.md |  2 +-
 gcc/config/i386/i386.md      |  6 +++---
 gcc/config/i386/sse.md       |  8 ++++----
 gcc/config/m68k/m68k.md      |  6 +++---
 gcc/config/mips/mips.md      |  6 +++---
 gcc/config/or1k/or1k.md      |  2 +-
 gcc/config/sh/sh.md          |  2 +-
 gcc/config/sparc/sparc.md    | 12 ++++++------
 gcc/gensupport.c             |  3 +++
 12 files changed, 29 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH 01/11] gen: Emit error msg for empty split condition

[Kewen Lin]

As Segher suggested, this patch is to emit the error message
if the split condition of define_insn_and_split is empty while
the insn condition isn't.

gcc/ChangeLog:

	* gensupport.c (process_rtx): Emit error message for empty
	split condition in define_insn_and_split while the insn
	condition isn't.
---
 gcc/gensupport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index 0f19bd70664..52cee120215 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc)
 	  }
 	else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
 	  error_at (loc, "the rewrite condition must start with `&&'");
+	else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0)
+	  error_at (loc, "the split condition mustn't be empty if the "
+			 "insn condition isn't empty");
 	XSTR (split, 1) = split_cond;
 	if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
 	  XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
-- 
2.17.1

[PATCH 02/11] arc: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/arc/arc.md (*bbit_di): Fix empty split condition.
---
 gcc/config/arc/arc.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
index 7a52551eef5..a03840c4c36 100644
--- a/gcc/config/arc/arc.md
+++ b/gcc/config/arc/arc.md
@@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di"
    (clobber (reg:CC_ZN CC_REG))]
   "!CROSSING_JUMP_P (insn)"
   "#"
-  ""
+  "&& 1"
   [(parallel
      [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc)))
       (clobber (reg:CC_ZN CC_REG))])]
-- 
2.17.1

[PATCH 03/11] arm: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/arm/vfp.md (no_literal_pool_df_immediate,
	no_literal_pool_sf_immediate): Fix empty split condition.
---
 gcc/config/arm/vfp.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
index f97af92716b..55b6c1ac585 100644
--- a/gcc/config/arm/vfp.md
+++ b/gcc/config/arm/vfp.md
@@ -2129,7 +2129,7 @@ (define_insn_and_split "no_literal_pool_df_immediate"
    && !arm_const_double_rtx (operands[1])
    && !(TARGET_VFP_DOUBLE && vfp3_const_double_rtx (operands[1]))"
   "#"
-  ""
+  "&& 1"
   [(const_int 0)]
 {
   long buf[2];
@@ -2154,7 +2154,7 @@ (define_insn_and_split "no_literal_pool_sf_immediate"
    && TARGET_VFP_BASE
    && !vfp3_const_double_rtx (operands[1])"
   "#"
-  ""
+  "&& 1"
   [(const_int 0)]
 {
   long buf;
-- 
2.17.1

[PATCH 04/11] cris: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/cris/cris.md (*addi_reload): Fix empty split condition.
---
 gcc/config/cris/cris.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md
index 7de0ec63fcf..d5a3c703a83 100644
--- a/gcc/config/cris/cris.md
+++ b/gcc/config/cris/cris.md
@@ -1311,7 +1311,7 @@ (define_insn_and_split "*addi_reload"
    && (INTVAL (operands[3]) == 2 || INTVAL (operands[3]) == 4)
    && (reload_in_progress || reload_completed)"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(plus:SI (ashift:SI (match_dup 2) (match_dup 3)) (match_dup 1)))]
   "operands[3] = operands[3] == const2_rtx ? const1_rtx : const2_rtx;")
-- 
2.17.1

[PATCH 05/11] h8300: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/h8300/combiner.md (*andsi3_lshiftrt_n_sb): Fix empty split
	condition.
---
 gcc/config/h8300/combiner.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/h8300/combiner.md b/gcc/config/h8300/combiner.md
index 20e19da0419..e31bd507a6f 100644
--- a/gcc/config/h8300/combiner.md
+++ b/gcc/config/h8300/combiner.md
@@ -271,7 +271,7 @@ (define_insn_and_split "*andsi3_lshiftrt_n_sb"
   "exact_log2 (INTVAL (operands[3])) < 16
    && INTVAL (operands[2]) + exact_log2 (INTVAL (operands[3])) == 31"
   "#"
-  ""
+  "&& 1"
   [(parallel [(set (match_dup 0)
 		   (and:SI (lshiftrt:SI (match_dup 1) (match_dup 2))
 			   (match_dup 3)))
-- 
2.17.1

[PATCH 06/11] i386: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/i386/i386.md (*load_tp_x32_zext, *add_tp_x32_zext,
	*tls_dynamic_gnu2_combine_32): Fix empty split condition.
	* config/i386/sse.md (*<sse2_avx2>_pmovmskb_lt,
	*<sse2_avx2>_pmovmskb_zext_lt, *sse2_pmovmskb_ext_lt,
	*<sse4_1_avx2>_pblendvb_lt): Likewise.
---
 gcc/config/i386/i386.md | 6 +++---
 gcc/config/i386/sse.md  | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 9ff35d9a607..545d048906d 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext"
 	  (unspec:SI [(const_int 0)] UNSPEC_TP)))]
   "TARGET_X32"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(zero_extend:DI (match_dup 1)))]
 {
@@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext"
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_X32"
   "#"
-  ""
+  "&& 1"
   [(parallel
      [(set (match_dup 0)
      	   (zero_extend:DI
@@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32"
    (clobber (reg:CC FLAGS_REG))]
   "!TARGET_64BIT && TARGET_GNU2_TLS"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (match_dup 5))]
 {
   operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0];
diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
index 9d3728d1cb0..a9d78030119 100644
--- a/gcc/config/i386/sse.md
+++ b/gcc/config/i386/sse.md
@@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt"
 	  UNSPEC_MOVMSK))]
   "TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))]
   ""
@@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt"
 	    UNSPEC_MOVMSK)))]
   "TARGET_64BIT && TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
   ""
@@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt"
 	    UNSPEC_MOVMSK)))]
   "TARGET_64BIT && TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
   ""
@@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt"
 	  UNSPEC_BLENDV))]
   "TARGET_SSE4_1"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(unspec:VI1_AVX2
 	 [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))]
-- 
2.17.1

[PATCH 07/11] m68k: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/m68k/m68k.md (*zero_extend_inc, *zero_extend_dec,
	*zero_extendsidi2): Fix empty split condition.
---
 gcc/config/m68k/m68k.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/config/m68k/m68k.md b/gcc/config/m68k/m68k.md
index 59a456cd496..82d075e8bf0 100644
--- a/gcc/config/m68k/m68k.md
+++ b/gcc/config/m68k/m68k.md
@@ -1693,7 +1693,7 @@ (define_insn_and_split "*zero_extend_inc"
    GET_MODE_CLASS (GET_MODE (operands[1])) == MODE_INT &&
    GET_MODE_SIZE (GET_MODE (operands[0])) == GET_MODE_SIZE (GET_MODE (operands[1])) * 2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(const_int 0))
    (set (match_dup 0)
@@ -1710,7 +1710,7 @@ (define_insn_and_split "*zero_extend_dec"
    GET_MODE_CLASS (GET_MODE (operands[1])) == MODE_INT &&
    GET_MODE_SIZE (GET_MODE (operands[0])) == GET_MODE_SIZE (GET_MODE (operands[1])) * 2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(match_dup 1))
    (set (match_dup 0)
@@ -1764,7 +1764,7 @@ (define_insn_and_split "*zero_extendsidi2"
 	(zero_extend:DI (match_operand:SI 1 "nonimmediate_src_operand" "")))]
   "GET_CODE (operands[0]) != MEM || GET_CODE (operands[1]) != MEM"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 2)
 	(match_dup 1))
    (set (match_dup 3)
-- 
2.17.1

[PATCH 08/11] mips: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/mips/mips.md (<anonymous>, bswapsi2, bswapdi2): Fix empty
	split condition.
---
 gcc/config/mips/mips.md | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index eef3cfd50a8..455b9b802f6 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -5835,7 +5835,7 @@ (define_insn_and_split ""
 		     (match_operand:SI 2 "immediate_operand" "I")))]
   "TARGET_MIPS16"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (match_dup 1))
    (set (match_dup 0) (lshiftrt:SI (match_dup 0) (match_dup 2)))]
   ""
@@ -5871,7 +5871,7 @@ (define_insn_and_split "bswapsi2"
 	(bswap:SI (match_operand:SI 1 "register_operand" "d")))]
   "ISA_HAS_WSBH && ISA_HAS_ROR"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (unspec:SI [(match_dup 1)] UNSPEC_WSBH))
    (set (match_dup 0) (rotatert:SI (match_dup 0) (const_int 16)))]
   ""
@@ -5882,7 +5882,7 @@ (define_insn_and_split "bswapdi2"
 	(bswap:DI (match_operand:DI 1 "register_operand" "d")))]
   "TARGET_64BIT && ISA_HAS_WSBH"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (unspec:DI [(match_dup 1)] UNSPEC_DSBH))
    (set (match_dup 0) (unspec:DI [(match_dup 0)] UNSPEC_DSHD))]
   ""
-- 
2.17.1

[PATCH 09/11] or1k: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/or1k/or1k.md (*movdi): Fix empty split condition.
---
 gcc/config/or1k/or1k.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/or1k/or1k.md b/gcc/config/or1k/or1k.md
index eb94efba0e4..495b3e277ba 100644
--- a/gcc/config/or1k/or1k.md
+++ b/gcc/config/or1k/or1k.md
@@ -351,7 +351,7 @@ (define_insn_and_split "*movdi"
   "register_operand (operands[0], DImode)
    || reg_or_0_operand (operands[1], DImode)"
   "#"
-  ""
+  "&& 1"
   [(const_int 0)]
 {
   rtx l0 = operand_subword (operands[0], 0, 0, DImode);
-- 
2.17.1

[PATCH 10/11] sh: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/sh/sh.md (doloop_end_split): Fix empty split condition.
---
 gcc/config/sh/sh.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
index e3af9ae21c1..93ee7c9a7de 100644
--- a/gcc/config/sh/sh.md
+++ b/gcc/config/sh/sh.md
@@ -6424,7 +6424,7 @@ (define_insn_and_split "doloop_end_split"
    (clobber (reg:SI T_REG))]
   "TARGET_SH2"
   "#"
-  ""
+  "&& 1"
   [(parallel [(set (reg:SI T_REG)
 		   (eq:SI (match_dup 2) (const_int 1)))
 	      (set (match_dup 0) (plus:SI (match_dup 2) (const_int -1)))])
-- 
2.17.1

[PATCH 11/11] sparc: Update unexpected empty split condition

[Kewen Lin]

gcc/ChangeLog:

	* config/sparc/sparc.md (*snedi<W:mode>_zero_vis3,
	*neg_snedi<W:mode>_zero_subxc, *plus_snedi<W:mode>_zero,
	*plus_plus_snedi<W:mode>_zero, *minus_snedi<W:mode>_zero,
	*minus_minus_snedi<W:mode>_zero): Fix empty split condition.
---
 gcc/config/sparc/sparc.md | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gcc/config/sparc/sparc.md b/gcc/config/sparc/sparc.md
index c5d369626cc..0f85cb192c8 100644
--- a/gcc/config/sparc/sparc.md
+++ b/gcc/config/sparc/sparc.md
@@ -855,7 +855,7 @@ (define_insn_and_split "*snedi<W:mode>_zero_vis3"
    (clobber (reg:CCX CC_REG))]
   "TARGET_ARCH64 && TARGET_VIS3"
   "#"
-  ""
+  "&& 1"
   [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1)))
    (set (match_dup 0) (ltu:W (reg:CCXC CC_REG) (const_int 0)))]
   ""
@@ -882,7 +882,7 @@ (define_insn_and_split "*neg_snedi<W:mode>_zero_subxc"
    (clobber (reg:CCX CC_REG))]
   "TARGET_ARCH64 && TARGET_SUBXC"
   "#"
-  ""
+  "&& 1"
   [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1)))
    (set (match_dup 0) (neg:W (ltu:W (reg:CCXC CC_REG) (const_int 0))))]
   ""
@@ -984,7 +984,7 @@ (define_insn_and_split "*plus_snedi<W:mode>_zero"
    (clobber (reg:CCX CC_REG))]
   "TARGET_ARCH64 && TARGET_VIS3"
   "#"
-  ""
+  "&& 1"
   [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1)))
    (set (match_dup 0) (plus:W (ltu:W (reg:CCXC CC_REG) (const_int 0))
 			      (match_dup 2)))]
@@ -1000,7 +1000,7 @@ (define_insn_and_split "*plus_plus_snedi<W:mode>_zero"
    (clobber (reg:CCX CC_REG))]
   "TARGET_ARCH64 && TARGET_VIS3"
   "#"
-  ""
+  "&& 1"
   [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1)))
    (set (match_dup 0) (plus:W (plus:W (ltu:W (reg:CCXC CC_REG) (const_int 0))
 				      (match_dup 2))
@@ -1048,7 +1048,7 @@ (define_insn_and_split "*minus_snedi<W:mode>_zero"
    (clobber (reg:CCX CC_REG))]
   "TARGET_ARCH64 && TARGET_SUBXC"
   "#"
-  ""
+  "&& 1"
   [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1)))
    (set (match_dup 0) (minus:W (match_dup 2)
 			       (ltu:W (reg:CCXC CC_REG) (const_int 0))))]
@@ -1064,7 +1064,7 @@ (define_insn_and_split "*minus_minus_snedi<W:mode>_zero"
    (clobber (reg:CCX CC_REG))]
   "TARGET_ARCH64 && TARGET_SUBXC"
   "#"
-  ""
+  "&& 1"
   [(set (reg:CCXC CC_REG) (compare:CCXC (not:DI (match_dup 1)) (const_int -1)))
    (set (match_dup 0) (minus:W (minus:W (match_dup 2)
 				        (ltu:W (reg:CCXC CC_REG) (const_int 0)))
-- 
2.17.1

Re: [PATCH 10/11] sh: Update unexpected empty split condition

[Oleg Endo]

On Wed, 2021-06-02 at 00:05 -0500, Kewen Lin wrote:
> gcc/ChangeLog:
> 
> 	* config/sh/sh.md (doloop_end_split): Fix empty split condition.
> ---
>  gcc/config/sh/sh.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
> index e3af9ae21c1..93ee7c9a7de 100644
> --- a/gcc/config/sh/sh.md
> +++ b/gcc/config/sh/sh.md
> @@ -6424,7 +6424,7 @@ (define_insn_and_split "doloop_end_split"
>     (clobber (reg:SI T_REG))]
>    "TARGET_SH2"
>    "#"
> -  ""
> +  "&& 1"
>    [(parallel [(set (reg:SI T_REG)
>  		   (eq:SI (match_dup 2) (const_int 1)))
>  	      (set (match_dup 0) (plus:SI (match_dup 2) (const_int -1)))])

This is OK (obvious).

Cheers,
Oleg

Re: [PATCH 06/11] i386: Update unexpected empty split condition

[Uros Bizjak]

On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote:
>
> gcc/ChangeLog:
>
>         * config/i386/i386.md (*load_tp_x32_zext, *add_tp_x32_zext,
>         *tls_dynamic_gnu2_combine_32): Fix empty split condition.
>         * config/i386/sse.md (*<sse2_avx2>_pmovmskb_lt,
>         *<sse2_avx2>_pmovmskb_zext_lt, *sse2_pmovmskb_ext_lt,
>         *<sse4_1_avx2>_pblendvb_lt): Likewise.

OK.

Thanks,
Uros.

> ---
>  gcc/config/i386/i386.md | 6 +++---
>  gcc/config/i386/sse.md  | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 9ff35d9a607..545d048906d 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext"
>           (unspec:SI [(const_int 0)] UNSPEC_TP)))]
>    "TARGET_X32"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0)
>         (zero_extend:DI (match_dup 1)))]
>  {
> @@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext"
>     (clobber (reg:CC FLAGS_REG))]
>    "TARGET_X32"
>    "#"
> -  ""
> +  "&& 1"
>    [(parallel
>       [(set (match_dup 0)
>            (zero_extend:DI
> @@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32"
>     (clobber (reg:CC FLAGS_REG))]
>    "!TARGET_64BIT && TARGET_GNU2_TLS"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0) (match_dup 5))]
>  {
>    operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0];
> diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
> index 9d3728d1cb0..a9d78030119 100644
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt"
>           UNSPEC_MOVMSK))]
>    "TARGET_SSE2"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0)
>         (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))]
>    ""
> @@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt"
>             UNSPEC_MOVMSK)))]
>    "TARGET_64BIT && TARGET_SSE2"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0)
>         (zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
>    ""
> @@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt"
>             UNSPEC_MOVMSK)))]
>    "TARGET_64BIT && TARGET_SSE2"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0)
>         (sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
>    ""
> @@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt"
>           UNSPEC_BLENDV))]
>    "TARGET_SSE4_1"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0)
>         (unspec:VI1_AVX2
>          [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))]
> --
> 2.17.1
>

Re: [PATCH 02/11] arc: Update unexpected empty split condition

[Claudiu Zissulescu]

Hi,

Indeed, the split condition needs to be populated. However, I doubt that the pattern in question is used by the compiler. Do you have an example where it is exercised?

Thanks,
Claudiu
________________________________
From: Kewen Lin <linkw@linux.ibm.com>
Sent: Wednesday, June 2, 2021 8:04 AM
To: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Cc: Kewen Lin <linkw@linux.ibm.com>; gnu@amylaar.uk <gnu@amylaar.uk>; Claudiu Zissulescu <claziss@synopsys.com>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com>
Subject: [PATCH 02/11] arc: Update unexpected empty split condition

gcc/ChangeLog:

        * config/arc/arc.md (*bbit_di): Fix empty split condition.
---
 gcc/config/arc/arc.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
index 7a52551eef5..a03840c4c36 100644
--- a/gcc/config/arc/arc.md
+++ b/gcc/config/arc/arc.md
@@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di"
    (clobber (reg:CC_ZN CC_REG))]
   "!CROSSING_JUMP_P (insn)"
   "#"
-  ""
+  "&& 1"
   [(parallel
      [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc)))
       (clobber (reg:CC_ZN CC_REG))])]
--
2.17.1

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Richard Biener]

On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote:
>
> As Segher suggested, this patch is to emit the error message
> if the split condition of define_insn_and_split is empty while
> the insn condition isn't.

I wonder whether it would be a good idea to automagically make
the split condition "&& 1" via gensupport?

> gcc/ChangeLog:
>
>         * gensupport.c (process_rtx): Emit error message for empty
>         split condition in define_insn_and_split while the insn
>         condition isn't.
> ---
>  gcc/gensupport.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> index 0f19bd70664..52cee120215 100644
> --- a/gcc/gensupport.c
> +++ b/gcc/gensupport.c
> @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc)
>           }
>         else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>           error_at (loc, "the rewrite condition must start with `&&'");
> +       else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0)
> +         error_at (loc, "the split condition mustn't be empty if the "
> +                        "insn condition isn't empty");
>         XSTR (split, 1) = split_cond;
>         if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>           XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
> --
> 2.17.1
>

Re: [PATCH 02/11] arc: Update unexpected empty split condition

[Kewen.Lin]

Hi Claudiu,

on 2021/6/2 下午2:52, Claudiu Zissulescu wrote:
> Hi,
> 
> Indeed, the split condition needs to be populated. However, I doubt that the pattern in question is used by the compiler. Do you have an example where it is exercised?
> 

Thanks for the reply!  Sorry that I don't have an example, the gensupport change will emit an error message
for this pattern in build stage even without actual running.

BR,
Kewen

> Thanks,
> Claudiu
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Kewen Lin <linkw@linux.ibm.com>
> *Sent:* Wednesday, June 2, 2021 8:04 AM
> *To:* gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
> *Cc:* Kewen Lin <linkw@linux.ibm.com>; gnu@amylaar.uk <gnu@amylaar.uk>; Claudiu Zissulescu <claziss@synopsys.com>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com>
> *Subject:* [PATCH 02/11] arc: Update unexpected empty split condition
>  
> gcc/ChangeLog:
> 
>         * config/arc/arc.md (*bbit_di): Fix empty split condition.
> ---
>  gcc/config/arc/arc.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
> index 7a52551eef5..a03840c4c36 100644
> --- a/gcc/config/arc/arc.md
> +++ b/gcc/config/arc/arc.md
> @@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di"
>     (clobber (reg:CC_ZN CC_REG))]
>    "!CROSSING_JUMP_P (insn)"
>    "#"
> -  ""
> +  "&& 1"
>    [(parallel
>       [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc)))
>        (clobber (reg:CC_ZN CC_REG))])]
> -- 
> 2.17.1

Re: [PATCH 02/11] arc: Update unexpected empty split condition

[Claudiu Zissulescu]

Hi Kewen,

Maybe it is best just to remove the pattern entirely, I couldn't exercise it myself. I was secretly hopping someone could do it.
Please can you submit a patch which removes it if it is not too much trouble?

Thanks,
Claudiu
________________________________
From: Kewen.Lin <linkw@linux.ibm.com>
Sent: Wednesday, June 2, 2021 10:05 AM
To: Claudiu Zissulescu <claziss@synopsys.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Cc: gnu@amylaar.uk <gnu@amylaar.uk>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com>
Subject: Re: [PATCH 02/11] arc: Update unexpected empty split condition

Hi Claudiu,

on 2021/6/2 下午2:52, Claudiu Zissulescu wrote:
> Hi,
>
> Indeed, the split condition needs to be populated. However, I doubt that the pattern in question is used by the compiler. Do you have an example where it is exercised?
>

Thanks for the reply!  Sorry that I don't have an example, the gensupport change will emit an error message
for this pattern in build stage even without actual running.

BR,
Kewen

> Thanks,
> Claudiu
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Kewen Lin <linkw@linux.ibm.com>
> *Sent:* Wednesday, June 2, 2021 8:04 AM
> *To:* gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
> *Cc:* Kewen Lin <linkw@linux.ibm.com>; gnu@amylaar.uk <gnu@amylaar.uk>; Claudiu Zissulescu <claziss@synopsys.com>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com>
> *Subject:* [PATCH 02/11] arc: Update unexpected empty split condition
>
> gcc/ChangeLog:
>
>         * config/arc/arc.md (*bbit_di): Fix empty split condition.
> ---
>  gcc/config/arc/arc.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
> index 7a52551eef5..a03840c4c36 100644
> --- a/gcc/config/arc/arc.md
> +++ b/gcc/config/arc/arc.md
> @@ -5020,7 +5020,7 @@ (define_insn_and_split "*bbit_di"
>     (clobber (reg:CC_ZN CC_REG))]
>    "!CROSSING_JUMP_P (insn)"
>    "#"
> -  ""
> +  "&& 1"
>    [(parallel
>       [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc)))
>        (clobber (reg:CC_ZN CC_REG))])]
> --
> 2.17.1

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Kewen.Lin]

Hi Richi,

on 2021/6/2 下午3:04, Richard Biener wrote:
> On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote:
>>
>> As Segher suggested, this patch is to emit the error message
>> if the split condition of define_insn_and_split is empty while
>> the insn condition isn't.
> 
> I wonder whether it would be a good idea to automagically make
> the split condition "&& 1" via gensupport?
> 

Thanks for the comment!  Do you happen to have some similar examples?
IIUC this idea has to lay on the assumption always holding that when
one developer puts split condition as blank meanwhile leaves insn
condition as non-empty, he/she exactly wants to make split condition
the same as insn condition.  Once one developer doesn't think like
that way (that is to want split to deal with more), the automatic
condtion filling seems to over fill.

The way that the current patch provided is not to allow the developer
to write that kind of pattern, instead he/she has to go with separated
define_insn and define_split.

BR,
Kewen

>> gcc/ChangeLog:
>>
>>         * gensupport.c (process_rtx): Emit error message for empty
>>         split condition in define_insn_and_split while the insn
>>         condition isn't.
>> ---
>>  gcc/gensupport.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
>> index 0f19bd70664..52cee120215 100644
>> --- a/gcc/gensupport.c
>> +++ b/gcc/gensupport.c
>> @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc)
>>           }
>>         else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>>           error_at (loc, "the rewrite condition must start with `&&'");
>> +       else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0)
>> +         error_at (loc, "the split condition mustn't be empty if the "
>> +                        "insn condition isn't empty");
>>         XSTR (split, 1) = split_cond;
>>         if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>>           XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
>> --
>> 2.17.1
>>

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Richard Biener]

On Wed, Jun 2, 2021 at 9:28 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> Hi Richi,
>
> on 2021/6/2 下午3:04, Richard Biener wrote:
> > On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote:
> >>
> >> As Segher suggested, this patch is to emit the error message
> >> if the split condition of define_insn_and_split is empty while
> >> the insn condition isn't.
> >
> > I wonder whether it would be a good idea to automagically make
> > the split condition "&& 1" via gensupport?
> >
>
> Thanks for the comment!  Do you happen to have some similar examples?

Not sure, the docs say

@var{insn-pattern}, @var{condition}, @var{output-template}, and
@var{insn-attributes} are used as in @code{define_insn}.
...
The @var{split-condition} is also used as in
@code{define_split}, with the additional behavior that if the condition starts
with @samp{&&}, the condition used for the split will be the constructed as a
logical ``and'' of the split condition with the insn condition.

so one can indeed read this as "" meaning 'true' w/o considering the
define_insn condition.  But then we say

The @code{define_insn_and_split} construction provides exactly the same
functionality as two separate @code{define_insn} and @code{define_split}
patterns.  It exists for compactness, and as a maintenance tool to prevent
having to ensure the two patterns' templates match.

But then when I split a define_insn_and_split with a "" split condition
they are not functionally identical?  Also "" as split condition _does_
seem valid, just maybe unintended?  How would one create a
functionally equivalent example? "|| 1" will likely not work.

Note I'm not familiar with all the details here but the documentation
does seem ambiguous at best, not supporting to error on empty
split-conditions at least.

Richard.

> IIUC this idea has to lay on the assumption always holding that when
> one developer puts split condition as blank meanwhile leaves insn
> condition as non-empty, he/she exactly wants to make split condition
> the same as insn condition.  Once one developer doesn't think like
> that way (that is to want split to deal with more), the automatic
> condtion filling seems to over fill.
>
> The way that the current patch provided is not to allow the developer
> to write that kind of pattern, instead he/she has to go with separated
> define_insn and define_split.
>
> BR,
> Kewen
>
> >> gcc/ChangeLog:
> >>
> >>         * gensupport.c (process_rtx): Emit error message for empty
> >>         split condition in define_insn_and_split while the insn
> >>         condition isn't.
> >> ---
> >>  gcc/gensupport.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> >> index 0f19bd70664..52cee120215 100644
> >> --- a/gcc/gensupport.c
> >> +++ b/gcc/gensupport.c
> >> @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc)
> >>           }
> >>         else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> >>           error_at (loc, "the rewrite condition must start with `&&'");
> >> +       else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0)
> >> +         error_at (loc, "the split condition mustn't be empty if the "
> >> +                        "insn condition isn't empty");
> >>         XSTR (split, 1) = split_cond;
> >>         if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> >>           XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
> >> --
> >> 2.17.1
> >>
>

[PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di

[Kewen.Lin]

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

Hi Claudiu,

on 2021/6/2 下午3:12, Claudiu Zissulescu wrote:
> Hi Kewen,
> 
> Maybe it is best just to remove the pattern entirely, I couldn't exercise it myself. I was secretly hopping someone could do it.
> Please can you submit a patch which removes it if it is not too much trouble?
> 

The patch v2 has been attached which removes define_insn_and_split "*bbit_di" as you suggested.

Does it look good to you?

BR,
Kewen
-----
gcc/ChangeLog:

	* config/arc/arc.md (*bbit_di): Remove.

[-- Attachment #2: arc_v2.diff --]
[-- Type: text/plain, Size: 1419 bytes --]

diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
index b6f2d8e28be..a67bb581003 100644
--- a/gcc/config/arc/arc.md
+++ b/gcc/config/arc/arc.md
@@ -5016,34 +5016,6 @@ (define_insn "*bbit"
 	(if_then_else (match_test "get_attr_length (insn) == 6")
 		      (const_string "true") (const_string "false")))])
 
-; ??? When testing a bit from a DImode register, combine creates a
-; zero_extract in DImode.  This goes via an AND with a DImode constant,
-; so can only be observed on 64 bit hosts.
-(define_insn_and_split "*bbit_di"
-  [(set (pc)
-	(if_then_else
-	  (match_operator 3 "equality_comparison_operator"
-	    [(zero_extract:DI (match_operand:SI 1 "register_operand" "Rcqq,c")
-			      (const_int 1)
-			      (match_operand 2 "immediate_operand" "L,L"))
-	     (const_int 0)])
-	  (label_ref (match_operand 0 "" ""))
-	  (pc)))
-   (clobber (reg:CC_ZN CC_REG))]
-  "!CROSSING_JUMP_P (insn)"
-  "#"
-  ""
-  [(parallel
-     [(set (pc) (if_then_else (match_dup 3) (label_ref (match_dup 0)) (pc)))
-      (clobber (reg:CC_ZN CC_REG))])]
-{
-  rtx xtr;
-
-  xtr = gen_rtx_ZERO_EXTRACT (SImode, operands[1], const1_rtx, operands[2]);
-  operands[3] = gen_rtx_fmt_ee (GET_CODE (operands[3]), GET_MODE (operands[3]),
-				xtr, const0_rtx);
-})
-
 ;; -------------------------------------------------------------------
 ;; Hardware loop
 ;; -------------------------------------------------------------------

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Sandiford]

Kewen Lin <linkw@linux.ibm.com> writes:
> Hi all,
>
> define_insn_and_split should avoid to use empty split condition
> if the condition for define_insn isn't empty, otherwise it can
> sometimes result in unexpected consequence, since the split
> will always be done even if the insn condition doesn't hold.
>
> To avoid forgetting to add "&& 1" onto split condition, as
> Segher suggested in thread[1], this series is to add the check
> and raise an error if it catches the unexpected cases.  With
> this new check, we have to fix up some existing
> define_insn_and_split which are detected as error.  I hope all
> these places are not intentional to be kept as blank.

I wonder whether we should instead redefine the semantics of
define_insn_and_split so that the split condition is always applied
on top of the insn condition.  It's rare for a define_insn_and_split
to have independent insn and split conditions, so at the moment,
we're making the common case hard.

Thanks,
Richard

> Any comments are highly appreciated.
>
> BR,
> Kewen
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566970.html
>
> Kewen Lin (11):
>   gen: Emit error msg for empty split condition
>   arc: Update unexpected empty split condition
>   arm: Update unexpected empty split condition
>   cris: Update unexpected empty split condition
>   h8300: Update unexpected empty split condition
>   i386: Update unexpected empty split condition
>   m68k: Update unexpected empty split condition
>   mips: Update unexpected empty split condition
>   or1k: Update unexpected empty split condition
>   sh: Update unexpected empty split condition
>   sparc: Update unexpected empty split condition
>
>  gcc/config/arc/arc.md        |  2 +-
>  gcc/config/arm/vfp.md        |  4 ++--
>  gcc/config/cris/cris.md      |  2 +-
>  gcc/config/h8300/combiner.md |  2 +-
>  gcc/config/i386/i386.md      |  6 +++---
>  gcc/config/i386/sse.md       |  8 ++++----
>  gcc/config/m68k/m68k.md      |  6 +++---
>  gcc/config/mips/mips.md      |  6 +++---
>  gcc/config/or1k/or1k.md      |  2 +-
>  gcc/config/sh/sh.md          |  2 +-
>  gcc/config/sparc/sparc.md    | 12 ++++++------
>  gcc/gensupport.c             |  3 +++
>  12 files changed, 29 insertions(+), 26 deletions(-)

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Kewen.Lin]

on 2021/6/2 下午3:43, Richard Biener wrote:
> On Wed, Jun 2, 2021 at 9:28 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> Hi Richi,
>>
>> on 2021/6/2 下午3:04, Richard Biener wrote:
>>> On Wed, Jun 2, 2021 at 7:05 AM Kewen Lin <linkw@linux.ibm.com> wrote:
>>>>
>>>> As Segher suggested, this patch is to emit the error message
>>>> if the split condition of define_insn_and_split is empty while
>>>> the insn condition isn't.
>>>
>>> I wonder whether it would be a good idea to automagically make
>>> the split condition "&& 1" via gensupport?
>>>
>>
>> Thanks for the comment!  Do you happen to have some similar examples?
> 
> Not sure, the docs say
> 
> @var{insn-pattern}, @var{condition}, @var{output-template}, and
> @var{insn-attributes} are used as in @code{define_insn}.
> ...
> The @var{split-condition} is also used as in
> @code{define_split}, with the additional behavior that if the condition starts
> with @samp{&&}, the condition used for the split will be the constructed as a
> logical ``and'' of the split condition with the insn condition.
> 
> so one can indeed read this as "" meaning 'true' w/o considering the
> define_insn condition.  

Yes, the "" in split condition does mean 'true' (always).

> But then we say
> 
> The @code{define_insn_and_split} construction provides exactly the same
> functionality as two separate @code{define_insn} and @code{define_split}
> patterns.  It exists for compactness, and as a maintenance tool to prevent
> having to ensure the two patterns' templates match.
> 
> But then when I split a define_insn_and_split with a "" split condition
> they are not functionally identical?  

Without this patch, they are indeed functionally identical.  It's like
the writer want to have one define_insn to match under some condition, but
want to have one define_split to match always.

> Also "" as split condition _does_
> seem valid, just maybe unintended?  

Yes, it's valid without this patch.  That's why I asked whether there is
some good reason to keep it be [1].  In Segher's opinion, there is no
good reason, he pointed out "A reader does not expect a
define_insn_and_split to split any other insns."

> How would one create a
> functionally equivalent example? "|| 1" will likely not work.
> 

I think "|| 1" works just like "" if people want the define_split to
split all the time, even with this patch.


> Note I'm not familiar with all the details here but the documentation
> does seem ambiguous at best, not supporting to error on empty
> split-conditions at least.
> 

Yes, the current patch will stop the "" condition which was accepted
before.  Thanks for bringing this up!  We have to update the
documentation if people reach a consensus.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567014.html


BR,
Kewen

Re: [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di

[Claudiu Zissulescu]

Looks good :) You can go ahead and commit it.

Thank you for your contribution,
Claudiu
________________________________
From: Kewen.Lin <linkw@linux.ibm.com>
Sent: Wednesday, June 2, 2021 10:43 AM
To: Claudiu Zissulescu <claziss@synopsys.com>
Cc: gnu@amylaar.uk <gnu@amylaar.uk>; andrew.burgess@embecosm.com <andrew.burgess@embecosm.com>; gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>
Subject: [PATCH 02/11 v2] arc: Remove define_insn_and_split *bbit_di

Hi Claudiu,

on 2021/6/2 下午3:12, Claudiu Zissulescu wrote:
> Hi Kewen,
>
> Maybe it is best just to remove the pattern entirely, I couldn't exercise it myself. I was secretly hopping someone could do it.
> Please can you submit a patch which removes it if it is not too much trouble?
>

The patch v2 has been attached which removes define_insn_and_split "*bbit_di" as you suggested.

Does it look good to you?

BR,
Kewen
-----
gcc/ChangeLog:

        * config/arc/arc.md (*bbit_di): Remove.

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

Hi Richard,

on 2021/6/2 下午4:11, Richard Sandiford wrote:
> Kewen Lin <linkw@linux.ibm.com> writes:
>> Hi all,
>>
>> define_insn_and_split should avoid to use empty split condition
>> if the condition for define_insn isn't empty, otherwise it can
>> sometimes result in unexpected consequence, since the split
>> will always be done even if the insn condition doesn't hold.
>>
>> To avoid forgetting to add "&& 1" onto split condition, as
>> Segher suggested in thread[1], this series is to add the check
>> and raise an error if it catches the unexpected cases.  With
>> this new check, we have to fix up some existing
>> define_insn_and_split which are detected as error.  I hope all
>> these places are not intentional to be kept as blank.
> 
> I wonder whether we should instead redefine the semantics of
> define_insn_and_split so that the split condition is always applied
> on top of the insn condition.  It's rare for a define_insn_and_split
> to have independent insn and split conditions, so at the moment,
> we're making the common case hard.
> 

Just want to confirm that the suggestion is just applied for empty
split condition or all split conditions in define_insn_and_split? 
I guess it's the former?  It's like what Richi suggested with
auto-filling, the semantics redefining will eliminate the over-fill
concern.  Thanks for the suggestion!

BR,
Kewen

RE: [PATCH 03/11] arm: Update unexpected empty split condition

[Kyrylo Tkachov]

> -----Original Message-----
> From: Kewen Lin <linkw@linux.ibm.com>
> Sent: 02 June 2021 06:05
> To: gcc-patches@gcc.gnu.org
> Cc: Kewen Lin <linkw@linux.ibm.com>; nickc@redhat.com; Richard
> Earnshaw <Richard.Earnshaw@arm.com>; Ramana Radhakrishnan
> <Ramana.Radhakrishnan@arm.com>; Kyrylo Tkachov
> <Kyrylo.Tkachov@arm.com>
> Subject: [PATCH 03/11] arm: Update unexpected empty split condition
> 
> gcc/ChangeLog:
> 
> 	* config/arm/vfp.md (no_literal_pool_df_immediate,
> 	no_literal_pool_sf_immediate): Fix empty split condition.

Ok, thanks for doing this.
Kyrill

> ---
>  gcc/config/arm/vfp.md | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/arm/vfp.md b/gcc/config/arm/vfp.md
> index f97af92716b..55b6c1ac585 100644
> --- a/gcc/config/arm/vfp.md
> +++ b/gcc/config/arm/vfp.md
> @@ -2129,7 +2129,7 @@ (define_insn_and_split
> "no_literal_pool_df_immediate"
>     && !arm_const_double_rtx (operands[1])
>     && !(TARGET_VFP_DOUBLE && vfp3_const_double_rtx (operands[1]))"
>    "#"
> -  ""
> +  "&& 1"
>    [(const_int 0)]
>  {
>    long buf[2];
> @@ -2154,7 +2154,7 @@ (define_insn_and_split
> "no_literal_pool_sf_immediate"
>     && TARGET_VFP_BASE
>     && !vfp3_const_double_rtx (operands[1])"
>    "#"
> -  ""
> +  "&& 1"
>    [(const_int 0)]
>  {
>    long buf;
> --
> 2.17.1

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Sandiford]

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Richard,
>
> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:
>> Kewen Lin <linkw@linux.ibm.com> writes:
>>> Hi all,
>>>
>>> define_insn_and_split should avoid to use empty split condition
>>> if the condition for define_insn isn't empty, otherwise it can
>>> sometimes result in unexpected consequence, since the split
>>> will always be done even if the insn condition doesn't hold.
>>>
>>> To avoid forgetting to add "&& 1" onto split condition, as
>>> Segher suggested in thread[1], this series is to add the check
>>> and raise an error if it catches the unexpected cases.  With
>>> this new check, we have to fix up some existing
>>> define_insn_and_split which are detected as error.  I hope all
>>> these places are not intentional to be kept as blank.
>> 
>> I wonder whether we should instead redefine the semantics of
>> define_insn_and_split so that the split condition is always applied
>> on top of the insn condition.  It's rare for a define_insn_and_split
>> to have independent insn and split conditions, so at the moment,
>> we're making the common case hard.
>> 
>
> Just want to confirm that the suggestion is just applied for empty
> split condition or all split conditions in define_insn_and_split? 
> I guess it's the former?

No, I meant tha latter.  E.g. in:

(define_insn_and_split
  […]
  "TARGET_FOO"
  "…"
  […]
  "reload_completed"
  […]
)

the "reload_completed" condition is almost always a typo for
"&& reload_completed".

Like I say, it rarely makes sense for the split condition to
ignore the insn condition and specify an entirely independent condition.
There might be some define_insn_and_splits that do that, but it'd often
be less confusing to write the insn and split separately if so.

Even if we do want to support independent insn and split conditions,
that's always going to be the rare and surprising case, so it's the one
that should need extra syntax.

Thanks,
Richard

  It's like what Richi suggested with
> auto-filling, the semantics redefining will eliminate the over-fill
> concern.  Thanks for the suggestion!
>
> BR,
> Kewen

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

on 2021/6/2 下午5:13, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi Richard,
>>
>> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:
>>> Kewen Lin <linkw@linux.ibm.com> writes:
>>>> Hi all,
>>>>
>>>> define_insn_and_split should avoid to use empty split condition
>>>> if the condition for define_insn isn't empty, otherwise it can
>>>> sometimes result in unexpected consequence, since the split
>>>> will always be done even if the insn condition doesn't hold.
>>>>
>>>> To avoid forgetting to add "&& 1" onto split condition, as
>>>> Segher suggested in thread[1], this series is to add the check
>>>> and raise an error if it catches the unexpected cases.  With
>>>> this new check, we have to fix up some existing
>>>> define_insn_and_split which are detected as error.  I hope all
>>>> these places are not intentional to be kept as blank.
>>>
>>> I wonder whether we should instead redefine the semantics of
>>> define_insn_and_split so that the split condition is always applied
>>> on top of the insn condition.  It's rare for a define_insn_and_split
>>> to have independent insn and split conditions, so at the moment,
>>> we're making the common case hard.
>>>
>>
>> Just want to confirm that the suggestion is just applied for empty
>> split condition or all split conditions in define_insn_and_split? 
>> I guess it's the former?
> 
> No, I meant tha latter.  E.g. in:
> 
> (define_insn_and_split
>   […]
>   "TARGET_FOO"
>   "…"
>   […]
>   "reload_completed"
>   […]
> )
> 
> the "reload_completed" condition is almost always a typo for
> "&& reload_completed".
> 
> Like I say, it rarely makes sense for the split condition to
> ignore the insn condition and specify an entirely independent condition.
> There might be some define_insn_and_splits that do that, but it'd often
> be less confusing to write the insn and split separately if so.
> 
> Even if we do want to support independent insn and split conditions,
> that's always going to be the rare and surprising case, so it's the one
> that should need extra syntax.
> 

Thanks for the clarification!

Since it may impact all ports, I wonder if there is a way to find out
this kind of "rare and surprising" case without a big coverage testing?
I'm happy to make a draft patch for it, but not sure how to early catch
those cases which need to be rewritten for those ports that I can't test
on (even with cfarm machines, the coverage seems still limited).

BR,
Kewen

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Biener]

On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> on 2021/6/2 下午5:13, Richard Sandiford wrote:
> > "Kewen.Lin" <linkw@linux.ibm.com> writes:
> >> Hi Richard,
> >>
> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:
> >>> Kewen Lin <linkw@linux.ibm.com> writes:
> >>>> Hi all,
> >>>>
> >>>> define_insn_and_split should avoid to use empty split condition
> >>>> if the condition for define_insn isn't empty, otherwise it can
> >>>> sometimes result in unexpected consequence, since the split
> >>>> will always be done even if the insn condition doesn't hold.
> >>>>
> >>>> To avoid forgetting to add "&& 1" onto split condition, as
> >>>> Segher suggested in thread[1], this series is to add the check
> >>>> and raise an error if it catches the unexpected cases.  With
> >>>> this new check, we have to fix up some existing
> >>>> define_insn_and_split which are detected as error.  I hope all
> >>>> these places are not intentional to be kept as blank.
> >>>
> >>> I wonder whether we should instead redefine the semantics of
> >>> define_insn_and_split so that the split condition is always applied
> >>> on top of the insn condition.  It's rare for a define_insn_and_split
> >>> to have independent insn and split conditions, so at the moment,
> >>> we're making the common case hard.
> >>>
> >>
> >> Just want to confirm that the suggestion is just applied for empty
> >> split condition or all split conditions in define_insn_and_split?
> >> I guess it's the former?
> >
> > No, I meant tha latter.  E.g. in:
> >
> > (define_insn_and_split
> >   […]
> >   "TARGET_FOO"
> >   "…"
> >   […]
> >   "reload_completed"
> >   […]
> > )
> >
> > the "reload_completed" condition is almost always a typo for
> > "&& reload_completed".
> >
> > Like I say, it rarely makes sense for the split condition to
> > ignore the insn condition and specify an entirely independent condition.
> > There might be some define_insn_and_splits that do that, but it'd often
> > be less confusing to write the insn and split separately if so.
> >
> > Even if we do want to support independent insn and split conditions,
> > that's always going to be the rare and surprising case, so it's the one
> > that should need extra syntax.
> >
>
> Thanks for the clarification!
>
> Since it may impact all ports, I wonder if there is a way to find out
> this kind of "rare and surprising" case without a big coverage testing?
> I'm happy to make a draft patch for it, but not sure how to early catch
> those cases which need to be rewritten for those ports that I can't test
> on (even with cfarm machines, the coverage seems still limited).

So what Richard suggests would be to disallow split conditions
that do not start with "&& ", it's probably easy to do that as well
and look for build fails.  That should catch all cases to look at.

Richard.

> BR,
> Kewen
>

Re: [PATCH 04/11] cris: Update unexpected empty split condition

[Hans-Peter Nilsson]

> From: Kewen Lin <linkw@linux.ibm.com>
> Date: Wed, 2 Jun 2021 07:04:54 +0200

> gcc/ChangeLog:
> 
> 	* config/cris/cris.md (*addi_reload): Fix empty split condition.
> ---
>  gcc/config/cris/cris.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md
> index 7de0ec63fcf..d5a3c703a83 100644
> --- a/gcc/config/cris/cris.md
> +++ b/gcc/config/cris/cris.md
> @@ -1311,7 +1311,7 @@ (define_insn_and_split "*addi_reload"
>     && (INTVAL (operands[3]) == 2 || INTVAL (operands[3]) == 4)
>     && (reload_in_progress || reload_completed)"
>    "#"
> -  ""
> +  "&& 1"
>    [(set (match_dup 0)
>  	(plus:SI (ashift:SI (match_dup 2) (match_dup 3)) (match_dup 1)))]
>    "operands[3] = operands[3] == const2_rtx ? const1_rtx : const2_rtx;")
> -- 
> 2.17.1
> 

Ok, thanks, if only for all-round consistency.

In preparation for a warning for an empty condition?  I'm
usually all for .md-warnings, but I'm not sure about the
benefit of that one, though.  Those "&& 1" look...hackish.

brgds, H-P

Re: [PATCH 07/11] m68k: Update unexpected empty split condition

[Jeff Law]

On 6/1/2021 11:04 PM, Kewen Lin wrote:
> gcc/ChangeLog:
>
> 	* config/m68k/m68k.md (*zero_extend_inc, *zero_extend_dec,
> 	*zero_extendsidi2): Fix empty split condition.
OK.  Thanks.
jeff

Re: [PATCH 05/11] h8300: Update unexpected empty split condition

[Jeff Law]

On 6/1/2021 11:04 PM, Kewen Lin wrote:
> gcc/ChangeLog:
>
> 	* config/h8300/combiner.md (*andsi3_lshiftrt_n_sb): Fix empty split
> 	condition.
Hold off on this.  We may need a stronger condition in there and that's 
something I'm in the process of cleaning up in the H8 port.




jeff

Re: [PATCH 08/11] mips: Update unexpected empty split condition

[Jeff Law]

On 6/1/2021 11:04 PM, Kewen Lin via Gcc-patches wrote:
> gcc/ChangeLog:
>
> 	* config/mips/mips.md (<anonymous>, bswapsi2, bswapdi2): Fix empty
> 	split condition.
The mips, or1k and sparc changes are fine.  They're all preserving 
existing behavior.


jeff

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Sandiford]

Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> on 2021/6/2 下午5:13, Richard Sandiford wrote:
>> > "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> >> Hi Richard,
>> >>
>> >> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:
>> >>> Kewen Lin <linkw@linux.ibm.com> writes:
>> >>>> Hi all,
>> >>>>
>> >>>> define_insn_and_split should avoid to use empty split condition
>> >>>> if the condition for define_insn isn't empty, otherwise it can
>> >>>> sometimes result in unexpected consequence, since the split
>> >>>> will always be done even if the insn condition doesn't hold.
>> >>>>
>> >>>> To avoid forgetting to add "&& 1" onto split condition, as
>> >>>> Segher suggested in thread[1], this series is to add the check
>> >>>> and raise an error if it catches the unexpected cases.  With
>> >>>> this new check, we have to fix up some existing
>> >>>> define_insn_and_split which are detected as error.  I hope all
>> >>>> these places are not intentional to be kept as blank.
>> >>>
>> >>> I wonder whether we should instead redefine the semantics of
>> >>> define_insn_and_split so that the split condition is always applied
>> >>> on top of the insn condition.  It's rare for a define_insn_and_split
>> >>> to have independent insn and split conditions, so at the moment,
>> >>> we're making the common case hard.
>> >>>
>> >>
>> >> Just want to confirm that the suggestion is just applied for empty
>> >> split condition or all split conditions in define_insn_and_split?
>> >> I guess it's the former?
>> >
>> > No, I meant tha latter.  E.g. in:
>> >
>> > (define_insn_and_split
>> >   […]
>> >   "TARGET_FOO"
>> >   "…"
>> >   […]
>> >   "reload_completed"
>> >   […]
>> > )
>> >
>> > the "reload_completed" condition is almost always a typo for
>> > "&& reload_completed".
>> >
>> > Like I say, it rarely makes sense for the split condition to
>> > ignore the insn condition and specify an entirely independent condition.
>> > There might be some define_insn_and_splits that do that, but it'd often
>> > be less confusing to write the insn and split separately if so.
>> >
>> > Even if we do want to support independent insn and split conditions,
>> > that's always going to be the rare and surprising case, so it's the one
>> > that should need extra syntax.
>> >
>>
>> Thanks for the clarification!
>>
>> Since it may impact all ports, I wonder if there is a way to find out
>> this kind of "rare and surprising" case without a big coverage testing?
>> I'm happy to make a draft patch for it, but not sure how to early catch
>> those cases which need to be rewritten for those ports that I can't test
>> on (even with cfarm machines, the coverage seems still limited).
>
> So what Richard suggests would be to disallow split conditions
> that do not start with "&& ", it's probably easy to do that as well
> and look for build fails.  That should catch all cases to look at.

Yeah.  As a strawman proposal, how about:

- add a new "define_independent_insn_and_split" that has the
  current semantics of define_insn_and_split.  This should be
  mechanical.

- find the define_insn_and_splits that are missing the "&&", and where
  missing the "&&" might make a difference.  Change them to
  define_independent_insn_and_splits.

  Like Richard says, this can be done by temporarily disallowing
  define_insn_and_splits that have no "&&".

  I think this should remain a mechanical step.  If port maintainers
  think that the missing "&&" is a mistake, they should fix it as
  a separate patch.

- flip the default for define_insn_and_split so that the "&&" is implicit
  (but can still be specified redundantly)

Then port maintainers who don't mind the churn can remove the
redundant "&&"s from the remaining define_insn_and_splits.

Thanks,
Richard

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Jeff Law]

On 6/2/2021 11:32 AM, Richard Sandiford wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Wed, Jun 2, 2021 at 12:01 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>> on 2021/6/2 下午5:13, Richard Sandiford wrote:
>>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>>> Hi Richard,
>>>>>
>>>>> on 2021/6/2 锟斤拷锟斤拷4:11, Richard Sandiford wrote:
>>>>>> Kewen Lin <linkw@linux.ibm.com> writes:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> define_insn_and_split should avoid to use empty split condition
>>>>>>> if the condition for define_insn isn't empty, otherwise it can
>>>>>>> sometimes result in unexpected consequence, since the split
>>>>>>> will always be done even if the insn condition doesn't hold.
>>>>>>>
>>>>>>> To avoid forgetting to add "&& 1" onto split condition, as
>>>>>>> Segher suggested in thread[1], this series is to add the check
>>>>>>> and raise an error if it catches the unexpected cases.  With
>>>>>>> this new check, we have to fix up some existing
>>>>>>> define_insn_and_split which are detected as error.  I hope all
>>>>>>> these places are not intentional to be kept as blank.
>>>>>> I wonder whether we should instead redefine the semantics of
>>>>>> define_insn_and_split so that the split condition is always applied
>>>>>> on top of the insn condition.  It's rare for a define_insn_and_split
>>>>>> to have independent insn and split conditions, so at the moment,
>>>>>> we're making the common case hard.
>>>>>>
>>>>> Just want to confirm that the suggestion is just applied for empty
>>>>> split condition or all split conditions in define_insn_and_split?
>>>>> I guess it's the former?
>>>> No, I meant tha latter.  E.g. in:
>>>>
>>>> (define_insn_and_split
>>>>    […]
>>>>    "TARGET_FOO"
>>>>    "…"
>>>>    […]
>>>>    "reload_completed"
>>>>    […]
>>>> )
>>>>
>>>> the "reload_completed" condition is almost always a typo for
>>>> "&& reload_completed".
>>>>
>>>> Like I say, it rarely makes sense for the split condition to
>>>> ignore the insn condition and specify an entirely independent condition.
>>>> There might be some define_insn_and_splits that do that, but it'd often
>>>> be less confusing to write the insn and split separately if so.
>>>>
>>>> Even if we do want to support independent insn and split conditions,
>>>> that's always going to be the rare and surprising case, so it's the one
>>>> that should need extra syntax.
>>>>
>>> Thanks for the clarification!
>>>
>>> Since it may impact all ports, I wonder if there is a way to find out
>>> this kind of "rare and surprising" case without a big coverage testing?
>>> I'm happy to make a draft patch for it, but not sure how to early catch
>>> those cases which need to be rewritten for those ports that I can't test
>>> on (even with cfarm machines, the coverage seems still limited).
>> So what Richard suggests would be to disallow split conditions
>> that do not start with "&& ", it's probably easy to do that as well
>> and look for build fails.  That should catch all cases to look at.
> Yeah.  As a strawman proposal, how about:
>
> - add a new "define_independent_insn_and_split" that has the
>    current semantics of define_insn_and_split.  This should be
>    mechanical.
>
> - find the define_insn_and_splits that are missing the "&&", and where
>    missing the "&&" might make a difference.  Change them to
>    define_independent_insn_and_splits.
>
>    Like Richard says, this can be done by temporarily disallowing
>    define_insn_and_splits that have no "&&".
>
>    I think this should remain a mechanical step.  If port maintainers
>    think that the missing "&&" is a mistake, they should fix it as
>    a separate patch.
>
> - flip the default for define_insn_and_split so that the "&&" is implicit
>    (but can still be specified redundantly)
>
> Then port maintainers who don't mind the churn can remove the
> redundant "&&"s from the remaining define_insn_and_splits.
That works for me.  If we'd had this in place earlier I wouldn't have 
mucked up the H8 port.
jeff

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Segher Boessenkool]

On Wed, Jun 02, 2021 at 04:18:46PM +0800, Kewen.Lin wrote:
> on 2021/6/2 下午3:43, Richard Biener wrote:
> Yes, the "" in split condition does mean 'true' (always).

Right -- which means it will be split whenever it matches.  This *can*
be intended, but in define_insn_and_split it is almost always a simple
bug.

> > Also "" as split condition _does_
> > seem valid, just maybe unintended?  
> 
> Yes, it's valid without this patch.  That's why I asked whether there is
> some good reason to keep it be [1].  In Segher's opinion, there is no
> good reason, he pointed out "A reader does not expect a
> define_insn_and_split to split any other insns."

Yes, but considering plain define_split, it can be wanted, esp. in
simpler backends that do not have a lot of historical baggage.

> > How would one create a
> > functionally equivalent example? "|| 1" will likely not work.
> 
> I think "|| 1" works just like "" if people want the define_split to
> split all the time, even with this patch.

Except "|| 1" is a syntax error.

You can always write just "1".

> > Note I'm not familiar with all the details here but the documentation
> > does seem ambiguous at best, not supporting to error on empty
> > split-conditions at least.
> 
> Yes, the current patch will stop the "" condition which was accepted
> before.  Thanks for bringing this up!  We have to update the
> documentation if people reach a consensus.

It will help if the error message tells you
  If this is what you intended, write "1".
or similar.  No more documentation is needed then :-)


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
> > So what Richard suggests would be to disallow split conditions
> > that do not start with "&& ", it's probably easy to do that as well
> > and look for build fails.  That should catch all cases to look at.
> 
> Yeah.  As a strawman proposal, how about:
> 
> - add a new "define_independent_insn_and_split" that has the
>   current semantics of define_insn_and_split.  This should be
>   mechanical.

I'd rather not have that -- we can just write separate define_insn and
define_split in that case.

How many such cases *are* there?  There are no users exposed to this,
and when the split condition is required to start with "&&" (instead of
getting that implied) it is not a silent change ever, either.

> - find the define_insn_and_splits that are missing the "&&", and where
>   missing the "&&" might make a difference.  Change them to
>   define_independent_insn_and_splits.
> 
>   Like Richard says, this can be done by temporarily disallowing
>   define_insn_and_splits that have no "&&".

If we make that change permanently, that is all steps we ever need!

Very old backends use the same insn condition and split condition
sometimes still; it isn't hard to detect that as well, if that seems
prudent.


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

Hi Richi/Richard/Jeff/Segher,

Thanks for the comments!

on 2021/6/3 上午7:52, Segher Boessenkool wrote:
> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:
>> Richard Biener <richard.guenther@gmail.com> writes:
>>> So what Richard suggests would be to disallow split conditions
>>> that do not start with "&& ", it's probably easy to do that as well
>>> and look for build fails.  That should catch all cases to look at.
>>
>> Yeah.  As a strawman proposal, how about:
>>
>> - add a new "define_independent_insn_and_split" that has the
>>   current semantics of define_insn_and_split.  This should be
>>   mechanical.
> 
> I'd rather not have that -- we can just write separate define_insn and
> define_split in that case.
> 

Not sure if someone would argue that he/she would like to go with one shared
pattern as before, to avoid any possible differences between two seperated
patterns and have good maintainability (like only editing on place) and
slightly better efficiency.

> How many such cases *are* there?  There are no users exposed to this,
> and when the split condition is required to start with "&&" (instead of
> getting that implied) it is not a silent change ever, either.
> 

If I read the proposal right, the explicit "&&" is only required when going
to find all potential problematic places for final implied "&&" change.
But one explicit "&&" does offer good readability.

>> - find the define_insn_and_splits that are missing the "&&", and where
>>   missing the "&&" might make a difference.  Change them to
>>   define_independent_insn_and_splits.
>>
>>   Like Richard says, this can be done by temporarily disallowing
>>   define_insn_and_splits that have no "&&".
> 
> If we make that change permanently, that is all steps we ever need!
> 

So the question is that: whether we need to demand an explicit "&&".
Richard's proposal is for answer "no" which aligns with Richi's auto
filling advice before.  I think it would result in fewer changes since
those places without explicit "&&" are mostly unintentional, all the jobs
are done by implied "&&".  Its downside seems to be bad readability, new
readers may take it as two seperated conditions at first glance, but I
guess if we emphasize this change in the document it would be fine?
Or emitting one warning if missing an explicit "&&"?

BR,
Kewen
 
> Very old backends use the same insn condition and split condition
> sometimes still; it isn't hard to detect that as well, if that seems
> prudent.
> 
> 
> Segher
> 

Re: [PATCH 04/11] cris: Update unexpected empty split condition

[Kewen.Lin]

Hi Nilsson,

on 2021/6/2 下午8:45, Hans-Peter Nilsson wrote:
>> From: Kewen Lin <linkw@linux.ibm.com>
>> Date: Wed, 2 Jun 2021 07:04:54 +0200
> 
>> gcc/ChangeLog:
>>
>> 	* config/cris/cris.md (*addi_reload): Fix empty split condition.
>> ---
>>  gcc/config/cris/cris.md | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md
>> index 7de0ec63fcf..d5a3c703a83 100644
>> --- a/gcc/config/cris/cris.md
>> +++ b/gcc/config/cris/cris.md
>> @@ -1311,7 +1311,7 @@ (define_insn_and_split "*addi_reload"
>>     && (INTVAL (operands[3]) == 2 || INTVAL (operands[3]) == 4)
>>     && (reload_in_progress || reload_completed)"
>>    "#"
>> -  ""
>> +  "&& 1"
>>    [(set (match_dup 0)
>>  	(plus:SI (ashift:SI (match_dup 2) (match_dup 3)) (match_dup 1)))]
>>    "operands[3] = operands[3] == const2_rtx ? const1_rtx : const2_rtx;")
>> -- 
>> 2.17.1
>>
> 
> Ok, thanks, if only for all-round consistency.
> 
> In preparation for a warning for an empty condition?  I'm
> usually all for .md-warnings, but I'm not sure about the
> benefit of that one, though.  Those "&& 1" look...hackish.

Thanks!  Yeah, the 01/11 patch aims to raise one error message
for the define_insn_and_split whose split condition is empty
while insn condition isn't.  In most cases, when we write one
define_insn_and_split we want the splitting only to take effect
while we see the define_insn matching happen (insn cond holds),
but if we leave the split condition empty, the splitting will
be done always, it could result in some unexpected consequence.
Mostly this is unintentional.  The error message is to avoid
people to make it unintentionally.

As you may have seen from the discussion under the 00/11 thread,
we will probably end up with some other solution, so I will hold
the changes for the ports, sorry for wasting your time and the
other port maintainers'.

BR,
Kewen

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

Hi!

On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
> on 2021/6/3 上午7:52, Segher Boessenkool wrote:
> >> - add a new "define_independent_insn_and_split" that has the
> >>   current semantics of define_insn_and_split.  This should be
> >>   mechanical.
> > 
> > I'd rather not have that -- we can just write separate define_insn and
> > define_split in that case.
> 
> Not sure if someone would argue that he/she would like to go with one shared
> pattern as before, to avoid any possible differences between two seperated
> patterns and have good maintainability (like only editing on place) and
> slightly better efficiency.

You only would do this if you have a different insn condition and split
condition, which is a very important thing to know, it doesn't hurt to
draw attention to it.  The efficiency is exactly the same btw,
define_insn_and_split is just syntactic sugar.

The whole point of requiring the split condition to start with && is so
it will become harder to mess things up (it will make the gen* code a
tiny little bit simpler as well).  And there is no transition period or
anything like that needed either.  Just the bunch that will break will
need fixing.  So let's find out how many of those there are :-)

> > How many such cases *are* there?  There are no users exposed to this,
> > and when the split condition is required to start with "&&" (instead of
> > getting that implied) it is not a silent change ever, either.
> 
> If I read the proposal right, the explicit "&&" is only required when going
> to find all potential problematic places for final implied "&&" change.
> But one explicit "&&" does offer good readability.

My proposal is to always require && (or maybe identical insn and split
conditions should be allowed as well, if people still use that -- that
is how we wrote "&& 1" before that existed).


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Sandiford]

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Richi/Richard/Jeff/Segher,
>
> Thanks for the comments!
>
> on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote:
>> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:
>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>> So what Richard suggests would be to disallow split conditions
>>>> that do not start with "&& ", it's probably easy to do that as well
>>>> and look for build fails.  That should catch all cases to look at.
>>>
>>> Yeah.  As a strawman proposal, how about:
>>>
>>> - add a new "define_independent_insn_and_split" that has the
>>>   current semantics of define_insn_and_split.  This should be
>>>   mechanical.
>> 
>> I'd rather not have that -- we can just write separate define_insn and
>> define_split in that case.
>> 
>
> Not sure if someone would argue that he/she would like to go with one shared
> pattern as before, to avoid any possible differences between two seperated
> patterns and have good maintainability (like only editing on place) and
> slightly better efficiency.

Right.  Plus it creates less make-work.  If we didn't have it, someone
would need to split the define_insn_and_splits that don't currently
use "&&", then later someone might decide that the missing "&&" was a
mistake and need to put them together again (or just revert the patch
and edit from there, I guess).

Plus define_independent_insn_and_split would act as a flag for something
that might be suspect.  If we split them then the define_split condition
will seem to have been chosen deliberately in isolation.

>> How many such cases *are* there?  There are no users exposed to this,
>> and when the split condition is required to start with "&&" (instead of
>> getting that implied) it is not a silent change ever, either.
>> 
>
> If I read the proposal right, the explicit "&&" is only required when going
> to find all potential problematic places for final implied "&&" change.
> But one explicit "&&" does offer good readability.

I don't know.  "&& 1" looks kind of weird to me.

One thing I'd been wondering about a while ago was whether we should key
the split part of define_insn_and_splits off the insn code, instead of
repeating the pattern match and insn C condition.  That would make the
split apply only to the associated define_insns, whereas at the moment
they also apply to any earlier (less general) define_insn that happens
to match the pattern and the C conditions.  It would also reduce the
complexity of the autogenerated define_split logic.

I don't know whether that's a good idea or not.  But having an explicit
"&&" implies that the generator shouldn't do that, and that it should
retest the insn condition from scratch.

>>> - find the define_insn_and_splits that are missing the "&&", and where
>>>   missing the "&&" might make a difference.  Change them to
>>>   define_independent_insn_and_splits.
>>>
>>>   Like Richard says, this can be done by temporarily disallowing
>>>   define_insn_and_splits that have no "&&".
>> 
>> If we make that change permanently, that is all steps we ever need!
>> 
>
> So the question is that: whether we need to demand an explicit "&&".
> Richard's proposal is for answer "no" which aligns with Richi's auto
> filling advice before.  I think it would result in fewer changes since
> those places without explicit "&&" are mostly unintentional, all the jobs
> are done by implied "&&".  Its downside seems to be bad readability, new
> readers may take it as two seperated conditions at first glance, but I
> guess if we emphasize this change in the document it would be fine?
> Or emitting one warning if missing an explicit "&&"?

IMO the natural way to read it is that the split C condition gives the
conditions under which the instruction should be split.  I think that's
why forgetting the "&&" is such a common mistake.  (I know I've done it
plenty of times.)

IMO requiring the "&&" is baking in an alternative, less intuitive,
interpretation.

Thanks,
Richard

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote:
> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
> The whole point of requiring the split condition to start with && is so
> it will become harder to mess things up (it will make the gen* code a
> tiny little bit simpler as well).  And there is no transition period or
> anything like that needed either.  Just the bunch that will break will
> need fixing.  So let's find out how many of those there are :-)
> 
> > > How many such cases *are* there?  There are no users exposed to this,
> > > and when the split condition is required to start with "&&" (instead of
> > > getting that implied) it is not a silent change ever, either.
> > 
> > If I read the proposal right, the explicit "&&" is only required when going
> > to find all potential problematic places for final implied "&&" change.
> > But one explicit "&&" does offer good readability.
> 
> My proposal is to always require && (or maybe identical insn and split
> conditions should be allowed as well, if people still use that -- that
> is how we wrote "&& 1" before that existed).

I prototyped this.  There are very many errors.  Iterators often modify
the insn condition (for one iteration of it), but that does not work if
the split condition does not start with "&&"!

See attached prototype.


Segher

= = =

diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index 2cb760ffb90f..05d46fd3775c 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc)
     case DEFINE_INSN_AND_SPLIT:
     case DEFINE_INSN_AND_REWRITE:
       {
-	const char *split_cond;
 	rtx split;
 	rtvec attr;
 	int i;
@@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc)
 
 	/* If the split condition starts with "&&", append it to the
 	   insn condition to create the new split condition.  */
-	split_cond = XSTR (desc, 4);
-	if (split_cond[0] == '&' && split_cond[1] == '&')
+	const char *insn_cond = XSTR (desc, 2);
+	const char *split_cond = XSTR (desc, 4);
+	if (!strncmp (split_cond, "&&", 2))
 	  {
 	    rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond);
-	    split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2),
+	    split_cond = rtx_reader_ptr->join_c_conditions (insn_cond,
 							    split_cond + 2);
+	  } else if (insn_cond[0]) {
+	    if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
+	      error_at (loc, "the rewrite condition must start with `&&'");
+	    else
+	      error_at (loc, "the split condition must start with `&&' [%s]", insn_cond);
 	  }
-	else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
-	  error_at (loc, "the rewrite condition must start with `&&'");
+
 	XSTR (split, 1) = split_cond;
 	if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
 	  XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

On Thu, Jun 03, 2021 at 09:05:02AM +0100, Richard Sandiford via Gcc-patches wrote:
> Right.  Plus it creates less make-work.  If we didn't have it, someone
> would need to split the define_insn_and_splits that don't currently
> use "&&", then later someone might decide that the missing "&&" was a
> mistake and need to put them together again (or just revert the patch
> and edit from there, I guess).

I would hope no maintainer would be foolish enough to flip-flop like
that.

> Plus define_independent_insn_and_split would act as a flag for something
> that might be suspect.

That always is a conceptual error, even.  The name says so already: the
insn and the split are very different things, with different conditions,
they just happen to share a pattern.

> If we split them then the define_split condition
> will seem to have been chosen deliberately in isolation.

And it will have beenm chosen deliberately!  Why *else* write things
like this?

> > If I read the proposal right, the explicit "&&" is only required when going
> > to find all potential problematic places for final implied "&&" change.
> > But one explicit "&&" does offer good readability.
> 
> I don't know.  "&& 1" looks kind of weird to me.

We have it in rs6000.md since 2004.  sparc has had it since 2002.  i386
has had it since 2001.  arm still does not have it :-)

> > So the question is that: whether we need to demand an explicit "&&".
> > Richard's proposal is for answer "no" which aligns with Richi's auto
> > filling advice before.  I think it would result in fewer changes since
> > those places without explicit "&&" are mostly unintentional, all the jobs
> > are done by implied "&&".  Its downside seems to be bad readability, new
> > readers may take it as two seperated conditions at first glance, but I
> > guess if we emphasize this change in the document it would be fine?
> > Or emitting one warning if missing an explicit "&&"?
> 
> IMO the natural way to read it is that the split C condition gives the
> conditions under which the instruction should be split.  I think that's
> why forgetting the "&&" is such a common mistake.  (I know I've done it
> plenty of times.)

You even do in this description!  :-)  You do not want the define_split
in a define_insn_and_split to happen for insns that do not match the
insn condition, that would not be intuitive at all.

> IMO requiring the "&&" is baking in an alternative, less intuitive,
> interpretation.

But you want the exact same semantics, right?  I do agree that the
syntax without "&&" looks nicer.  It has many practical problems though,
so it is nice to aim for, but we cannot move there until we have all
existing machine descriptions fixed up first.


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Jun 03, 2021 at 09:05:02AM +0100, Richard Sandiford via Gcc-patches wrote:
>> Right.  Plus it creates less make-work.  If we didn't have it, someone
>> would need to split the define_insn_and_splits that don't currently
>> use "&&", then later someone might decide that the missing "&&" was a
>> mistake and need to put them together again (or just revert the patch
>> and edit from there, I guess).
>
> I would hope no maintainer would be foolish enough to flip-flop like
> that.

But the point is that the maintainers wouldn't be flip-flopping.
The move to define_independent_insn_and_split would be a mechnical
change made by someone else.

We shouldn't just add "&&" to all define_insn_and_splits that currently
lack them.  That might well break things.  So to get the perfect result,
someone has to look at each individual define_insn_and_split that currently
lacks a "&&" to see what the effects of adding "&&" would be.

IMO it's not reasonable to ask Kewen to do that for all ports.  So the
process I suggested was a way of mechanically changing existing ports in
a way that would not require input from target maintainers, or extensive
testing on affected targets.  It would keep the behaviour of the port
the same as it is now (whether that's good or bad).  This part of the
process could be scripted.

>> > If I read the proposal right, the explicit "&&" is only required when going
>> > to find all potential problematic places for final implied "&&" change.
>> > But one explicit "&&" does offer good readability.
>> 
>> I don't know.  "&& 1" looks kind of weird to me.
>
> We have it in rs6000.md since 2004.  sparc has had it since 2002.  i386
> has had it since 2001.  arm still does not have it :-)

Sure, this syntax goes back 20 years.  I don't think that answers the
point though.  The question was whether a split condition "&& 1" is
more readable than a condition "", given that "" means "always" in other
contexts.  Given the choice, IMO "" is more readable and "&& 1" looks
weird/inconsistent.

Thanks,
Richard

Re: [PATCH 04/11] cris: Update unexpected empty split condition

[Hans-Peter Nilsson]

> From: Kewen.Lin <linkw@linux.ibm.com>
> Date: Thu, 3 Jun 2021 07:45:57 +0200

> on 2021/6/2 Hans-Peter Nilsson wrote:
> >> From: Kewen Lin <linkw@linux.ibm.com>
> >> Date: Wed, 2 Jun 2021 07:04:54 +0200
> > 
> >> gcc/ChangeLog:
> >>
> >> 	* config/cris/cris.md (*addi_reload): Fix empty split condition.

> >> -  ""
> >> +  "&& 1"

> > Ok, thanks, if only for all-round consistency.
> > 
> > In preparation for a warning for an empty condition?  I'm
> > usually all for .md-warnings, but I'm not sure about the
> > benefit of that one, though.  Those "&& 1" look...hackish.
> 
> Thanks!  Yeah, the 01/11 patch aims to raise one error message
> for the define_insn_and_split whose split condition is empty
> while insn condition isn't.  In most cases, when we write one
> define_insn_and_split we want the splitting only to take effect
> while we see the define_insn matching happen (insn cond holds),
> but if we leave the split condition empty, the splitting will
> be done always, it could result in some unexpected consequence.
> Mostly this is unintentional.

It certainly was in the patch above!

>  The error message is to avoid
> people to make it unintentionally.
> 
> As you may have seen from the discussion under the 00/11 thread,
> we will probably end up with some other solution, so I will hold
> the changes for the ports, sorry for wasting your time and the
> other port maintainers'.

No worries: I certainly don't consider it wasted and I'd
prefer to have the patch above committed sooner than the
conclusion of that discussion.  (If you don't get to it,
I'll do it, after a round of testing.)

If you're considering further target patches to adjust for
eventually changed semantics in the define_insn_and_split
split-condition, then whatever trivial patch to cris.md that
gets the effect of the one you sent is preapproved.

Again, thanks.

brgds, H-P

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Jeff Law]

On 6/3/2021 2:00 AM, Segher Boessenkool wrote:
> Hi!
>
> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
>> on 2021/6/3 上午7:52, Segher Boessenkool wrote:
>>>> - add a new "define_independent_insn_and_split" that has the
>>>>    current semantics of define_insn_and_split.  This should be
>>>>    mechanical.
>>> I'd rather not have that -- we can just write separate define_insn and
>>> define_split in that case.
>> Not sure if someone would argue that he/she would like to go with one shared
>> pattern as before, to avoid any possible differences between two seperated
>> patterns and have good maintainability (like only editing on place) and
>> slightly better efficiency.
> You only would do this if you have a different insn condition and split
> condition, which is a very important thing to know, it doesn't hurt to
> draw attention to it.  The efficiency is exactly the same btw,
> define_insn_and_split is just syntactic sugar.
>
> The whole point of requiring the split condition to start with && is so
> it will become harder to mess things up (it will make the gen* code a
> tiny little bit simpler as well).  And there is no transition period or
> anything like that needed either.  Just the bunch that will break will
> need fixing.  So let's find out how many of those there are :-)
Exactly.   While these empty conditions or those not starting with "&&" 
are technically valid, they're all suspicious from a port correctness 
standpoint, particularly if the main condition is non-empty.

Having made that mistake when converting the H8 away from CC0, I can say 
fairly confidently that if we had this in place a year ago that those 
mistakes would likely have been avoided.  Thankfully the H8 isn't a 
heavily used port and has limped along until I stumbled over the issue a 
week or so ago while polishing some improvements to the port.
Jeff

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

On Thu, Jun 03, 2021 at 11:25:49AM +0100, Richard Sandiford wrote:
> We shouldn't just add "&&" to all define_insn_and_splits that currently
> lack them.

My previous post shows that this *already* is required.

> IMO it's not reasonable to ask Kewen to do that for all ports.  So the
> process I suggested was a way of mechanically changing existing ports in
> a way that would not require input from target maintainers, or extensive
> testing on affected targets.

I fear it will end up as Yet Another unfinished transition this way :-(

> >> I don't know.  "&& 1" looks kind of weird to me.
> >
> > We have it in rs6000.md since 2004.  sparc has had it since 2002.  i386
> > has had it since 2001.  arm still does not have it :-)
> 
> Sure, this syntax goes back 20 years.  I don't think that answers the
> point though.  The question was whether a split condition "&& 1" is
> more readable than a condition "", given that "" means "always" in other
> contexts.  Given the choice, IMO "" is more readable and "&& 1" looks
> weird/inconsistent.

In most ports "&& 1" is all over the place and well-known already.  Sure
we could change to "" meaning always, but that is not what it currently
means!

If we could just start all over we could do it perfectly (but see
second-system syndrome, heh).  But we cannot.  IMO we should especially
avoid everything that uses new semantics for old syntax.


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Jakub Jelinek]

On Thu, Jun 03, 2021 at 04:25:44PM -0500, Segher Boessenkool wrote:
> If we could just start all over we could do it perfectly (but see
> second-system syndrome, heh).  But we cannot.  IMO we should especially
> avoid everything that uses new semantics for old syntax.

Agreed, that would be a nightmare for backporting.

	Jakub

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

On Thu, Jun 03, 2021 at 11:11:53AM -0600, Jeff Law wrote:
> On 6/3/2021 2:00 AM, Segher Boessenkool wrote:
> >The whole point of requiring the split condition to start with && is so
> >it will become harder to mess things up (it will make the gen* code a
> >tiny little bit simpler as well).  And there is no transition period or
> >anything like that needed either.  Just the bunch that will break will
> >need fixing.  So let's find out how many of those there are :-)
> Exactly.   While these empty conditions or those not starting with "&&" 
> are technically valid, they're all suspicious from a port correctness 
> standpoint, particularly if the main condition is non-empty.

And note that this is also the case if you wrote the insn condition as
an empty string, but you used some iterator with a condition.  I found
many of these in rs6000.  This will need to be fixed before we do
anything else.

> Having made that mistake when converting the H8 away from CC0, I can say 
> fairly confidently that if we had this in place a year ago that those 
> mistakes would likely have been avoided.  Thankfully the H8 isn't a 
> heavily used port and has limped along until I stumbled over the issue a 
> week or so ago while polishing some improvements to the port.

I've noticed unexpected splits in rs6000 only a few times over the
years. That doesn't mean more didn't happen, they just didn't cause
obvious enough problems :-)


Segher

Re: [PATCH 04/11] cris: Update unexpected empty split condition

[Hans-Peter Nilsson]

> From: Hans-Peter Nilsson <hp@axis.com>
> CC: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
> Date: Thu, 3 Jun 2021 18:12:25 +0200

> I'd
> prefer to have the patch above committed sooner than the
> conclusion of that discussion.  (If you don't get to it,
> I'll do it, after a round of testing.)

Done; no regressions.

brgds, H-P

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

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

Hi Segher,

on 2021/6/3 下午5:18, Segher Boessenkool wrote:
> On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote:
>> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
>> The whole point of requiring the split condition to start with && is so
>> it will become harder to mess things up (it will make the gen* code a
>> tiny little bit simpler as well).  And there is no transition period or
>> anything like that needed either.  Just the bunch that will break will
>> need fixing.  So let's find out how many of those there are :-)
>>

To find out those need fixing seems to be the critical part.  It's
not hard to add one explicit "&&" to those that don't have it now, but
even with further bootstrapped and regression tested I'm still not
confident the adjustments are safe enough, since the testing coverage
could be limited.  It may need more efforts to revisit, or/and test
with more coverages, and port maintainers' reviews.

In order to find one example which needs more fixing, for rs6000/i386/
aarch64, I fixed all define_insn_and_splits whose insn cond isn't empty
(from gensupport's view since the iterator can add more on) while split
cond don't start with "&&" , also skipped those whose insn conds are
the same as their split conds.  Unfortunately (or fortunately :-\) all
were bootstrapped and regress-tested.

The related diffs are attached, which is based on r12-0.

>>>> How many such cases *are* there?  There are no users exposed to this,
>>>> and when the split condition is required to start with "&&" (instead of
>>>> getting that implied) it is not a silent change ever, either.
>>>
>>> If I read the proposal right, the explicit "&&" is only required when going
>>> to find all potential problematic places for final implied "&&" change.
>>> But one explicit "&&" does offer good readability.
>>
>> My proposal is to always require && (or maybe identical insn and split
>> conditions should be allowed as well, if people still use that -- that
>> is how we wrote "&& 1" before that existed).
> 
> I prototyped this.  There are very many errors.  Iterators often modify
> the insn condition (for one iteration of it), but that does not work if
> the split condition does not start with "&&"!
> 
> See attached prototype.
> 
> 

Thanks for the prototype!

BR,
Kewen

> Segher
> 
> = = =
> 
> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> index 2cb760ffb90f..05d46fd3775c 100644
> --- a/gcc/gensupport.c
> +++ b/gcc/gensupport.c
> @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc)
>      case DEFINE_INSN_AND_SPLIT:
>      case DEFINE_INSN_AND_REWRITE:
>        {
> -	const char *split_cond;
>  	rtx split;
>  	rtvec attr;
>  	int i;
> @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc)
>  
>  	/* If the split condition starts with "&&", append it to the
>  	   insn condition to create the new split condition.  */
> -	split_cond = XSTR (desc, 4);
> -	if (split_cond[0] == '&' && split_cond[1] == '&')
> +	const char *insn_cond = XSTR (desc, 2);
> +	const char *split_cond = XSTR (desc, 4);
> +	if (!strncmp (split_cond, "&&", 2))
>  	  {
>  	    rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond);
> -	    split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2),
> +	    split_cond = rtx_reader_ptr->join_c_conditions (insn_cond,
>  							    split_cond + 2);
> +	  } else if (insn_cond[0]) {
> +	    if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> +	      error_at (loc, "the rewrite condition must start with `&&'");
> +	    else
> +	      error_at (loc, "the split condition must start with `&&' [%s]", insn_cond);
>  	  }
> -	else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> -	  error_at (loc, "the rewrite condition must start with `&&'");
> +
>  	XSTR (split, 1) = split_cond;
>  	if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>  	  XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
> 

[-- Attachment #2: aarch64.diff --]
[-- Type: text/plain, Size: 1063 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index abfd845..86869d9 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1283,7 +1283,7 @@ (define_insn_and_split "*movsi_aarch64"
    fmov\\t%w0, %s1
    fmov\\t%s0, %s1
    * return aarch64_output_scalar_simd_mov_immediate (operands[1], SImode);"
-  "CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode)
+  "&& CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode)
     && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
    [(const_int 0)]
    "{
@@ -1319,7 +1319,7 @@ (define_insn_and_split "*movdi_aarch64"
    fmov\\t%x0, %d1
    fmov\\t%d0, %d1
    * return aarch64_output_scalar_simd_mov_immediate (operands[1], DImode);"
-   "(CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode))
+   "&& (CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode))
     && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
    [(const_int 0)]
    "{

[-- Attachment #3: i386.diff --]
[-- Type: text/plain, Size: 11811 bytes --]

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 9ff35d9..8d72f34 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -5249,7 +5249,7 @@ (define_insn_and_split "*add<dwi>3_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -6050,7 +6050,7 @@ (define_insn_and_split "*addv<dwi>4_doubleword"
 	(plus:<DWI> (match_dup 1) (match_dup 2)))]
   "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -6097,7 +6097,7 @@ (define_insn_and_split "*addv<dwi>4_doubleword_1"
    && CONST_SCALAR_INT_P (operands[2])
    && rtx_equal_p (operands[2], operands[3])"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -6391,7 +6391,7 @@ (define_insn_and_split "*sub<dwi>3_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CC FLAGS_REG)
 		   (compare:CC (match_dup 1) (match_dup 2)))
 	      (set (match_dup 0)
@@ -6559,7 +6559,7 @@ (define_insn_and_split "*subv<dwi>4_doubleword"
 	(minus:<DWI> (match_dup 1) (match_dup 2)))]
   "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CC FLAGS_REG)
 		   (compare:CC (match_dup 1) (match_dup 2)))
 	      (set (match_dup 0)
@@ -6604,7 +6604,7 @@ (define_insn_and_split "*subv<dwi>4_doubleword_1"
    && CONST_SCALAR_INT_P (operands[2])
    && rtx_equal_p (operands[2], operands[3])"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CC FLAGS_REG)
 		   (compare:CC (match_dup 1) (match_dup 2)))
 	      (set (match_dup 0)
@@ -7204,7 +7204,7 @@ (define_insn_and_split "*add<dwi>3_doubleword_cc_overflow_1"
 	(plus:<DWI> (match_dup 1) (match_dup 2)))]
   "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (reg:CCC FLAGS_REG)
 		   (compare:CCC
 		     (plus:DWIH (match_dup 1) (match_dup 2))
@@ -8161,7 +8161,7 @@ (define_insn_and_split "divmod<mode>4_1"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (match_dup 1)
 		   (ashiftrt:SWI48 (match_dup 4) (match_dup 5)))
 	      (clobber (reg:CC FLAGS_REG))])
@@ -8196,7 +8196,7 @@ (define_insn_and_split "udivmod<mode>4_1"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(set (match_dup 1) (const_int 0))
    (parallel [(set (match_dup 0)
 		   (udiv:SWI48 (match_dup 2) (match_dup 3)))
@@ -8336,7 +8336,7 @@ (define_insn_and_split "*divmod<mode>4"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel [(set (match_dup 1)
 		   (ashiftrt:SWIM248 (match_dup 4) (match_dup 5)))
 	      (clobber (reg:CC FLAGS_REG))])
@@ -8371,7 +8371,7 @@ (define_insn_and_split "*udivmod<mode>4"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(set (match_dup 1) (const_int 0))
    (parallel [(set (match_dup 0)
 		   (udiv:SWIM248 (match_dup 2) (match_dup 3)))
@@ -10069,7 +10069,7 @@ (define_insn_and_split "*neg<dwi>2_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   "ix86_unary_operator_ok (NEG, <DWI>mode, operands)"
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(parallel
     [(set (reg:CCC FLAGS_REG)
 	  (ne:CCC (match_dup 1) (const_int 0)))
@@ -11545,7 +11545,7 @@ (define_insn_and_split "*<insn><mode>3_doubleword"
    (clobber (reg:CC FLAGS_REG))]
   ""
   "#"
-  "epilogue_completed"
+  "&& epilogue_completed"
   [(const_int 0)]
   "ix86_split_<insn> (operands, NULL_RTX, <MODE>mode); DONE;"
   [(set_attr "type" "multi")])
@@ -12045,7 +12045,7 @@ (define_insn_and_split "ix86_rotl<dwi>3_doubleword"
   (clobber (match_scratch:DWIH 3 "=&r"))]
  ""
  "#"
- "reload_completed"
+ "&& reload_completed"
  [(set (match_dup 3) (match_dup 4))
   (parallel
    [(set (match_dup 4)
@@ -12073,7 +12073,7 @@ (define_insn_and_split "ix86_rotr<dwi>3_doubleword"
   (clobber (match_scratch:DWIH 3 "=&r"))]
  ""
  "#"
- "reload_completed"
+ "&& reload_completed"
  [(set (match_dup 3) (match_dup 4))
   (parallel
    [(set (match_dup 4)
@@ -14308,7 +14308,7 @@ (define_insn_and_split "ctz<mode>2"
 
   return "bsf{<imodesuffix>}\t{%1, %0|%0, %1}";
 }
-  "(TARGET_BMI || TARGET_GENERIC)
+  "&& (TARGET_BMI || TARGET_GENERIC)
    && TARGET_AVOID_FALSE_DEP_FOR_BMI && epilogue_completed
    && optimize_function_for_speed_p (cfun)
    && !reg_mentioned_p (operands[0], operands[1])"
@@ -15712,7 +15712,7 @@ (define_insn_and_split "*load_tp_x32_zext"
 	  (unspec:SI [(const_int 0)] UNSPEC_TP)))]
   "TARGET_X32"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(zero_extend:DI (match_dup 1)))]
 {
@@ -15750,7 +15750,7 @@ (define_insn_and_split "*add_tp_x32_zext"
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_X32"
   "#"
-  ""
+  "&& 1"
   [(parallel
      [(set (match_dup 0)
      	   (zero_extend:DI
@@ -15841,7 +15841,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_32"
    (clobber (reg:CC FLAGS_REG))]
   "!TARGET_64BIT && TARGET_GNU2_TLS"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (match_dup 5))]
 {
   operands[5] = can_create_pseudo_p () ? gen_reg_rtx (Pmode) : operands[0];
@@ -15900,7 +15900,7 @@ (define_insn_and_split "*tls_dynamic_gnu2_combine_64_<mode>"
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_64BIT && TARGET_GNU2_TLS"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0) (match_dup 4))]
 {
   operands[4] = can_create_pseudo_p () ? gen_reg_rtx (ptr_mode) : operands[0];
diff --git a/gcc/config/i386/mmx.md b/gcc/config/i386/mmx.md
index 4c2b724..e6737b1 100644
--- a/gcc/config/i386/mmx.md
+++ b/gcc/config/i386/mmx.md
@@ -1668,7 +1668,7 @@ (define_insn_and_split "mmx_pack<s_trunsuffix>swb"
    pack<s_trunsuffix>swb\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_pack (operands, <any_s_truncate:CODE>); DONE;"
@@ -1688,7 +1688,7 @@ (define_insn_and_split "mmx_packssdw"
    packssdw\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_pack (operands, SS_TRUNCATE); DONE;"
@@ -1711,7 +1711,7 @@ (define_insn_and_split "mmx_punpckhbw"
    punpckhbw\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, true); DONE;"
@@ -1734,7 +1734,7 @@ (define_insn_and_split "mmx_punpcklbw"
    punpcklbw\t{%2, %0|%0, %k2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, false); DONE;"
@@ -1755,7 +1755,7 @@ (define_insn_and_split "mmx_punpckhwd"
    punpckhwd\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, true); DONE;"
@@ -1776,7 +1776,7 @@ (define_insn_and_split "mmx_punpcklwd"
    punpcklwd\t{%2, %0|%0, %k2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, false); DONE;"
@@ -1797,7 +1797,7 @@ (define_insn_and_split "mmx_punpckhdq"
    punpckhdq\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, true); DONE;"
@@ -1818,7 +1818,7 @@ (define_insn_and_split "mmx_punpckldq"
    punpckldq\t{%2, %0|%0, %k2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
   "ix86_split_mmx_punpck (operands, false); DONE;"
@@ -2542,7 +2542,7 @@ (define_insn_and_split "mmx_pmovmskb"
   "@
    pmovmskb\t{%1, %0|%0, %1}
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REGNO_P (REGNO (operands[1]))"
   [(set (match_dup 0)
         (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))
diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
index 9d3728d..9919cc0 100644
--- a/gcc/config/i386/sse.md
+++ b/gcc/config/i386/sse.md
@@ -5223,7 +5223,7 @@ (define_insn_and_split "sse_cvtpi2ps"
    cvtpi2ps\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REG_P (operands[2])"
   [(const_int 0)]
 {
@@ -5278,7 +5278,7 @@ (define_insn_and_split "sse_cvtps2pi"
   "@
    cvtps2pi\t{%1, %0|%0, %q1}
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REG_P (operands[0])"
   [(const_int 0)]
 {
@@ -5310,7 +5310,7 @@ (define_insn_and_split "sse_cvttps2pi"
   "@
    cvttps2pi\t{%1, %0|%0, %q1}
    #"
-  "TARGET_SSE2 && reload_completed
+  "&& TARGET_SSE2 && reload_completed
    && SSE_REG_P (operands[0])"
   [(const_int 0)]
 {
@@ -16467,7 +16467,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_lt"
 	  UNSPEC_MOVMSK))]
   "TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(unspec:SI [(match_dup 1)] UNSPEC_MOVMSK))]
   ""
@@ -16489,7 +16489,7 @@ (define_insn_and_split "*<sse2_avx2>_pmovmskb_zext_lt"
 	    UNSPEC_MOVMSK)))]
   "TARGET_64BIT && TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(zero_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
   ""
@@ -16511,7 +16511,7 @@ (define_insn_and_split "*sse2_pmovmskb_ext_lt"
 	    UNSPEC_MOVMSK)))]
   "TARGET_64BIT && TARGET_SSE2"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(sign_extend:DI (unspec:SI [(match_dup 1)] UNSPEC_MOVMSK)))]
   ""
@@ -16692,7 +16692,7 @@ (define_insn_and_split "ssse3_ph<plusminus_mnemonic>wv4hi3"
    ph<plusminus_mnemonic>w\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
 {
@@ -16777,7 +16777,7 @@ (define_insn_and_split "ssse3_ph<plusminus_mnemonic>dv2si3"
    ph<plusminus_mnemonic>d\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(const_int 0)]
 {
@@ -17185,7 +17185,7 @@ (define_insn_and_split "*ssse3_pshufbv8qi3"
    pshufb\t{%2, %0|%0, %2}
    #
    #"
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(set (match_dup 3)
 	(and:V4SI (match_dup 3) (match_dup 2)))
@@ -17315,7 +17315,7 @@ (define_insn_and_split "ssse3_palignrdi"
       gcc_unreachable ();
     }
 }
-  "TARGET_SSSE3 && reload_completed
+  "&& TARGET_SSSE3 && reload_completed
    && SSE_REGNO_P (REGNO (operands[0]))"
   [(set (match_dup 0)
 	(lshiftrt:V1TI (match_dup 0) (match_dup 3)))]
@@ -17769,7 +17769,7 @@ (define_insn_and_split "*<sse4_1_avx2>_pblendvb_lt"
 	  UNSPEC_BLENDV))]
   "TARGET_SSE4_1"
   "#"
-  ""
+  "&& 1"
   [(set (match_dup 0)
 	(unspec:VI1_AVX2
 	 [(match_dup 1) (match_dup 2) (match_dup 3)] UNSPEC_BLENDV))]

[-- Attachment #4: rs6000.diff --]
[-- Type: text/plain, Size: 4522 bytes --]

diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 3f59b544f6a..feeeaffcc35 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -6752,7 +6752,7 @@ (define_insn_and_split "*and<mode>3_internal"
 
   return "#";
 }
-  "reload_completed && int_reg_operand (operands[0], <MODE>mode)"
+  "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, AND, false, false, false);
@@ -6788,7 +6788,7 @@ (define_insn_and_split "*bool<mode>3_internal"
 
   return "#";
 }
-  "reload_completed && int_reg_operand (operands[0], <MODE>mode)"
+  "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, GET_CODE (operands[3]), false, false, false);
@@ -6825,7 +6825,7 @@ (define_insn_and_split "*boolc<mode>3_internal1"
 
   return "#";
 }
-  "(TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
+  "&& (TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
    && reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
@@ -6854,7 +6854,7 @@ (define_insn_and_split "*boolc<mode>3_internal2"
 	  (match_operand:TI2 1 "int_reg_operand" "r,r,0")]))]
   "!TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   "#"
-  "reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
+  "&& reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, GET_CODE (operands[3]), false, false, true);
@@ -6885,7 +6885,7 @@ (define_insn_and_split "*boolcc<mode>3_internal1"
 
   return "#";
 }
-  "(TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
+  "&& (TARGET_P8_VECTOR || (GET_CODE (operands[3]) == AND))
    && reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
@@ -6915,7 +6915,7 @@ (define_insn_and_split "*boolcc<mode>3_internal2"
 	   (match_operand:TI2 2 "int_reg_operand" "r,r,0"))]))]
   "!TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   "#"
-  "reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
+  "&& reload_completed && !TARGET_P8_VECTOR && (GET_CODE (operands[3]) != AND)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, GET_CODE (operands[3]), false, true, true);
@@ -6943,7 +6943,7 @@ (define_insn_and_split "*eqv<mode>3_internal1"
 
   return "#";
 }
-  "TARGET_P8_VECTOR && reload_completed
+  "&& TARGET_P8_VECTOR && reload_completed
    && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
@@ -6972,7 +6972,7 @@ (define_insn_and_split "*eqv<mode>3_internal2"
 	  (match_operand:TI2 2 "int_reg_operand" "r,r,0"))))]
   "!TARGET_P8_VECTOR"
   "#"
-  "reload_completed && !TARGET_P8_VECTOR"
+  "&& reload_completed && !TARGET_P8_VECTOR"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, XOR, true, false, false);
@@ -7000,7 +7000,7 @@ (define_insn_and_split "one_cmpl<mode>2"
 
   return "#";
 }
-  "reload_completed && int_reg_operand (operands[0], <MODE>mode)"
+  "&& reload_completed && int_reg_operand (operands[0], <MODE>mode)"
   [(const_int 0)]
 {
   rs6000_split_logical (operands, NOT, false, false, false);
@@ -13532,7 +13532,7 @@ (define_insn_and_split "@eh_set_lr_<mode>"
    (clobber (match_scratch:P 1 "=&b"))]
   ""
   "#"
-  "reload_completed"
+  "&& reload_completed"
   [(const_int 0)]
 {
   rs6000_emit_eh_reg_restore (operands[0], operands[1]);
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index bcb92be2f5c..2e0abb27354 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -1630,7 +1630,7 @@ (define_insn_and_split "vsx_mul_v2di"
                      UNSPEC_VSX_MULSD))]
   "VECTOR_MEM_VSX_P (V2DImode)"
   "#"
-  "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
+  "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
   [(const_int 0)]
 {
   rtx op0 = operands[0];
@@ -1685,7 +1685,7 @@ (define_insn_and_split "vsx_div_v2di"
                      UNSPEC_VSX_DIVSD))]
   "VECTOR_MEM_VSX_P (V2DImode)"
   "#"
-  "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
+  "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
   [(const_int 0)]
 {
   rtx op0 = operands[0];
@@ -1732,7 +1732,7 @@ (define_insn_and_split "vsx_udiv_v2di"
                      UNSPEC_VSX_DIVUD))]
   "VECTOR_MEM_VSX_P (V2DImode)"
   "#"
-  "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
+  "&& VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
   [(const_int 0)]
 {
   rtx op0 = operands[0];

Re: [PATCH 04/11] cris: Update unexpected empty split condition

[Kewen.Lin]

on 2021/6/4 上午12:12, Hans-Peter Nilsson wrote:
>> From: Kewen.Lin <linkw@linux.ibm.com>
>> Date: Thu, 3 Jun 2021 07:45:57 +0200
> 
>> on 2021/6/2 Hans-Peter Nilsson wrote:
>>>> From: Kewen Lin <linkw@linux.ibm.com>
>>>> Date: Wed, 2 Jun 2021 07:04:54 +0200
>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 	* config/cris/cris.md (*addi_reload): Fix empty split condition.
> 
>>>> -  ""
>>>> +  "&& 1"
> 
>>> Ok, thanks, if only for all-round consistency.
>>>
>>> In preparation for a warning for an empty condition?  I'm
>>> usually all for .md-warnings, but I'm not sure about the
>>> benefit of that one, though.  Those "&& 1" look...hackish.
>>
>> Thanks!  Yeah, the 01/11 patch aims to raise one error message
>> for the define_insn_and_split whose split condition is empty
>> while insn condition isn't.  In most cases, when we write one
>> define_insn_and_split we want the splitting only to take effect
>> while we see the define_insn matching happen (insn cond holds),
>> but if we leave the split condition empty, the splitting will
>> be done always, it could result in some unexpected consequence.
>> Mostly this is unintentional.
> 
> It certainly was in the patch above!
> 
>>  The error message is to avoid
>> people to make it unintentionally.
>>
>> As you may have seen from the discussion under the 00/11 thread,
>> we will probably end up with some other solution, so I will hold
>> the changes for the ports, sorry for wasting your time and the
>> other port maintainers'.
> 
> No worries: I certainly don't consider it wasted and I'd
> prefer to have the patch above committed sooner than the
> conclusion of that discussion.  (If you don't get to it,
> I'll do it, after a round of testing.)
> 

Thanks for your help on testing!

> If you're considering further target patches to adjust for
> eventually changed semantics in the define_insn_and_split
> split-condition, then whatever trivial patch to cris.md that
> gets the effect of the one you sent is preapproved.
> 

OK, thanks again!

BR,
Kewen

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

on 2021/6/3 下午4:05, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi Richi/Richard/Jeff/Segher,
>>
>> Thanks for the comments!
>>
>> on 2021/6/3 锟斤拷锟斤拷7:52, Segher Boessenkool wrote:
>>> On Wed, Jun 02, 2021 at 06:32:13PM +0100, Richard Sandiford wrote:
>>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>>> So what Richard suggests would be to disallow split conditions
>>>>> that do not start with "&& ", it's probably easy to do that as well
>>>>> and look for build fails.  That should catch all cases to look at.
>>>>
>>>> Yeah.  As a strawman proposal, how about:
>>>>
>>>> - add a new "define_independent_insn_and_split" that has the
>>>>   current semantics of define_insn_and_split.  This should be
>>>>   mechanical.
>>>
>>> I'd rather not have that -- we can just write separate define_insn and
>>> define_split in that case.
>>>
>>
>> Not sure if someone would argue that he/she would like to go with one shared
>> pattern as before, to avoid any possible differences between two seperated
>> patterns and have good maintainability (like only editing on place) and
>> slightly better efficiency.
> 
> Right.  Plus it creates less make-work.  If we didn't have it, someone
> would need to split the define_insn_and_splits that don't currently
> use "&&", then later someone might decide that the missing "&&" was a
> mistake and need to put them together again (or just revert the patch
> and edit from there, I guess).
> 
> Plus define_independent_insn_and_split would act as a flag for something
> that might be suspect.  If we split them then the define_split condition
> will seem to have been chosen deliberately in isolation.
> 
>>> How many such cases *are* there?  There are no users exposed to this,
>>> and when the split condition is required to start with "&&" (instead of
>>> getting that implied) it is not a silent change ever, either.
>>>
>>
>> If I read the proposal right, the explicit "&&" is only required when going
>> to find all potential problematic places for final implied "&&" change.
>> But one explicit "&&" does offer good readability.
> 
> I don't know.  "&& 1" looks kind of weird to me.
> 
> One thing I'd been wondering about a while ago was whether we should key
> the split part of define_insn_and_splits off the insn code, instead of
> repeating the pattern match and insn C condition.  That would make the
> split apply only to the associated define_insns, whereas at the moment
> they also apply to any earlier (less general) define_insn that happens
> to match the pattern and the C conditions.  It would also reduce the
> complexity of the autogenerated define_split logic.
> 
> I don't know whether that's a good idea or not.  But having an explicit
> "&&" implies that the generator shouldn't do that, and that it should
> retest the insn condition from scratch.
> 
>>>> - find the define_insn_and_splits that are missing the "&&", and where
>>>>   missing the "&&" might make a difference.  Change them to
>>>>   define_independent_insn_and_splits.
>>>>
>>>>   Like Richard says, this can be done by temporarily disallowing
>>>>   define_insn_and_splits that have no "&&".
>>>
>>> If we make that change permanently, that is all steps we ever need!
>>>
>>
>> So the question is that: whether we need to demand an explicit "&&".
>> Richard's proposal is for answer "no" which aligns with Richi's auto
>> filling advice before.  I think it would result in fewer changes since
>> those places without explicit "&&" are mostly unintentional, all the jobs
>> are done by implied "&&".  Its downside seems to be bad readability, new
>> readers may take it as two seperated conditions at first glance, but I
>> guess if we emphasize this change in the document it would be fine?
>> Or emitting one warning if missing an explicit "&&"?
> 
> IMO the natural way to read it is that the split C condition gives the
> conditions under which the instruction should be split.  I think that's
> why forgetting the "&&" is such a common mistake.  (I know I've done it
> plenty of times.)
> 
> IMO requiring the "&&" is baking in an alternative, less intuitive,
> interpretation.
> 

Thanks for the explanation, I was thinking people may have got used to
the starting "&&" in split condition, so it's easy for them to read.
But I agree it's better not to have it in the natural way.

BR,
Kewen

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Martin Sebor]

On 6/1/21 11:04 PM, Kewen Lin via Gcc-patches wrote:
> As Segher suggested, this patch is to emit the error message
> if the split condition of define_insn_and_split is empty while
> the insn condition isn't.
> 
> gcc/ChangeLog:
> 
> 	* gensupport.c (process_rtx): Emit error message for empty
> 	split condition in define_insn_and_split while the insn
> 	condition isn't.
> ---
>   gcc/gensupport.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> index 0f19bd70664..52cee120215 100644
> --- a/gcc/gensupport.c
> +++ b/gcc/gensupport.c
> @@ -620,6 +620,9 @@ process_rtx (rtx desc, file_location loc)
>   	  }
>   	else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>   	  error_at (loc, "the rewrite condition must start with `&&'");
> +	else if (split_cond[0] == '\0' && strlen (XSTR (desc, 2)) != 0)
> +	  error_at (loc, "the split condition mustn't be empty if the "
> +			 "insn condition isn't empty");

The "mustn't" (and other similar contractions) should trigger
-Wdiag-format that GCC should be free of, or was not too long ago.
Can you please spell them out (the suggested alternative spelling
should be mentined in the warning)?

Also, "insn" is not a word, and even though it's common abbreviation
in GCC speak it's not necessarily something all users are familiar
with, and doesn't lend itself to translation.  Please spell out
the word instead.

Thanks
Martin

>   	XSTR (split, 1) = split_cond;
>   	if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>   	  XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
> 

Re: [PATCH 01/11] gen: Emit error msg for empty split condition

[Segher Boessenkool]

On Fri, Jun 04, 2021 at 01:03:34PM -0600, Martin Sebor wrote:
> Also, "insn" is not a word, and even though it's common abbreviation
> in GCC speak it's not necessarily something all users are familiar
> with, and doesn't lend itself to translation.  Please spell out
> the word instead.

This is a message for GCC developers, and it is not translated.


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Biener]

On Fri, Jun 4, 2021 at 4:58 AM Kewen.Lin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi Segher,
>
> on 2021/6/3 下午5:18, Segher Boessenkool wrote:
> > On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote:
> >> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
> >> The whole point of requiring the split condition to start with && is so
> >> it will become harder to mess things up (it will make the gen* code a
> >> tiny little bit simpler as well).  And there is no transition period or
> >> anything like that needed either.  Just the bunch that will break will
> >> need fixing.  So let's find out how many of those there are :-)
> >>
>
> To find out those need fixing seems to be the critical part.  It's
> not hard to add one explicit "&&" to those that don't have it now, but
> even with further bootstrapped and regression tested I'm still not
> confident the adjustments are safe enough, since the testing coverage
> could be limited.  It may need more efforts to revisit, or/and test
> with more coverages, and port maintainers' reviews.
>
> In order to find one example which needs more fixing, for rs6000/i386/
> aarch64, I fixed all define_insn_and_splits whose insn cond isn't empty
> (from gensupport's view since the iterator can add more on) while split
> cond don't start with "&&" , also skipped those whose insn conds are
> the same as their split conds.  Unfortunately (or fortunately :-\) all
> were bootstrapped and regress-tested.
>
> The related diffs are attached, which is based on r12-0.

For preserving existing behavior with requiring "&& " we can (mechanically?)
split define_insn_and_split into define_insn and define_split with the old
condition, right?

That is, all empty insn condition cases are of course obvious to fix.

> >>>> How many such cases *are* there?  There are no users exposed to this,
> >>>> and when the split condition is required to start with "&&" (instead of
> >>>> getting that implied) it is not a silent change ever, either.
> >>>
> >>> If I read the proposal right, the explicit "&&" is only required when going
> >>> to find all potential problematic places for final implied "&&" change.
> >>> But one explicit "&&" does offer good readability.
> >>
> >> My proposal is to always require && (or maybe identical insn and split
> >> conditions should be allowed as well, if people still use that -- that
> >> is how we wrote "&& 1" before that existed).
> >
> > I prototyped this.  There are very many errors.  Iterators often modify
> > the insn condition (for one iteration of it), but that does not work if
> > the split condition does not start with "&&"!
> >
> > See attached prototype.
> >
> >
>
> Thanks for the prototype!
>
> BR,
> Kewen
>
> > Segher
> >
> > = = =
> >
> > diff --git a/gcc/gensupport.c b/gcc/gensupport.c
> > index 2cb760ffb90f..05d46fd3775c 100644
> > --- a/gcc/gensupport.c
> > +++ b/gcc/gensupport.c
> > @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc)
> >      case DEFINE_INSN_AND_SPLIT:
> >      case DEFINE_INSN_AND_REWRITE:
> >        {
> > -     const char *split_cond;
> >       rtx split;
> >       rtvec attr;
> >       int i;
> > @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc)
> >
> >       /* If the split condition starts with "&&", append it to the
> >          insn condition to create the new split condition.  */
> > -     split_cond = XSTR (desc, 4);
> > -     if (split_cond[0] == '&' && split_cond[1] == '&')
> > +     const char *insn_cond = XSTR (desc, 2);
> > +     const char *split_cond = XSTR (desc, 4);
> > +     if (!strncmp (split_cond, "&&", 2))
> >         {
> >           rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond);
> > -         split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2),
> > +         split_cond = rtx_reader_ptr->join_c_conditions (insn_cond,
> >                                                           split_cond + 2);
> > +       } else if (insn_cond[0]) {
> > +         if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> > +           error_at (loc, "the rewrite condition must start with `&&'");
> > +         else
> > +           error_at (loc, "the split condition must start with `&&' [%s]", insn_cond);
> >         }
> > -     else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> > -       error_at (loc, "the rewrite condition must start with `&&'");
> > +
> >       XSTR (split, 1) = split_cond;
> >       if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
> >         XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
> >

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

Hi!

On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:
> To find out those need fixing seems to be the critical part.  It's
> not hard to add one explicit "&&" to those that don't have it now, but
> even with further bootstrapped and regression tested I'm still not
> confident the adjustments are safe enough, since the testing coverage
> could be limited.  It may need more efforts to revisit, or/and test
> with more coverages, and port maintainers' reviews.

https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html

This adds an "&&" everywhere (or in fact, it just skips any existing
one, it just has the same effect of adding it everywhere).  I tested it
with building gcc and Linux for all supported targets (31 of them; I do
some with multiple configs, mostly 32-bit and 64-bit).  None had any
difference before and after the change.

So I am no longer worried that there will be any fallout from doing
this.  There are many things that *could* go wrong, but I don't think
there will be enough at all to be an impediment to just throwing the
switch.

If we go this way no target will need any significant fixing, maybe none
at all will be needed across all targets.  And no changes will be needed
anywhere immediately.  We could make leading "&&" deprecated, and the
same for split condition "1" (which was "&& 1").  This is easy to change
automatically as well.


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

on 2021/6/7 下午3:12, Richard Biener wrote:
> On Fri, Jun 4, 2021 at 4:58 AM Kewen.Lin via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi Segher,
>>
>> on 2021/6/3 下午5:18, Segher Boessenkool wrote:
>>> On Thu, Jun 03, 2021 at 03:00:44AM -0500, Segher Boessenkool wrote:
>>>> On Thu, Jun 03, 2021 at 01:22:38PM +0800, Kewen.Lin wrote:
>>>> The whole point of requiring the split condition to start with && is so
>>>> it will become harder to mess things up (it will make the gen* code a
>>>> tiny little bit simpler as well).  And there is no transition period or
>>>> anything like that needed either.  Just the bunch that will break will
>>>> need fixing.  So let's find out how many of those there are :-)
>>>>
>>
>> To find out those need fixing seems to be the critical part.  It's
>> not hard to add one explicit "&&" to those that don't have it now, but
>> even with further bootstrapped and regression tested I'm still not
>> confident the adjustments are safe enough, since the testing coverage
>> could be limited.  It may need more efforts to revisit, or/and test
>> with more coverages, and port maintainers' reviews.
>>
>> In order to find one example which needs more fixing, for rs6000/i386/
>> aarch64, I fixed all define_insn_and_splits whose insn cond isn't empty
>> (from gensupport's view since the iterator can add more on) while split
>> cond don't start with "&&" , also skipped those whose insn conds are
>> the same as their split conds.  Unfortunately (or fortunately :-\) all
>> were bootstrapped and regress-tested.
>>
>> The related diffs are attached, which is based on r12-0.
> 
> For preserving existing behavior with requiring "&& " we can (mechanically?)
> split define_insn_and_split into define_insn and define_split with the old
> condition, right?
> 

Yeah, we can replace all possible "unsafe" ones with separated define_insn
and define_split.

> That is, all empty insn condition cases are of course obvious to fix.
> 

Sorry for the confusion, those ones are false positive.  Mode iterators
can append more conditions onto empty insn condition to make it not empty,
I noticed it happened for split condition at the same time.  The previous
checking is very rough and didn't care about them as they are not harmful
for the testing.  My latest local patch has excluded them by checking the
beginning and the end, also for the variant with one more external "()".

BR,
Kewen

>>>>>> How many such cases *are* there?  There are no users exposed to this,
>>>>>> and when the split condition is required to start with "&&" (instead of
>>>>>> getting that implied) it is not a silent change ever, either.
>>>>>
>>>>> If I read the proposal right, the explicit "&&" is only required when going
>>>>> to find all potential problematic places for final implied "&&" change.
>>>>> But one explicit "&&" does offer good readability.
>>>>
>>>> My proposal is to always require && (or maybe identical insn and split
>>>> conditions should be allowed as well, if people still use that -- that
>>>> is how we wrote "&& 1" before that existed).
>>>
>>> I prototyped this.  There are very many errors.  Iterators often modify
>>> the insn condition (for one iteration of it), but that does not work if
>>> the split condition does not start with "&&"!
>>>
>>> See attached prototype.
>>>
>>>
>>
>> Thanks for the prototype!
>>
>> BR,
>> Kewen
>>
>>> Segher
>>>
>>> = = =
>>>
>>> diff --git a/gcc/gensupport.c b/gcc/gensupport.c
>>> index 2cb760ffb90f..05d46fd3775c 100644
>>> --- a/gcc/gensupport.c
>>> +++ b/gcc/gensupport.c
>>> @@ -590,7 +590,6 @@ process_rtx (rtx desc, file_location loc)
>>>      case DEFINE_INSN_AND_SPLIT:
>>>      case DEFINE_INSN_AND_REWRITE:
>>>        {
>>> -     const char *split_cond;
>>>       rtx split;
>>>       rtvec attr;
>>>       int i;
>>> @@ -611,15 +610,20 @@ process_rtx (rtx desc, file_location loc)
>>>
>>>       /* If the split condition starts with "&&", append it to the
>>>          insn condition to create the new split condition.  */
>>> -     split_cond = XSTR (desc, 4);
>>> -     if (split_cond[0] == '&' && split_cond[1] == '&')
>>> +     const char *insn_cond = XSTR (desc, 2);
>>> +     const char *split_cond = XSTR (desc, 4);
>>> +     if (!strncmp (split_cond, "&&", 2))
>>>         {
>>>           rtx_reader_ptr->copy_md_ptr_loc (split_cond + 2, split_cond);
>>> -         split_cond = rtx_reader_ptr->join_c_conditions (XSTR (desc, 2),
>>> +         split_cond = rtx_reader_ptr->join_c_conditions (insn_cond,
>>>                                                           split_cond + 2);
>>> +       } else if (insn_cond[0]) {
>>> +         if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>>> +           error_at (loc, "the rewrite condition must start with `&&'");
>>> +         else
>>> +           error_at (loc, "the split condition must start with `&&' [%s]", insn_cond);
>>>         }
>>> -     else if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>>> -       error_at (loc, "the rewrite condition must start with `&&'");
>>> +
>>>       XSTR (split, 1) = split_cond;
>>>       if (GET_CODE (desc) == DEFINE_INSN_AND_REWRITE)
>>>         XVEC (split, 2) = gen_rewrite_sequence (XVEC (desc, 1));
>>>

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Kewen.Lin]

Hi Segher,

on 2021/6/8 上午7:50, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:
>> To find out those need fixing seems to be the critical part.  It's
>> not hard to add one explicit "&&" to those that don't have it now, but
>> even with further bootstrapped and regression tested I'm still not
>> confident the adjustments are safe enough, since the testing coverage
>> could be limited.  It may need more efforts to revisit, or/and test
>> with more coverages, and port maintainers' reviews.
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html
> 
> This adds an "&&" everywhere (or in fact, it just skips any existing
> one, it just has the same effect of adding it everywhere).  I tested it
> with building gcc and Linux for all supported targets (31 of them; I do
> some with multiple configs, mostly 32-bit and 64-bit).  None had any
> difference before and after the change.
> 
> So I am no longer worried that there will be any fallout from doing
> this.  There are many things that *could* go wrong, but I don't think
> there will be enough at all to be an impediment to just throwing the
> switch.
> 
> If we go this way no target will need any significant fixing, maybe none
> at all will be needed across all targets.  And no changes will be needed
> anywhere immediately.  We could make leading "&&" deprecated, and the
> same for split condition "1" (which was "&& 1").  This is easy to change
> automatically as well.
> 
Thanks very much for doing this!

I guess we are not going to backport this?  If we won't, it seems to need
some way to ensure the implied "&&" will show up explicitly when backporting
some define_insn_and_split.

BR,
Kewen

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Biener]

On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi Segher,
>
> on 2021/6/8 上午7:50, Segher Boessenkool wrote:
> > Hi!
> >
> > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:
> >> To find out those need fixing seems to be the critical part.  It's
> >> not hard to add one explicit "&&" to those that don't have it now, but
> >> even with further bootstrapped and regression tested I'm still not
> >> confident the adjustments are safe enough, since the testing coverage
> >> could be limited.  It may need more efforts to revisit, or/and test
> >> with more coverages, and port maintainers' reviews.
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html
> >
> > This adds an "&&" everywhere (or in fact, it just skips any existing
> > one, it just has the same effect of adding it everywhere).  I tested it
> > with building gcc and Linux for all supported targets (31 of them; I do
> > some with multiple configs, mostly 32-bit and 64-bit).  None had any
> > difference before and after the change.
> >
> > So I am no longer worried that there will be any fallout from doing
> > this.  There are many things that *could* go wrong, but I don't think
> > there will be enough at all to be an impediment to just throwing the
> > switch.
> >
> > If we go this way no target will need any significant fixing, maybe none
> > at all will be needed across all targets.  And no changes will be needed
> > anywhere immediately.  We could make leading "&&" deprecated, and the
> > same for split condition "1" (which was "&& 1").  This is easy to change
> > automatically as well.
> >
> Thanks very much for doing this!
>
> I guess we are not going to backport this?  If we won't, it seems to need
> some way to ensure the implied "&&" will show up explicitly when backporting
> some define_insn_and_split.

For this reason I'd prefer the explicit "&& ", Seghers testing means
mass-changing all define_insn_and_split is reasonable.

Richard.

> BR,
> Kewen

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Segher Boessenkool]

On Tue, Jun 08, 2021 at 09:08:56AM +0200, Richard Biener wrote:
> On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> > on 2021/6/8 上午7:50, Segher Boessenkool wrote:
> > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:
> > >> To find out those need fixing seems to be the critical part.  It's
> > >> not hard to add one explicit "&&" to those that don't have it now, but
> > >> even with further bootstrapped and regression tested I'm still not
> > >> confident the adjustments are safe enough, since the testing coverage
> > >> could be limited.  It may need more efforts to revisit, or/and test
> > >> with more coverages, and port maintainers' reviews.
> > >
> > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html
> > >
> > > This adds an "&&" everywhere (or in fact, it just skips any existing
> > > one, it just has the same effect of adding it everywhere).  I tested it
> > > with building gcc and Linux for all supported targets (31 of them; I do
> > > some with multiple configs, mostly 32-bit and 64-bit).  None had any
> > > difference before and after the change.
> > >
> > > So I am no longer worried that there will be any fallout from doing
> > > this.  There are many things that *could* go wrong, but I don't think
> > > there will be enough at all to be an impediment to just throwing the
> > > switch.
> > >
> > > If we go this way no target will need any significant fixing, maybe none
> > > at all will be needed across all targets.  And no changes will be needed
> > > anywhere immediately.  We could make leading "&&" deprecated, and the
> > > same for split condition "1" (which was "&& 1").  This is easy to change
> > > automatically as well.
> > >
> > Thanks very much for doing this!
> >
> > I guess we are not going to backport this?  If we won't, it seems to need
> > some way to ensure the implied "&&" will show up explicitly when backporting
> > some define_insn_and_split.
> 
> For this reason I'd prefer the explicit "&& ", Seghers testing means
> mass-changing all define_insn_and_split is reasonable.

So mass-change all define_insn_and_split where the split condition is
not inclusive of the insn condition (as written, i.e. before the
iterators have added stuff), to be separate define_insn and define_split
patterns?  And *then* add the &&?


Segher

Re: [RFC/PATCH 00/11] Fix up some unexpected empty split conditions

[Richard Biener]

On Tue, Jun 8, 2021 at 2:32 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Tue, Jun 08, 2021 at 09:08:56AM +0200, Richard Biener wrote:
> > On Tue, Jun 8, 2021 at 4:10 AM Kewen.Lin via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > > on 2021/6/8 上午7:50, Segher Boessenkool wrote:
> > > > On Fri, Jun 04, 2021 at 10:57:51AM +0800, Kewen.Lin via Gcc-patches wrote:
> > > >> To find out those need fixing seems to be the critical part.  It's
> > > >> not hard to add one explicit "&&" to those that don't have it now, but
> > > >> even with further bootstrapped and regression tested I'm still not
> > > >> confident the adjustments are safe enough, since the testing coverage
> > > >> could be limited.  It may need more efforts to revisit, or/and test
> > > >> with more coverages, and port maintainers' reviews.
> > > >
> > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572120.html
> > > >
> > > > This adds an "&&" everywhere (or in fact, it just skips any existing
> > > > one, it just has the same effect of adding it everywhere).  I tested it
> > > > with building gcc and Linux for all supported targets (31 of them; I do
> > > > some with multiple configs, mostly 32-bit and 64-bit).  None had any
> > > > difference before and after the change.
> > > >
> > > > So I am no longer worried that there will be any fallout from doing
> > > > this.  There are many things that *could* go wrong, but I don't think
> > > > there will be enough at all to be an impediment to just throwing the
> > > > switch.
> > > >
> > > > If we go this way no target will need any significant fixing, maybe none
> > > > at all will be needed across all targets.  And no changes will be needed
> > > > anywhere immediately.  We could make leading "&&" deprecated, and the
> > > > same for split condition "1" (which was "&& 1").  This is easy to change
> > > > automatically as well.
> > > >
> > > Thanks very much for doing this!
> > >
> > > I guess we are not going to backport this?  If we won't, it seems to need
> > > some way to ensure the implied "&&" will show up explicitly when backporting
> > > some define_insn_and_split.
> >
> > For this reason I'd prefer the explicit "&& ", Seghers testing means
> > mass-changing all define_insn_and_split is reasonable.
>
> So mass-change all define_insn_and_split where the split condition is
> not inclusive of the insn condition (as written, i.e. before the
> iterators have added stuff), to be separate define_insn and define_split
> patterns?  And *then* add the &&?

Possible.  Maybe first enable a warning for the case with not starting
with "&& "
to give maintainers a chance to fix them and only then do the mass change
to separate define_insn and define_split to avoid churn in the .md files.  Well
maintained ports should be quick to fixup themselves.

And some cases might even be obvious.

Richard.

>
> Segher