[PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

For stack_tie, currently below insn is generated:
(insn 15 14 16 3 (parallel [
             (set (mem/c:BLK (reg/f:DI 1 1) [1  A8])
                 (const_int 0 [0]))
         ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie}
      (nil))

It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])".  This maybe
looks like "a memory block is zerored", while actually stack_tie
may be more like a placeholder, and does not generate any thing.

To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could
be used here like other ports.

This patch does this.  Bootstrap&regtest pass on ppc64{,le}.
Is this ok for trunk?

BR,
Jeff (Jiufu Guo)

---
 gcc/config/rs6000/predicates.md   | 11 +++++++----
 gcc/config/rs6000/rs6000-logue.cc |  4 +++-
 gcc/config/rs6000/rs6000.cc       |  4 ++++
 gcc/config/rs6000/rs6000.md       | 14 ++++++++++----
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index a16ee30f0c0..4748cb37ce8 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation"
 (define_predicate "tie_operand"
   (match_code "parallel")
 {
-  return (GET_CODE (XVECEXP (op, 0, 0)) == SET
-	  && MEM_P (XEXP (XVECEXP (op, 0, 0), 0))
-	  && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode
-	  && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx);
+  rtx set = XVECEXP (op, 0, 0);
+  return (GET_CODE (set) == SET
+	  && MEM_P (SET_DEST (set))
+	  && GET_MODE (SET_DEST (set)) == BLKmode
+	  && GET_CODE (SET_SRC (set)) == UNSPEC
+	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
+	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
 })
 
 ;; Match a small code model toc reference (or medium and large
diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc
index bc6b153b59f..b99f43a8282 100644
--- a/gcc/config/rs6000/rs6000-logue.cc
+++ b/gcc/config/rs6000/rs6000-logue.cc
@@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)
   while (--i >= 0)
     {
       rtx mem = gen_frame_mem (BLKmode, regs[i]);
-      RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx);
+      RTVEC_ELT (p, i)
+	= gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
+					    UNSPEC_TIE));
     }
 
   emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p)));
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index d197c3f3289..0c81ebea711 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -1760,6 +1760,10 @@ static const struct attribute_spec rs6000_attribute_table[] =
 
 #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
 #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info
+
+#undef TARGET_CONST_ANCHOR
+#define TARGET_CONST_ANCHOR 0x8000
+
 \f
 
 /* Processor table.  */
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index b0db8ae508d..fdcf8347812 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -158,6 +158,7 @@ (define_c_enum "unspec"
    UNSPEC_HASHCHK
    UNSPEC_XXSPLTIDP_CONST
    UNSPEC_XXSPLTIW_CONST
+   UNSPEC_TIE
   ])
 
 ;;
@@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
   operands[4] = gen_frame_mem (Pmode, operands[1]);
   p = rtvec_alloc (1);
   RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
-				  const0_rtx);
+				  gen_rtx_UNSPEC (BLKmode,
+						  gen_rtvec (1, const0_rtx),
+						  UNSPEC_TIE));
   operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
 })
 
@@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal"
   operands[5] = gen_frame_mem (Pmode, operands[3]);
   p = rtvec_alloc (1);
   RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
-				  const0_rtx);
+				  gen_rtx_UNSPEC (BLKmode,
+						  gen_rtvec (1, const0_rtx),
+						  UNSPEC_TIE));
   operands[6] = gen_rtx_PARALLEL (VOIDmode, p);
 })
 \f
@@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs_<mode>_r1"
 ; not be moved over loads from or stores to stack memory.
 (define_insn "stack_tie"
   [(match_parallel 0 "tie_operand"
-		   [(set (mem:BLK (reg 1)) (const_int 0))])]
+		   [(set (mem:BLK (reg 1))
+		    (unspec:BLK [(const_int 0)] UNSPEC_TIE))])]
   ""
   ""
   [(set_attr "length" "0")])
@@ -13910,7 +13916,7 @@ (define_insn "stack_restore_tie"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r")
 	(plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r")
 		 (match_operand:SI 2 "reg_or_cint_operand" "O,rI")))
-   (set (mem:BLK (scratch)) (const_int 0))]
+   (set (mem:BLK (scratch)) (unspec:BLK [(const_int 0)] UNSPEC_TIE))]
   "TARGET_32BIT"
   "@
    mr %0,%1
-- 
2.39.3

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[David Edelsohn]

Hi, Jiufu

This definitely seems to be a better solution.

The TARGET_CONST_ANCHOR change should not be part of this patch.  Also
there is no ChangeLog for the patch.

This generally looks correct and consistent with other ports. I want
to give Segher a chance to double check it, if he wishes.

Thanks David

On Mon, Jun 12, 2023 at 9:19 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>
> Hi,
>
> For stack_tie, currently below insn is generated:
> (insn 15 14 16 3 (parallel [
>              (set (mem/c:BLK (reg/f:DI 1 1) [1  A8])
>                  (const_int 0 [0]))
>          ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie}
>       (nil))
>
> It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])".  This maybe
> looks like "a memory block is zerored", while actually stack_tie
> may be more like a placeholder, and does not generate any thing.
>
> To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could
> be used here like other ports.
>
> This patch does this.  Bootstrap&regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu Guo)
>
> ---
>  gcc/config/rs6000/predicates.md   | 11 +++++++----
>  gcc/config/rs6000/rs6000-logue.cc |  4 +++-
>  gcc/config/rs6000/rs6000.cc       |  4 ++++
>  gcc/config/rs6000/rs6000.md       | 14 ++++++++++----
>  4 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
> index a16ee30f0c0..4748cb37ce8 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation"
>  (define_predicate "tie_operand"
>    (match_code "parallel")
>  {
> -  return (GET_CODE (XVECEXP (op, 0, 0)) == SET
> -         && MEM_P (XEXP (XVECEXP (op, 0, 0), 0))
> -         && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode
> -         && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx);
> +  rtx set = XVECEXP (op, 0, 0);
> +  return (GET_CODE (set) == SET
> +         && MEM_P (SET_DEST (set))
> +         && GET_MODE (SET_DEST (set)) == BLKmode
> +         && GET_CODE (SET_SRC (set)) == UNSPEC
> +         && XINT (SET_SRC (set), 1) == UNSPEC_TIE
> +         && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
>  })
>
>  ;; Match a small code model toc reference (or medium and large
> diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc
> index bc6b153b59f..b99f43a8282 100644
> --- a/gcc/config/rs6000/rs6000-logue.cc
> +++ b/gcc/config/rs6000/rs6000-logue.cc
> @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)
>    while (--i >= 0)
>      {
>        rtx mem = gen_frame_mem (BLKmode, regs[i]);
> -      RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx);
> +      RTVEC_ELT (p, i)
> +       = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
> +                                           UNSPEC_TIE));
>      }
>
>    emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p)));
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index d197c3f3289..0c81ebea711 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -1760,6 +1760,10 @@ static const struct attribute_spec rs6000_attribute_table[] =
>
>  #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
>  #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info
> +
> +#undef TARGET_CONST_ANCHOR
> +#define TARGET_CONST_ANCHOR 0x8000
> +
>
>
>  /* Processor table.  */
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index b0db8ae508d..fdcf8347812 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -158,6 +158,7 @@ (define_c_enum "unspec"
>     UNSPEC_HASHCHK
>     UNSPEC_XXSPLTIDP_CONST
>     UNSPEC_XXSPLTIW_CONST
> +   UNSPEC_TIE
>    ])
>
>  ;;
> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
>    operands[4] = gen_frame_mem (Pmode, operands[1]);
>    p = rtvec_alloc (1);
>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
> -                                 const0_rtx);
> +                                 gen_rtx_UNSPEC (BLKmode,
> +                                                 gen_rtvec (1, const0_rtx),
> +                                                 UNSPEC_TIE));
>    operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
>  })
>
> @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal"
>    operands[5] = gen_frame_mem (Pmode, operands[3]);
>    p = rtvec_alloc (1);
>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
> -                                 const0_rtx);
> +                                 gen_rtx_UNSPEC (BLKmode,
> +                                                 gen_rtvec (1, const0_rtx),
> +                                                 UNSPEC_TIE));
>    operands[6] = gen_rtx_PARALLEL (VOIDmode, p);
>  })
>
> @@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs_<mode>_r1"
>  ; not be moved over loads from or stores to stack memory.
>  (define_insn "stack_tie"
>    [(match_parallel 0 "tie_operand"
> -                  [(set (mem:BLK (reg 1)) (const_int 0))])]
> +                  [(set (mem:BLK (reg 1))
> +                   (unspec:BLK [(const_int 0)] UNSPEC_TIE))])]
>    ""
>    ""
>    [(set_attr "length" "0")])
> @@ -13910,7 +13916,7 @@ (define_insn "stack_restore_tie"
>    [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r")
>         (plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r")
>                  (match_operand:SI 2 "reg_or_cint_operand" "O,rI")))
> -   (set (mem:BLK (scratch)) (const_int 0))]
> +   (set (mem:BLK (scratch)) (unspec:BLK [(const_int 0)] UNSPEC_TIE))]
>    "TARGET_32BIT"
>    "@
>     mr %0,%1
> --
> 2.39.3
>

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi David,

David Edelsohn <dje.gcc@gmail.com> writes:

> Hi, Jiufu
>
> This definitely seems to be a better solution.
>
> The TARGET_CONST_ANCHOR change should not be part of this patch.  Also
> there is no ChangeLog for the patch.

Thanks a lot for your quick review!! And sorry for the sending this patch
in a hurry.  I would update the patch accordingly.


BR,
Jeff (Jiufu Guo)

>
> This generally looks correct and consistent with other ports. I want
> to give Segher a chance to double check it, if he wishes.
>
> Thanks David
>
> On Mon, Jun 12, 2023 at 9:19 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> For stack_tie, currently below insn is generated:
>> (insn 15 14 16 3 (parallel [
>>              (set (mem/c:BLK (reg/f:DI 1 1) [1  A8])
>>                  (const_int 0 [0]))
>>          ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie}
>>       (nil))
>>
>> It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])".  This maybe
>> looks like "a memory block is zerored", while actually stack_tie
>> may be more like a placeholder, and does not generate any thing.
>>
>> To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could
>> be used here like other ports.
>>
>> This patch does this.  Bootstrap&regtest pass on ppc64{,le}.
>> Is this ok for trunk?
>>
>> BR,
>> Jeff (Jiufu Guo)
>>
>> ---
>>  gcc/config/rs6000/predicates.md   | 11 +++++++----
>>  gcc/config/rs6000/rs6000-logue.cc |  4 +++-
>>  gcc/config/rs6000/rs6000.cc       |  4 ++++
>>  gcc/config/rs6000/rs6000.md       | 14 ++++++++++----
>>  4 files changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
>> index a16ee30f0c0..4748cb37ce8 100644
>> --- a/gcc/config/rs6000/predicates.md
>> +++ b/gcc/config/rs6000/predicates.md
>> @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation"
>>  (define_predicate "tie_operand"
>>    (match_code "parallel")
>>  {
>> -  return (GET_CODE (XVECEXP (op, 0, 0)) == SET
>> -         && MEM_P (XEXP (XVECEXP (op, 0, 0), 0))
>> -         && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode
>> -         && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx);
>> +  rtx set = XVECEXP (op, 0, 0);
>> +  return (GET_CODE (set) == SET
>> +         && MEM_P (SET_DEST (set))
>> +         && GET_MODE (SET_DEST (set)) == BLKmode
>> +         && GET_CODE (SET_SRC (set)) == UNSPEC
>> +         && XINT (SET_SRC (set), 1) == UNSPEC_TIE
>> +         && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
>>  })
>>
>>  ;; Match a small code model toc reference (or medium and large
>> diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc
>> index bc6b153b59f..b99f43a8282 100644
>> --- a/gcc/config/rs6000/rs6000-logue.cc
>> +++ b/gcc/config/rs6000/rs6000-logue.cc
>> @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)
>>    while (--i >= 0)
>>      {
>>        rtx mem = gen_frame_mem (BLKmode, regs[i]);
>> -      RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx);
>> +      RTVEC_ELT (p, i)
>> +       = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
>> +                                           UNSPEC_TIE));
>>      }
>>
>>    emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p)));
>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> index d197c3f3289..0c81ebea711 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -1760,6 +1760,10 @@ static const struct attribute_spec rs6000_attribute_table[] =
>>
>>  #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
>>  #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info
>> +
>> +#undef TARGET_CONST_ANCHOR
>> +#define TARGET_CONST_ANCHOR 0x8000
>> +
>>
>>
>>  /* Processor table.  */
>> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
>> index b0db8ae508d..fdcf8347812 100644
>> --- a/gcc/config/rs6000/rs6000.md
>> +++ b/gcc/config/rs6000/rs6000.md
>> @@ -158,6 +158,7 @@ (define_c_enum "unspec"
>>     UNSPEC_HASHCHK
>>     UNSPEC_XXSPLTIDP_CONST
>>     UNSPEC_XXSPLTIW_CONST
>> +   UNSPEC_TIE
>>    ])
>>
>>  ;;
>> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
>>    operands[4] = gen_frame_mem (Pmode, operands[1]);
>>    p = rtvec_alloc (1);
>>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
>> -                                 const0_rtx);
>> +                                 gen_rtx_UNSPEC (BLKmode,
>> +                                                 gen_rtvec (1, const0_rtx),
>> +                                                 UNSPEC_TIE));
>>    operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
>>  })
>>
>> @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal"
>>    operands[5] = gen_frame_mem (Pmode, operands[3]);
>>    p = rtvec_alloc (1);
>>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
>> -                                 const0_rtx);
>> +                                 gen_rtx_UNSPEC (BLKmode,
>> +                                                 gen_rtvec (1, const0_rtx),
>> +                                                 UNSPEC_TIE));
>>    operands[6] = gen_rtx_PARALLEL (VOIDmode, p);
>>  })
>>
>> @@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs_<mode>_r1"
>>  ; not be moved over loads from or stores to stack memory.
>>  (define_insn "stack_tie"
>>    [(match_parallel 0 "tie_operand"
>> -                  [(set (mem:BLK (reg 1)) (const_int 0))])]
>> +                  [(set (mem:BLK (reg 1))
>> +                   (unspec:BLK [(const_int 0)] UNSPEC_TIE))])]
>>    ""
>>    ""
>>    [(set_attr "length" "0")])
>> @@ -13910,7 +13916,7 @@ (define_insn "stack_restore_tie"
>>    [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r")
>>         (plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r")
>>                  (match_operand:SI 2 "reg_or_cint_operand" "O,rI")))
>> -   (set (mem:BLK (scratch)) (const_int 0))]
>> +   (set (mem:BLK (scratch)) (unspec:BLK [(const_int 0)] UNSPEC_TIE))]
>>    "TARGET_32BIT"
>>    "@
>>     mr %0,%1
>> --
>> 2.39.3
>>

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote:
> David Edelsohn <dje.gcc@gmail.com> writes:
> >
> > This definitely seems to be a better solution.
> >
> > The TARGET_CONST_ANCHOR change should not be part of this patch.  Also
> > there is no ChangeLog for the patch.
> 
> Thanks a lot for your quick review!! And sorry for the sending this patch
> in a hurry.  I would update the patch accordingly.

> > This generally looks correct and consistent with other ports. I want
> > to give Segher a chance to double check it, if he wishes.

The documentation is very clear that the only thing for which you can
have BLKmode is "mem".  Not unspec, only "mem".

Let's not do this.  The existing code has clear and obvious semantics,
which is documented as well -- there is no reason to make it worse in
every respect!


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[David Edelsohn]

On Tue, Jun 13, 2023 at 2:16 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Hi!
>
> On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote:
> > David Edelsohn <dje.gcc@gmail.com> writes:
> > >
> > > This definitely seems to be a better solution.
> > >
> > > The TARGET_CONST_ANCHOR change should not be part of this patch.  Also
> > > there is no ChangeLog for the patch.
> >
> > Thanks a lot for your quick review!! And sorry for the sending this patch
> > in a hurry.  I would update the patch accordingly.
>
> > > This generally looks correct and consistent with other ports. I want
> > > to give Segher a chance to double check it, if he wishes.
>
> The documentation is very clear that the only thing for which you can
> have BLKmode is "mem".  Not unspec, only "mem".
>
> Let's not do this.  The existing code has clear and obvious semantics,
> which is documented as well -- there is no reason to make it worse in
> every respect.

Segher,

Unfortunately, GCC now is inconsistent and this response is incorrect.
The documentation is out of date or was ignored and the "facts on the
ground" contradict your review.

Yes, (const_int 0) is supposed to be a general no-op and BLKmode only
is supposed to be used for MEM, but other major targets (arm, aarch64,
riscv, s390) all use unspec:BLK and specifically UNSPEC_TIE.  rs6000
is the only port that does not follow this convention.  The middle-end
has adapted to the behavior of all of the other targets, whether that
conformed to the documentation or not.  The rs6000 port needs to be
fixed and Jiufu's approach is the correct one, consistent with all
other targets for stack tie.  If the documentation differs, the
documentation needs to be updated, not a different approach for the
rs6000 port.  Jiufu's patch is correct.

Thanks, David

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi Segher, David,

David Edelsohn <dje.gcc@gmail.com> writes:

> On Tue, Jun 13, 2023 at 2:16 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>>
>> Hi!
>>
>> On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote:
>> > David Edelsohn <dje.gcc@gmail.com> writes:
>> > >
>> > > This definitely seems to be a better solution.
>> > >
>> > > The TARGET_CONST_ANCHOR change should not be part of this patch.  Also
>> > > there is no ChangeLog for the patch.
>> >
>> > Thanks a lot for your quick review!! And sorry for the sending this patch
>> > in a hurry.  I would update the patch accordingly.
>>
>> > > This generally looks correct and consistent with other ports. I want
>> > > to give Segher a chance to double check it, if he wishes.
>>
>> The documentation is very clear that the only thing for which you can
>> have BLKmode is "mem".  Not unspec, only "mem".
>>
>> Let's not do this.  The existing code has clear and obvious semantics,
>> which is documented as well -- there is no reason to make it worse in
>> every respect.

Thanks for all your insight comments!

Yeap, while "unspec:BLK" is very widely used already on various ports.
And it seems a few place is using BLKmode without strictly align with
the document :( It would not be very good thing, but maybe no better
solutions.

For existing code "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])"
Since it is a set, the operand set_src should be valid for
the mode of the set_dest. While set_src is 'const_int 0'.
And this 'set' may be mis-readed as 'a memory is zeroed' or
'no-op to a mem'. Using unspec here would just say this is an special
operation instead a normal 'const_int 0'.

BR,
Jeff (Jiufu Guo)

>
> Segher,
>
> Unfortunately, GCC now is inconsistent and this response is incorrect.
> The documentation is out of date or was ignored and the "facts on the
> ground" contradict your review.
>
> Yes, (const_int 0) is supposed to be a general no-op and BLKmode only
> is supposed to be used for MEM, but other major targets (arm, aarch64,
> riscv, s390) all use unspec:BLK and specifically UNSPEC_TIE.  rs6000
> is the only port that does not follow this convention.  The middle-end
> has adapted to the behavior of all of the other targets, whether that
> conformed to the documentation or not.  The rs6000 port needs to be
> fixed and Jiufu's approach is the correct one, consistent with all
> other targets for stack tie.  If the documentation differs, the
> documentation needs to be updated, not a different approach for the
> rs6000 port.  Jiufu's patch is correct.
>
> Thanks, David

[PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

For stack_tie, currently below insn is generated:
(insn 15 14 16 3 (parallel [
             (set (mem/c:BLK (reg/f:DI 1 1) [1  A8])
                 (const_int 0 [0]))
         ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie}
      (nil))

It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])".  This maybe
looks like "a memory block is zerored", while actually stack_tie
may be more like a placeholder, and does not generate any thing.

To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could
be used here.

Compare with previous version, this addes ChangeLog and removes
const_anchor parts.
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html.

Bootstrap&regtest pass on ppc64{,le}.
Is this ok for trunk?

BR,
Jeff (Jiufu Guo)

gcc/ChangeLog:

	* config/rs6000/predicates.md (tie_operand): Update to match new
	stack_tie pattern.
	* config/rs6000/rs6000-logue.cc (rs6000_emit_stack_tie): Update to
	use the new stack_tie pattern.
	* config/rs6000/rs6000.md (UNSPEC_TIE): New UNSPEC.
	(restore_stack_block): Update to use the new stack_tie pattern.
	(restore_stack_nonlocal): Likewise.
	(stack_tie): Update pattern to use UNSPEC_TIE.
	(stack_restore_tie): Likewise.	

---
 gcc/config/rs6000/predicates.md   | 11 +++++++----
 gcc/config/rs6000/rs6000-logue.cc |  4 +++-
 gcc/config/rs6000/rs6000.md       | 14 ++++++++++----
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index a16ee30f0c0..4748cb37ce8 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation"
 (define_predicate "tie_operand"
   (match_code "parallel")
 {
-  return (GET_CODE (XVECEXP (op, 0, 0)) == SET
-	  && MEM_P (XEXP (XVECEXP (op, 0, 0), 0))
-	  && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode
-	  && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx);
+  rtx set = XVECEXP (op, 0, 0);
+  return (GET_CODE (set) == SET
+	  && MEM_P (SET_DEST (set))
+	  && GET_MODE (SET_DEST (set)) == BLKmode
+	  && GET_CODE (SET_SRC (set)) == UNSPEC
+	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
+	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
 })
 
 ;; Match a small code model toc reference (or medium and large
diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc
index bc6b153b59f..b99f43a8282 100644
--- a/gcc/config/rs6000/rs6000-logue.cc
+++ b/gcc/config/rs6000/rs6000-logue.cc
@@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)
   while (--i >= 0)
     {
       rtx mem = gen_frame_mem (BLKmode, regs[i]);
-      RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx);
+      RTVEC_ELT (p, i)
+	= gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
+					    UNSPEC_TIE));
     }
 
   emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p)));
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index b0db8ae508d..fdcf8347812 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -158,6 +158,7 @@ (define_c_enum "unspec"
    UNSPEC_HASHCHK
    UNSPEC_XXSPLTIDP_CONST
    UNSPEC_XXSPLTIW_CONST
+   UNSPEC_TIE
   ])
 
 ;;
@@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
   operands[4] = gen_frame_mem (Pmode, operands[1]);
   p = rtvec_alloc (1);
   RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
-				  const0_rtx);
+				  gen_rtx_UNSPEC (BLKmode,
+						  gen_rtvec (1, const0_rtx),
+						  UNSPEC_TIE));
   operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
 })
 
@@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal"
   operands[5] = gen_frame_mem (Pmode, operands[3]);
   p = rtvec_alloc (1);
   RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
-				  const0_rtx);
+				  gen_rtx_UNSPEC (BLKmode,
+						  gen_rtvec (1, const0_rtx),
+						  UNSPEC_TIE));
   operands[6] = gen_rtx_PARALLEL (VOIDmode, p);
 })
 \f
@@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs_<mode>_r1"
 ; not be moved over loads from or stores to stack memory.
 (define_insn "stack_tie"
   [(match_parallel 0 "tie_operand"
-		   [(set (mem:BLK (reg 1)) (const_int 0))])]
+		   [(set (mem:BLK (reg 1))
+		    (unspec:BLK [(const_int 0)] UNSPEC_TIE))])]
   ""
   ""
   [(set_attr "length" "0")])
@@ -13910,7 +13916,7 @@ (define_insn "stack_restore_tie"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r")
 	(plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r")
 		 (match_operand:SI 2 "reg_or_cint_operand" "O,rI")))
-   (set (mem:BLK (scratch)) (const_int 0))]
+   (set (mem:BLK (scratch)) (unspec:BLK [(const_int 0)] UNSPEC_TIE))]
   "TARGET_32BIT"
   "@
    mr %0,%1
-- 
2.39.3

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Xi Ruoyao]

On Tue, 2023-06-13 at 20:23 +0800, Jiufu Guo via Gcc-patches wrote:

> Compare with previous version, this addes ChangeLog and removes
> const_anchor parts.
> https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html.

[Off topic]

const_anchor is just broken now.  See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104843 and the thread
beginning at
https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591470.html.  If
you want to use it for rs6000 I guess you need to fix it first...

To me const_anchor needs a complete rework but I don't want to spend my
time on it.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Xi Ruoyao <xry111@xry111.site> writes:

> On Tue, 2023-06-13 at 20:23 +0800, Jiufu Guo via Gcc-patches wrote:
>
>> Compare with previous version, this addes ChangeLog and removes
>> const_anchor parts.
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html.
>
> [Off topic]
>
> const_anchor is just broken now.  See
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104843 and the thread
> beginning at
> https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591470.html.  If
> you want to use it for rs6000 I guess you need to fix it first...

Thanks so much for pointing out this.  It seems about supporting
negative value, right?

As you say: for 1. "g(0x8123ffff, 0x81240001)", it would be fine.

The generated insns are:
(insn 5 2 6 2 (set (reg:DI 117)
        (const_int -2128347135 [0xffffffff81240001])) "negative.c":5:3 681 {*movdi_internal64}
     (nil))
(insn 6 5 7 2 (set (reg:DI 118)
        (plus:DI (reg:DI 117)
            (const_int -2 [0xfffffffffffffffe]))) "negative.c":5:3 66 {*adddi3}
     (expr_list:REG_EQUAL (const_int -2128347137 [0xffffffff8123ffff])
        (nil)))

While for 2. "g (0x7fffffff, 0x80000001)", the generated rtl insns:
(insn 5 2 6 2 (set (reg:DI 117)
        (const_int -2147483647 [0xffffffff80000001])) "negative.c":5:3 681 {*movdi_internal64}
     (nil))
(insn 7 6 8 2 (set (reg:DI 3 3)
        (const_int 2147483647 [0x7fffffff])) "negative.c":5:3 681 {*movdi_internal64}
     (nil))

The current const_anchor does not generate sth like: "r3 = r117 - 2"
But I would lean to say it is the limitation of current implementation:
"0xffffffff80000001" and "0x7fffffff" hit different anchors(even these
two values are 'close' on some aspect.)

BR,
Jeff (Jiufu Guo)

>
> To me const_anchor needs a complete rework but I don't want to spend my
> time on it.

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Xi Ruoyao]

On Wed, 2023-06-14 at 09:55 +0800, Jiufu Guo wrote:
> Hi,
> 
> Xi Ruoyao <xry111@xry111.site> writes:
> 
> > On Tue, 2023-06-13 at 20:23 +0800, Jiufu Guo via Gcc-patches wrote:
> > 
> > > Compare with previous version, this addes ChangeLog and removes
> > > const_anchor parts.
> > > https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621356.html.
> > 
> > [Off topic]
> > 
> > const_anchor is just broken now.  See
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104843 and the thread
> > beginning at
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591470.html.  If
> > you want to use it for rs6000 I guess you need to fix it first...
> 
> Thanks so much for pointing out this.  It seems about supporting
> negative value, right?
> 
> As you say: for 1. "g(0x8123ffff, 0x81240001)", it would be fine.
> 
> The generated insns are:
> (insn 5 2 6 2 (set (reg:DI 117)
>         (const_int -2128347135 [0xffffffff81240001])) "negative.c":5:3 681 {*movdi_internal64}
>      (nil))
> (insn 6 5 7 2 (set (reg:DI 118)
>         (plus:DI (reg:DI 117)
>             (const_int -2 [0xfffffffffffffffe]))) "negative.c":5:3 66 {*adddi3}
>      (expr_list:REG_EQUAL (const_int -2128347137 [0xffffffff8123ffff])
>         (nil)))
> 
> While for 2. "g (0x7fffffff, 0x80000001)", the generated rtl insns:
> (insn 5 2 6 2 (set (reg:DI 117)
>         (const_int -2147483647 [0xffffffff80000001])) "negative.c":5:3 681 {*movdi_internal64}
>      (nil))
> (insn 7 6 8 2 (set (reg:DI 3 3)
>         (const_int 2147483647 [0x7fffffff])) "negative.c":5:3 681 {*movdi_internal64}
>      (nil))
> 
> The current const_anchor does not generate sth like: "r3 = r117 - 2"
> But I would lean to say it is the limitation of current implementation:
> "0xffffffff80000001" and "0x7fffffff" hit different anchors(even these
> two values are 'close' on some aspect.)

The generic issue here is to fix (not "papering over") the signed
overflow, we need to perform the addition in a target machine mode.  We
may always use Pmode (IIRC const_anchor was introduced for optimizing
some constant addresses), but can we do better?

Should we try addition in both DImode and SImode for a 64-bit capable
machine?

Or should we even try more operations than addition (for eg bit
operations like xor or shift)?  Doing so will need to create a new
target hook for const anchoring, this is the "complete rework" I meant.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Wed, Jun 14, 2023 at 05:18:15PM +0800, Xi Ruoyao wrote:
> The generic issue here is to fix (not "papering over") the signed
> overflow, we need to perform the addition in a target machine mode.  We
> may always use Pmode (IIRC const_anchor was introduced for optimizing
> some constant addresses), but can we do better?

The main issue is that the machine description generated target code to
compute some constants, but the sanitizer treats it as if it was user
code that might do wrong things.

> Should we try addition in both DImode and SImode for a 64-bit capable
> machine?

Why?  At least on PowerPC there is only one insn, and it is 64 bits.
The SImode version just ignores all bits other than the low 32 bits, in
both inputs and output.

> Or should we even try more operations than addition (for eg bit
> operations like xor or shift)?  Doing so will need to create a new
> target hook for const anchoring, this is the "complete rework" I meant.

This might make const anchor useful for way more targets maybe,
including rs6000, yes :-)


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Wed, Jun 14, 2023 at 05:18:15PM +0800, Xi Ruoyao wrote:
>> The generic issue here is to fix (not "papering over") the signed
>> overflow, we need to perform the addition in a target machine mode.  We
>> may always use Pmode (IIRC const_anchor was introduced for optimizing
>> some constant addresses), but can we do better?
>
> The main issue is that the machine description generated target code to
> compute some constants, but the sanitizer treats it as if it was user
> code that might do wrong things.
>
>> Should we try addition in both DImode and SImode for a 64-bit capable
>> machine?
>
> Why?  At least on PowerPC there is only one insn, and it is 64 bits.
> The SImode version just ignores all bits other than the low 32 bits, in
> both inputs and output.
>
>> Or should we even try more operations than addition (for eg bit
>> operations like xor or shift)?  Doing so will need to create a new
>> target hook for const anchoring, this is the "complete rework" I meant.
>

Yeap! This would be a different implementation than the current
const_anchor in cse.cc. In postreload.cc, there is another
implementation: "reload_cse_move2add" which checks all 'add's
instructions from the target. But both implementations have pros
and cons.

Using gcc source code as a benchmark, analyzing the relations
between constants (focusing on those constants in the same
function or the same basic block). IIRC, 'add's can cover
most of the relations. Small part of constants can be built
via other operations(e.g. shift, and, neg ,...).
There may be still some benchmarks that hit other operations
in the hot path.

Indeed, the const_anchor feature could be enhanced to cover
more cases.


BR,
Jeff (Jiufu Guo)

> This might make const anchor useful for way more targets maybe,
> including rs6000, yes :-)
>
>
> Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

As I said in a reply to the original patch: not okay.  Sorry.

But some comments on this patch:

On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote:
> +	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
> +	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);

This makes it required that the operand of an UNSPEC_TIE unspec is a
const_int 0.  This should be documented somewhere.  Ideally you would
want no operand at all here, but every unspec has an operand.

> +      RTVEC_ELT (p, i)
> +	= gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
> +					    UNSPEC_TIE));

If it is hard to indent your code, your code is trying to do to much.
Just have an extra temporary?

      rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), UNSPEC_TIE);
      RTVEC_ELT (p, i) = gen_rtx_SET (mem, un);

That is shorter even, and certainly more readable :-)

> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
>    operands[4] = gen_frame_mem (Pmode, operands[1]);
>    p = rtvec_alloc (1);
>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
> -				  const0_rtx);
> +				  gen_rtx_UNSPEC (BLKmode,
> +						  gen_rtvec (1, const0_rtx),
> +						  UNSPEC_TIE));
>    operands[5] = gen_rtx_PARALLEL (VOIDmode, p);

I have a hard time to see how this could ever be seen as clearer or more
obvious or anything like that :-(


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> As I said in a reply to the original patch: not okay.  Sorry.

Thanks a lot for your comments!
I'm also thinking about other solutions:
1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])"
  This is the existing pattern.  It may be read as an action
  to clean an unknown-size memory block.

2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
UNSPEC_TIE".
  Current patch is using this one.

3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
UNSPEC_TIE".
   This avoids using BLK on unspec, but using DI.

4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0])
UNSPEC_TIE"
   There is still a mode for the unspec.


>
> But some comments on this patch:
>
> On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote:
>> +	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
>> +	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
>
> This makes it required that the operand of an UNSPEC_TIE unspec is a
> const_int 0.  This should be documented somewhere.  Ideally you would
> want no operand at all here, but every unspec has an operand.

Right!  Since checked UNSPEC_TIE arleady, we may not need to check
the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);".

>
>> +      RTVEC_ELT (p, i)
>> +	= gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
>> +					    UNSPEC_TIE));
>
> If it is hard to indent your code, your code is trying to do to much.
> Just have an extra temporary?
>
>       rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), UNSPEC_TIE);
>       RTVEC_ELT (p, i) = gen_rtx_SET (mem, un);
>
> That is shorter even, and certainly more readable :-)

Yeap, thanks!

>
>> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
>>    operands[4] = gen_frame_mem (Pmode, operands[1]);
>>    p = rtvec_alloc (1);
>>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
>> -				  const0_rtx);
>> +				  gen_rtx_UNSPEC (BLKmode,
>> +						  gen_rtvec (1, const0_rtx),
>> +						  UNSPEC_TIE));
>>    operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
>
> I have a hard time to see how this could ever be seen as clearer or more
> obvious or anything like that :-(

I was thinking about just invoking gen_stack_tie here.

BR,
Jeff (Jiufu Guo)

>
>
> Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Biener]

On Wed, 14 Jun 2023, Jiufu Guo wrote:

> 
> Hi,
> 
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> 
> > Hi!
> >
> > As I said in a reply to the original patch: not okay.  Sorry.
> 
> Thanks a lot for your comments!
> I'm also thinking about other solutions:
> 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])"
>   This is the existing pattern.  It may be read as an action
>   to clean an unknown-size memory block.
> 
> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
> UNSPEC_TIE".
>   Current patch is using this one.
> 
> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
> UNSPEC_TIE".
>    This avoids using BLK on unspec, but using DI.

That gives the MEM a size which means we can interpret the (set ..)
as killing a specific area of memory, enabling DSE of earlier
stores.

AFAIU this special instruction is only supposed to prevent
code motion (of stack memory accesses?) across this instruction?
I'd say a

  (may_clobber (mem:BLK (reg:DI 1 1)))

might be more to the point?  I've used "may_clobber" which doesn't
exist since I'm not sure whether a clobber is considered a kill.
The docs say "Represents the storing or possible storing of an 
unpredictable..." - what is it?  Storing or possible storing?
I suppose stack_tie should be less strict than the documented
(clobber (mem:BLK (const_int 0))) (clobber all memory).

?

> 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0])
> UNSPEC_TIE"
>    There is still a mode for the unspec.
> 
> 
> >
> > But some comments on this patch:
> >
> > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote:
> >> +	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
> >> +	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
> >
> > This makes it required that the operand of an UNSPEC_TIE unspec is a
> > const_int 0.  This should be documented somewhere.  Ideally you would
> > want no operand at all here, but every unspec has an operand.
> 
> Right!  Since checked UNSPEC_TIE arleady, we may not need to check
> the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);".
> 
> >
> >> +      RTVEC_ELT (p, i)
> >> +	= gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
> >> +					    UNSPEC_TIE));
> >
> > If it is hard to indent your code, your code is trying to do to much.
> > Just have an extra temporary?
> >
> >       rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), UNSPEC_TIE);
> >       RTVEC_ELT (p, i) = gen_rtx_SET (mem, un);
> >
> > That is shorter even, and certainly more readable :-)
> 
> Yeap, thanks!
> 
> >
> >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
> >>    operands[4] = gen_frame_mem (Pmode, operands[1]);
> >>    p = rtvec_alloc (1);
> >>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
> >> -				  const0_rtx);
> >> +				  gen_rtx_UNSPEC (BLKmode,
> >> +						  gen_rtvec (1, const0_rtx),
> >> +						  UNSPEC_TIE));
> >>    operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
> >
> > I have a hard time to see how this could ever be seen as clearer or more
> > obvious or anything like that :-(
> 
> I was thinking about just invoking gen_stack_tie here.
> 
> BR,
> Jeff (Jiufu Guo)
> 
> >
> >
> > Segher
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> AFAIU this special instruction is only supposed to prevent
> code motion (of stack memory accesses?) across this instruction?
> I'd say a
>
>   (may_clobber (mem:BLK (reg:DI 1 1)))
>
> might be more to the point?  I've used "may_clobber" which doesn't
> exist since I'm not sure whether a clobber is considered a kill.
> The docs say "Represents the storing or possible storing of an 
> unpredictable..." - what is it? Storing or possible storing?

I'd also understood it to be either.  As in, it is a may-clobber
that can be used for must-clobber.  Alternatively: the value stored
is unpredictable, and can therefore be the same as the current value.

I think the main difference between:

  (clobber (mem:BLK …))

and

  (set (mem:BLK …) (unspec:BLK …))

is that the latter must happen for correctness (unless something
that understands the unspec proves otherwise) whereas a clobber
can validly be dropped.  So for something like stack_tie, a set
seems more correct than a clobber.

Thanks,
Richard

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Biener]

On Wed, 14 Jun 2023, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > AFAIU this special instruction is only supposed to prevent
> > code motion (of stack memory accesses?) across this instruction?
> > I'd say a
> >
> >   (may_clobber (mem:BLK (reg:DI 1 1)))
> >
> > might be more to the point?  I've used "may_clobber" which doesn't
> > exist since I'm not sure whether a clobber is considered a kill.
> > The docs say "Represents the storing or possible storing of an 
> > unpredictable..." - what is it? Storing or possible storing?
> 
> I'd also understood it to be either.  As in, it is a may-clobber
> that can be used for must-clobber.  Alternatively: the value stored
> is unpredictable, and can therefore be the same as the current value.
> 
> I think the main difference between:
> 
>   (clobber (mem:BLK ?))
> 
> and
> 
>   (set (mem:BLK ?) (unspec:BLK ?))
> 
> is that the latter must happen for correctness (unless something
> that understands the unspec proves otherwise) whereas a clobber
> can validly be dropped.  So for something like stack_tie, a set
> seems more correct than a clobber.

How can a clobber be validly dropped?  For the case of stack
memory if there's no stack use after it it could be elided
and I suppose the clobber itself can be moved.  But then
the function return is a stack use as well.

Btw, with the same reason the (set (mem:...)) could be removed, no?
Or is the (unspec:) SET_SRC having implicit side-effects that
prevents the removal (so rs6000 could have its stack_tie removed)?

That said, I fail to see how a clobber is special here.

Richard.

> Thanks,
> Richard
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> On Wed, 14 Jun 2023, Richard Sandiford wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> > AFAIU this special instruction is only supposed to prevent
>> > code motion (of stack memory accesses?) across this instruction?
>> > I'd say a
>> >
>> >   (may_clobber (mem:BLK (reg:DI 1 1)))
>> >
>> > might be more to the point?  I've used "may_clobber" which doesn't
>> > exist since I'm not sure whether a clobber is considered a kill.
>> > The docs say "Represents the storing or possible storing of an 
>> > unpredictable..." - what is it? Storing or possible storing?
>> 
>> I'd also understood it to be either.  As in, it is a may-clobber
>> that can be used for must-clobber.  Alternatively: the value stored
>> is unpredictable, and can therefore be the same as the current value.
>> 
>> I think the main difference between:
>> 
>>   (clobber (mem:BLK ?))
>> 
>> and
>> 
>>   (set (mem:BLK ?) (unspec:BLK ?))
>> 
>> is that the latter must happen for correctness (unless something
>> that understands the unspec proves otherwise) whereas a clobber
>> can validly be dropped.  So for something like stack_tie, a set
>> seems more correct than a clobber.
>
> How can a clobber be validly dropped?  For the case of stack
> memory if there's no stack use after it it could be elided
> and I suppose the clobber itself can be moved.  But then
> the function return is a stack use as well.
>
> Btw, with the same reason the (set (mem:...)) could be removed, no?
> Or is the (unspec:) SET_SRC having implicit side-effects that
> prevents the removal (so rs6000 could have its stack_tie removed)?
>
> That said, I fail to see how a clobber is special here.

Clobbers are for side-effects.  They don't start a def-use chain.
E.g. any use after a full clobber is an uninitialised read rather
than a read of the clobber “result”.

In contrast, a set of memory with an unspec source is in dataflow terms
the same as a set of memory with a specified source.  (some unspecs
actually have well-defined values, it's just that only the target code
knows what those well-defined value are.)

So a set of memory could only be removed if DSE proves that there are no
reads of the set bytes before the next set(s) to the same bytes of memory.
And memory is always live.

Thanks,
Richard

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Biener]

On Wed, 14 Jun 2023, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > On Wed, 14 Jun 2023, Richard Sandiford wrote:
> >
> >> Richard Biener <rguenther@suse.de> writes:
> >> > AFAIU this special instruction is only supposed to prevent
> >> > code motion (of stack memory accesses?) across this instruction?
> >> > I'd say a
> >> >
> >> >   (may_clobber (mem:BLK (reg:DI 1 1)))
> >> >
> >> > might be more to the point?  I've used "may_clobber" which doesn't
> >> > exist since I'm not sure whether a clobber is considered a kill.
> >> > The docs say "Represents the storing or possible storing of an 
> >> > unpredictable..." - what is it? Storing or possible storing?
> >> 
> >> I'd also understood it to be either.  As in, it is a may-clobber
> >> that can be used for must-clobber.  Alternatively: the value stored
> >> is unpredictable, and can therefore be the same as the current value.
> >> 
> >> I think the main difference between:
> >> 
> >>   (clobber (mem:BLK ?))
> >> 
> >> and
> >> 
> >>   (set (mem:BLK ?) (unspec:BLK ?))
> >> 
> >> is that the latter must happen for correctness (unless something
> >> that understands the unspec proves otherwise) whereas a clobber
> >> can validly be dropped.  So for something like stack_tie, a set
> >> seems more correct than a clobber.
> >
> > How can a clobber be validly dropped?  For the case of stack
> > memory if there's no stack use after it it could be elided
> > and I suppose the clobber itself can be moved.  But then
> > the function return is a stack use as well.
> >
> > Btw, with the same reason the (set (mem:...)) could be removed, no?
> > Or is the (unspec:) SET_SRC having implicit side-effects that
> > prevents the removal (so rs6000 could have its stack_tie removed)?
> >
> > That said, I fail to see how a clobber is special here.
> 
> Clobbers are for side-effects.  They don't start a def-use chain.
> E.g. any use after a full clobber is an uninitialised read rather
> than a read of the clobber ?result?.

I see.  So

(parallel
 (unspec stack_tie)
 (clobber (mem:BLK ...)))

then?  I suppose it needs to be an unspec_volatile?  It feels like
the stack_ties are a delicate hack preventing enough but not too
much optimization ...

> In contrast, a set of memory with an unspec source is in dataflow terms
> the same as a set of memory with a specified source.  (some unspecs
> actually have well-defined values, it's just that only the target code
> knows what those well-defined value are.)
> 
> So a set of memory could only be removed if DSE proves that there are no
> reads of the set bytes before the next set(s) to the same bytes of memory.
> And memory is always live.
> 
> Thanks,
> Richard
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> On Wed, 14 Jun 2023, Richard Sandiford wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> > On Wed, 14 Jun 2023, Richard Sandiford wrote:
>> >
>> >> Richard Biener <rguenther@suse.de> writes:
>> >> > AFAIU this special instruction is only supposed to prevent
>> >> > code motion (of stack memory accesses?) across this instruction?
>> >> > I'd say a
>> >> >
>> >> >   (may_clobber (mem:BLK (reg:DI 1 1)))
>> >> >
>> >> > might be more to the point?  I've used "may_clobber" which doesn't
>> >> > exist since I'm not sure whether a clobber is considered a kill.
>> >> > The docs say "Represents the storing or possible storing of an 
>> >> > unpredictable..." - what is it? Storing or possible storing?
>> >> 
>> >> I'd also understood it to be either.  As in, it is a may-clobber
>> >> that can be used for must-clobber.  Alternatively: the value stored
>> >> is unpredictable, and can therefore be the same as the current value.
>> >> 
>> >> I think the main difference between:
>> >> 
>> >>   (clobber (mem:BLK ?))
>> >> 
>> >> and
>> >> 
>> >>   (set (mem:BLK ?) (unspec:BLK ?))
>> >> 
>> >> is that the latter must happen for correctness (unless something
>> >> that understands the unspec proves otherwise) whereas a clobber
>> >> can validly be dropped.  So for something like stack_tie, a set
>> >> seems more correct than a clobber.
>> >
>> > How can a clobber be validly dropped?  For the case of stack
>> > memory if there's no stack use after it it could be elided
>> > and I suppose the clobber itself can be moved.  But then
>> > the function return is a stack use as well.
>> >
>> > Btw, with the same reason the (set (mem:...)) could be removed, no?
>> > Or is the (unspec:) SET_SRC having implicit side-effects that
>> > prevents the removal (so rs6000 could have its stack_tie removed)?
>> >
>> > That said, I fail to see how a clobber is special here.
>> 
>> Clobbers are for side-effects.  They don't start a def-use chain.
>> E.g. any use after a full clobber is an uninitialised read rather
>> than a read of the clobber ?result?.
>
> I see.  So
>
> (parallel
>  (unspec stack_tie)
>  (clobber (mem:BLK ...)))
>
> then?  I suppose it needs to be an unspec_volatile?

Yeah, it would need to be unspec_volatile, at which point it becomes
quite a big hammer.

> It feels like the stack_ties are a delicate hack preventing enough but
> not too much optimization ...

Yup.  I think the only non-hacky way would be to have dedicated RTL for
memory becoming valid and becoming invalid.  Anything else is a compromise.

But TBH, I still think the (set (mem:BLK …) (unspec:BLK …)) strikes
the right balance, unless there's a specific argument otherwise.
The effect on memory isn't a side effect (contrary to what clobber
implies) but instead is the main purpose of allocating and deallocating
stack memory.

Thanks,
Richard

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Wed, Jun 14, 2023 at 09:52:37AM +0000, Richard Biener wrote:
> I see.  So
> 
> (parallel
>  (unspec stack_tie)
>  (clobber (mem:BLK ...)))

Written like this, without a "set", *every* unspec has to be an
unspec_volatile, for the same reason as all inline asms without outputs
always are considered volatile asm.  The "unspec" arm of the parallel
can be omitted, and if that is valid RTL (possibly after other changes,
like omitting the parallel,replacing it by its one remaining arm), this
is a prefectly valid transformation.

> I suppose it needs to be an unspec_volatile?  It feels like
> the stack_ties are a delicate hack preventing enough but not too
> much optimization ...

Yes.  So let's please not disturb it :-)

It isn't a "delicate" hack I would say, but its effects are needed in
some places, and messing it up leads to hard to debug problems.  Which
had happened time and time again over the years.

It just is hard to deal with variable sized stack adjustments and the
like.  As long as we only use stack ties in such unusual cases, all is
fine.  There are worse things, like what we have the
frame_pointer_needed_indeed thing for :-)


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

On Wed, Jun 14, 2023 at 09:22:09AM +0000, Richard Biener wrote:
> How can a clobber be validly dropped?

Same as any other set: if no code executed after it can read whatever is
written.  This typically means a stack frame goes away, or simply no
more code is executed *at all* after this.

> For the case of stack
> memory if there's no stack use after it it could be elided
> and I suppose the clobber itself can be moved.  But then
> the function return is a stack use as well.

A function return does not access the stack at all on most
architectures, including PowerPC.  Some epilogue insns can do, of
course, but we expand to separate insns during expand already.


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Richard Sandiford <richard.sandiford@arm.com> writes:

> Richard Biener <rguenther@suse.de> writes:
>> AFAIU this special instruction is only supposed to prevent
>> code motion (of stack memory accesses?) across this instruction?
>> I'd say a
>>
>>   (may_clobber (mem:BLK (reg:DI 1 1)))
>>
>> might be more to the point?  I've used "may_clobber" which doesn't
>> exist since I'm not sure whether a clobber is considered a kill.
>> The docs say "Represents the storing or possible storing of an 
>> unpredictable..." - what is it? Storing or possible storing?
>
> I'd also understood it to be either.  As in, it is a may-clobber
> that can be used for must-clobber.  Alternatively: the value stored
> is unpredictable, and can therefore be the same as the current value.
>
> I think the main difference between:
>
>   (clobber (mem:BLK …))
>
> and
>
>   (set (mem:BLK …) (unspec:BLK …))
>
> is that the latter must happen for correctness (unless something
> that understands the unspec proves otherwise) whereas a clobber
> can validly be dropped.  So for something like stack_tie, a set
> seems more correct than a clobber.

Thanks a lot for all your helpful comments!

BR,
Jeff (Jiufu Guo)

>
> Thanks,
> Richard

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Wed, Jun 14, 2023 at 10:04:20AM +0100, Richard Sandiford wrote:
> I'd also understood it to be either.  As in, it is a may-clobber
> that can be used for must-clobber.  Alternatively: the value stored
> is unpredictable, and can therefore be the same as the current value.

Yes, it is a set with an unspecified RHS.

> I think the main difference between:
> 
>   (clobber (mem:BLK …))
> 
> and
> 
>   (set (mem:BLK …) (unspec:BLK …))
> 
> is that the latter must happen for correctness (unless something
> that understands the unspec proves otherwise) whereas a clobber
> can validly be dropped.  So for something like stack_tie, a set
> seems more correct than a clobber.

No, the latter can be removed as well, under exactly the same
conditions: if no code after it reads what was written.  This happens in
branches marked dead.


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Richard Biener <rguenther@suse.de> writes:

> On Wed, 14 Jun 2023, Jiufu Guo wrote:
>
>> 
>> Hi,
>> 
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> 
>> > Hi!
>> >
>> > As I said in a reply to the original patch: not okay.  Sorry.
>> 
>> Thanks a lot for your comments!
>> I'm also thinking about other solutions:
>> 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])"
>>   This is the existing pattern.  It may be read as an action
>>   to clean an unknown-size memory block.
>> 
>> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
>> UNSPEC_TIE".
>>   Current patch is using this one.
>> 
>> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
>> UNSPEC_TIE".
>>    This avoids using BLK on unspec, but using DI.
>
> That gives the MEM a size which means we can interpret the (set ..)
> as killing a specific area of memory, enabling DSE of earlier
> stores.

Oh, thanks!
While with 'unspec:DI', I'm wondering if it means this 'set' would
do some special things other than pure 'set' to the memory. 

BR,
Jeff (Jiufu Guo)

>
> AFAIU this special instruction is only supposed to prevent
> code motion (of stack memory accesses?) across this instruction?
> I'd say a
>
>   (may_clobber (mem:BLK (reg:DI 1 1)))
>
> might be more to the point?  I've used "may_clobber" which doesn't
> exist since I'm not sure whether a clobber is considered a kill.
> The docs say "Represents the storing or possible storing of an 
> unpredictable..." - what is it?  Storing or possible storing?
> I suppose stack_tie should be less strict than the documented
> (clobber (mem:BLK (const_int 0))) (clobber all memory).
>
> ?
>
>> 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0])
>> UNSPEC_TIE"
>>    There is still a mode for the unspec.
>> 
>> 
>> >
>> > But some comments on this patch:
>> >
>> > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote:
>> >> +	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
>> >> +	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
>> >
>> > This makes it required that the operand of an UNSPEC_TIE unspec is a
>> > const_int 0.  This should be documented somewhere.  Ideally you would
>> > want no operand at all here, but every unspec has an operand.
>> 
>> Right!  Since checked UNSPEC_TIE arleady, we may not need to check
>> the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);".
>> 
>> >
>> >> +      RTVEC_ELT (p, i)
>> >> +	= gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx),
>> >> +					    UNSPEC_TIE));
>> >
>> > If it is hard to indent your code, your code is trying to do to much.
>> > Just have an extra temporary?
>> >
>> >       rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), UNSPEC_TIE);
>> >       RTVEC_ELT (p, i) = gen_rtx_SET (mem, un);
>> >
>> > That is shorter even, and certainly more readable :-)
>> 
>> Yeap, thanks!
>> 
>> >
>> >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
>> >>    operands[4] = gen_frame_mem (Pmode, operands[1]);
>> >>    p = rtvec_alloc (1);
>> >>    RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
>> >> -				  const0_rtx);
>> >> +				  gen_rtx_UNSPEC (BLKmode,
>> >> +						  gen_rtvec (1, const0_rtx),
>> >> +						  UNSPEC_TIE));
>> >>    operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
>> >
>> > I have a hard time to see how this could ever be seen as clearer or more
>> > obvious or anything like that :-(
>> 
>> I was thinking about just invoking gen_stack_tie here.
>> 
>> BR,
>> Jeff (Jiufu Guo)
>> 
>> >
>> >
>> > Segher
>> 

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Wed, Jun 14, 2023 at 05:26:52PM +0800, Jiufu Guo wrote:
> Richard Biener <rguenther@suse.de> writes:
> >> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
> >> UNSPEC_TIE".
> >>    This avoids using BLK on unspec, but using DI.
> >
> > That gives the MEM a size which means we can interpret the (set ..)
> > as killing a specific area of memory, enabling DSE of earlier
> > stores.
> 
> Oh, thanks!
> While with 'unspec:DI', I'm wondering if it means this 'set' would
> do some special things other than pure 'set' to the memory. 

No, that is not what unspec means.  It just means "some DImode value I'm
not telling you anything about".  If to get that value there is some
important work done (that should not be oprimised away, say) you need
unspec_volatile, which means just that: there is an unspecified side
effect done by that insn, so it has to be done on the real machine
exactly like on the abstract C machine, so the insn has big restrictions
on being moved and removed etc.

We can replace the RHS of (almost) *every* set with an unspec, and the
compiler would still work, just would generate pretty lousy code.  But
at least CSE and DSE (and everything else purely dataflow) would still
work :-)


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Wed, Jun 14, 2023 at 07:59:04AM +0000, Richard Biener wrote:
> On Wed, 14 Jun 2023, Jiufu Guo wrote:
> > 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
> > UNSPEC_TIE".
> >    This avoids using BLK on unspec, but using DI.
> 
> That gives the MEM a size which means we can interpret the (set ..)
> as killing a specific area of memory, enabling DSE of earlier
> stores.

Or DSE can delete this tie even, if it can see some later store to the
same location without anything in between that can read what the tie
stores.

BLKmode avoids all of this.  You can call that elegant, you can call it
cheating, you can call it many things -- but it *works*.

> AFAIU this special instruction is only supposed to prevent
> code motion (of stack memory accesses?) across this instruction?

Form rs6000.md:
; This is to explain that changes to the stack pointer should
; not be moved over loads from or stores to stack memory.
(define_insn "stack_tie"

and from rs6000-logue.cc:
/* This ties together stack memory (MEM with an alias set of frame_alias_set)
   and the change to the stack pointer.  */
static void
rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)

A big reason this is needed is because of all the hard frame pointer
stuff, which the generic parts of GCC require, but there is no register
for that in the Power architecture.  Nothing is an issue here in most
cases, but sometimes we need to do unusual things to the stack, say for
alloca.

> I'd say a
> 
>   (may_clobber (mem:BLK (reg:DI 1 1)))

"clobber" always means "may clobber".  (clobber X) means X is written
with some unspecified value, which may well be whatever value it
currently holds.  Via some magical means or whatever, there is no
mechanism specified, just the effects :-)

> might be more to the point?  I've used "may_clobber" which doesn't
> exist since I'm not sure whether a clobber is considered a kill.
> The docs say "Represents the storing or possible storing of an 
> unpredictable..." - what is it?  Storing or possible storing?

It is the same thing.  "clobber" means the same thing as "set", except
the value that is written is not specified.

> I suppose stack_tie should be less strict than the documented
> (clobber (mem:BLK (const_int 0))) (clobber all memory).

"clobber" is nicer than the set to (const_int 0).  Does it work though?
All this code is always fragile :-/  I'm all for this change, don't get
me wrong, but preferably things stay in working order.

We use "stack_tie" as a last resort heavy hammer anyway, in all normal
cases we explain the actual data flow explicitly and correctly, also
between the various registers used in the *logues.


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Richard Biener]

> Am 14.06.2023 um 17:41 schrieb Segher Boessenkool <segher@kernel.crashing.org>:
> 
> Hi!
> 
>> On Wed, Jun 14, 2023 at 07:59:04AM +0000, Richard Biener wrote:
>>> On Wed, 14 Jun 2023, Jiufu Guo wrote:
>>> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
>>> UNSPEC_TIE".
>>>   This avoids using BLK on unspec, but using DI.
>> 
>> That gives the MEM a size which means we can interpret the (set ..)
>> as killing a specific area of memory, enabling DSE of earlier
>> stores.
> 
> Or DSE can delete this tie even, if it can see some later store to the
> same location without anything in between that can read what the tie
> stores.
> 
> BLKmode avoids all of this.  You can call that elegant, you can call it
> cheating, you can call it many things -- but it *works*.
> 
>> AFAIU this special instruction is only supposed to prevent
>> code motion (of stack memory accesses?) across this instruction?
> 
> Form rs6000.md:
> ; This is to explain that changes to the stack pointer should
> ; not be moved over loads from or stores to stack memory.
> (define_insn "stack_tie"

That suggests it’s the hard register value that‘s protected, not the memory pointed to.  I suppose that means an unspec volatile with the reg as input would serve the same?

Or maybe that’s not the whole story.


> and from rs6000-logue.cc:
> /* This ties together stack memory (MEM with an alias set of frame_alias_set)
>   and the change to the stack pointer.  */
> static void
> rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)

I cannot make sense of that comment, but not sure if I really want to know …

> A big reason this is needed is because of all the hard frame pointer
> stuff, which the generic parts of GCC require, but there is no register
> for that in the Power architecture.  Nothing is an issue here in most
> cases, but sometimes we need to do unusual things to the stack, say for
> alloca.
> 
>> I'd say a
>> 
>>  (may_clobber (mem:BLK (reg:DI 1 1)))
> 
> "clobber" always means "may clobber".  (clobber X) means X is written
> with some unspecified value, which may well be whatever value it
> currently holds.  Via some magical means or whatever, there is no
> mechanism specified, just the effects :-)
> 
>> might be more to the point?  I've used "may_clobber" which doesn't
>> exist since I'm not sure whether a clobber is considered a kill.
>> The docs say "Represents the storing or possible storing of an 
>> unpredictable..." - what is it?  Storing or possible storing?
> 
> It is the same thing.  "clobber" means the same thing as "set", except
> the value that is written is not specified.
> 
>> I suppose stack_tie should be less strict than the documented
>> (clobber (mem:BLK (const_int 0))) (clobber all memory).
> 
> "clobber" is nicer than the set to (const_int 0).  Does it work though?
> All this code is always fragile :-/  I'm all for this change, don't get
> me wrong, but preferably things stay in working order.
> 
> We use "stack_tie" as a last resort heavy hammer anyway, in all normal
> cases we explain the actual data flow explicitly and correctly, also
> between the various registers used in the *logues.
> 
> 
> Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

On Wed, Jun 14, 2023 at 06:25:10PM +0200, Richard Biener wrote:
> > Form rs6000.md:
> > ; This is to explain that changes to the stack pointer should
> > ; not be moved over loads from or stores to stack memory.
> > (define_insn "stack_tie"
> 
> That suggests it’s the hard register value that‘s protected, not the memory pointed to.  I suppose that means an unspec volatile with the reg as input would serve the same?

No?  It says what it says.  That is pretty vague language, of course,
not entirely by accident no doubt.

> Or maybe that’s not the whole story.
> 
> 
> > and from rs6000-logue.cc:
> > /* This ties together stack memory (MEM with an alias set of frame_alias_set)
> >   and the change to the stack pointer.  */
> > static void
> > rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)
> 
> I cannot make sense of that comment, but not sure if I really want to know …

It really is the same thing: this is a bloody heavy hammer keeping the
change to the stack pointer (or "hard" frame pointer) in place wrt any
accesses to the stack memory.

If there was a nice portable way to avoid needing this we haven't found
it yet -- or a non-portable way even, and it doesn't have to be all that
nice either come to think of it :-)


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

Hi!

On Wed, Jun 14, 2023 at 12:06:29PM +0800, Jiufu Guo wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> I'm also thinking about other solutions:
> 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])"
>   This is the existing pattern.  It may be read as an action
>   to clean an unknown-size memory block.

Including a size zero memory block, yes.  BLKmode was originally to do
things like bcopy (before modern names like memcpy were more usually
used), and those very much need size zero as well.

> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
> UNSPEC_TIE".
>   Current patch is using this one.

What would be the semantics of that?  Just the same as the current stuff
I'd say, or less?  It cannot be more!

> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
> UNSPEC_TIE".
>    This avoids using BLK on unspec, but using DI.

And is incorrect because of that.

> 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0])
> UNSPEC_TIE"
>    There is still a mode for the unspec.

It has VOIDmode here, which is incorrect.

> > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote:
> >> +	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
> >> +	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
> >
> > This makes it required that the operand of an UNSPEC_TIE unspec is a
> > const_int 0.  This should be documented somewhere.  Ideally you would
> > want no operand at all here, but every unspec has an operand.
> 
> Right!  Since checked UNSPEC_TIE arleady, we may not need to check
> the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);".

Yes.  But we should write down somewhere (in a comment near the unspec
constant def for example) what the operand is -- so, "operand is usually
(const_int 0) because we have to put *something* there" or such.  The
clumsiness of this is enough for me to prefer some other solution
already ;-)


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Wed, Jun 14, 2023 at 12:06:29PM +0800, Jiufu Guo wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> I'm also thinking about other solutions:
>> 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])"
>>   This is the existing pattern.  It may be read as an action
>>   to clean an unknown-size memory block.
>
> Including a size zero memory block, yes.  BLKmode was originally to do
> things like bcopy (before modern names like memcpy were more usually
> used), and those very much need size zero as well.h

The size is possible to be zero.  No asm code needs to
be generated for "set 'const_int 0' to zero size memory"".
stack_tie does not generate any real code.  It seems ok :)

While, it may not be zero size mem.  This may be a concern.
This is one reason that I would like to have an unspec_tie.

Another reason is unspec:blk is used but various ports :) 

>
>> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
>> UNSPEC_TIE".
>>   Current patch is using this one.
>
> What would be the semantics of that?  Just the same as the current stuff
> I'd say, or less?  It cannot be more!

The semantic that I trying to achieve is "this is a special
insn, not only a normal set to unknown size mem".

As you explained before on 'unspec:DI', the unspec would
just decorate the set_src part: something DI value with
machine-specific operation.

But, since 'tie_operand' is checked for this insn.
If 'tie_operand' checks UNPSEC_TIE, then the insn
with UNPSEC_TIE is 'a special insn'.  Or interpret
the semantic of this insn as: this insn stack_ite
indicates "set/operate a zero size block".

Does this make sense?

>
>> 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0])
>> UNSPEC_TIE".
>>    This avoids using BLK on unspec, but using DI.
>
> And is incorrect because of that.
>
>> 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0])
>> UNSPEC_TIE"
>>    There is still a mode for the unspec.
>
> It has VOIDmode here, which is incorrect.
>
>> > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote:
>> >> +	  && XINT (SET_SRC (set), 1) == UNSPEC_TIE
>> >> +	  && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
>> >
>> > This makes it required that the operand of an UNSPEC_TIE unspec is a
>> > const_int 0.  This should be documented somewhere.  Ideally you would
>> > want no operand at all here, but every unspec has an operand.
>> 
>> Right!  Since checked UNSPEC_TIE arleady, we may not need to check
>> the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);".
>
> Yes.  But we should write down somewhere (in a comment near the unspec
> constant def for example) what the operand is -- so, "operand is usually
> (const_int 0) because we have to put *something* there" or such.  The
> clumsiness of this is enough for me to prefer some other solution
> already ;-)

Thanks a lot for your comments!

BR,
Jeff (Jiufu Guo)

>
>
> Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Segher Boessenkool]

On Thu, Jun 15, 2023 at 03:00:40PM +0800, Jiufu Guo wrote:
> >>   This is the existing pattern.  It may be read as an action
> >>   to clean an unknown-size memory block.
> >
> > Including a size zero memory block, yes.  BLKmode was originally to do
> > things like bcopy (before modern names like memcpy were more usually
> > used), and those very much need size zero as well.h
> 
> The size is possible to be zero.  No asm code needs to
> be generated for "set 'const_int 0' to zero size memory"".
> stack_tie does not generate any real code.  It seems ok :)
> 
> While, it may not be zero size mem.  This may be a concern.
> This is one reason that I would like to have an unspec_tie.

It very much *can* be a zero size mem, that is perfectly find for
mem:BLK.

> Another reason is unspec:blk is used but various ports :) 

unspec:BLK is undefined.  BLKmode is allowed on mem only.

> >> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
> >> UNSPEC_TIE".
> >>   Current patch is using this one.
> >
> > What would be the semantics of that?  Just the same as the current stuff
> > I'd say, or less?  It cannot be more!
> 
> The semantic that I trying to achieve is "this is a special
> insn, not only a normal set to unknown size mem".

What does that *mean*?  "Special instruction"?  What would what code do
for that?  What would the RTL mean?

> As you explained before on 'unspec:DI', the unspec would
> just decorate the set_src part: something DI value with
> machine-specific operation.

An unspec is an operation on its operands, giving some (in this case)
DImode value.  There is nothing special about that operation, it can be
optimised like any other, it's just not specified what exactly that
value is (to the generic compiler, the backend itself can very much
optimise stuff with it).

> But, since 'tie_operand' is checked for this insn.
> If 'tie_operand' checks UNPSEC_TIE, then the insn
> with UNPSEC_TIE is 'a special insn'.  Or interpret
> the semantic of this insn as: this insn stack_ite
> indicates "set/operate a zero size block".

tie_operand is a predicate.  The predicate of an insn has to return 1,
or the insn is not recognised.  You can do the same in insn conditions
always (in principle anyway).


Segher

Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie

[Jiufu Guo]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Thu, Jun 15, 2023 at 03:00:40PM +0800, Jiufu Guo wrote:
>> >>   This is the existing pattern.  It may be read as an action
>> >>   to clean an unknown-size memory block.
>> >
>> > Including a size zero memory block, yes.  BLKmode was originally to do
>> > things like bcopy (before modern names like memcpy were more usually
>> > used), and those very much need size zero as well.h
>> 
>> The size is possible to be zero.  No asm code needs to
>> be generated for "set 'const_int 0' to zero size memory"".
>> stack_tie does not generate any real code.  It seems ok :)
>> 
>> While, it may not be zero size mem.  This may be a concern.
>> This is one reason that I would like to have an unspec_tie.
>
> It very much *can* be a zero size mem, that is perfectly find for
> mem:BLK.

There is still one concern: how to distinguish stack_tie
from other insn.
For example, below fake pattern:
(define_insn "xx_cleanmem"
  [(parallel: [(set (mem:BLK (xxx)) (const_int 0))
               (XXX/use "const_int_operand" "n")])]...

To avoid this pattern to be recognized as 'stack_tie',
'unspec_tie' was came to mind. 

>
>> Another reason is unspec:blk is used but various ports :) 
>
> unspec:BLK is undefined.  BLKmode is allowed on mem only.
>
>> >> 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0])
>> >> UNSPEC_TIE".
>> >>   Current patch is using this one.
>> >
>> > What would be the semantics of that?  Just the same as the current stuff
>> > I'd say, or less?  It cannot be more!
>> 
>> The semantic that I trying to achieve is "this is a special
>> insn, not only a normal set to unknown size mem".
>
> What does that *mean*?  "Special instruction"?  What would what code do
> for that?  What would the RTL mean?
>
>> As you explained before on 'unspec:DI', the unspec would
>> just decorate the set_src part: something DI value with
>> machine-specific operation.
>
> An unspec is an operation on its operands, giving some (in this case)
> DImode value.  There is nothing special about that operation, it can be
> optimised like any other, it's just not specified what exactly that
> value is (to the generic compiler, the backend itself can very much
> optimise stuff with it).
>
>> But, since 'tie_operand' is checked for this insn.
>> If 'tie_operand' checks UNPSEC_TIE, then the insn
>> with UNPSEC_TIE is 'a special insn'.  Or interpret
>> the semantic of this insn as: this insn stack_ite
>> indicates "set/operate a zero size block".
>
> tie_operand is a predicate.  The predicate of an insn has to return 1,
> or the insn is not recognised.  You can do the same in insn conditions
> always (in principle anyway).

Thank you very much for your detailed and patient explanation!

BR,
Jeff (Jiufu Guo)

>
>
> Segher