[PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jiufu Guo]

Hi,

As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P (mode))"
in "try_const_anchors".
This assert seems correct because the function try_const_anchors cares
about integer values currently, and modes other than SCALAR_INT_MODE_P
are not needed to support.

This patch makes sure SCALAR_INT_MODE_P when calling try_const_anchors.

This patch is raised when drafting below one.
https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
try_const_anchors, and hits the assert/ice.

Boostrap and regtest pass on ppc64{,le} and x86_64.
Is this ok for trunk?


BR,
Jeff (Jiufu Guo)

gcc/ChangeLog:

	* cse.cc (cse_insn): Add SCALAR_INT_MODE_P condition.

---
 gcc/cse.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/cse.cc b/gcc/cse.cc
index 2bb63ac4105..f213fa0faf7 100644
*** a/gcc/cse.cc
--- b/gcc/cse.cc
***************
*** 5003,5009 ****
        if (targetm.const_anchor
  	  && !src_related
  	  && src_const
! 	  && GET_CODE (src_const) == CONST_INT)
  	{
  	  src_related = try_const_anchors (src_const, mode);
  	  src_related_is_const_anchor = src_related != NULL_RTX;
- - 
--- 5003,5010 ----
        if (targetm.const_anchor
  	  && !src_related
  	  && src_const
! 	  && GET_CODE (src_const) == CONST_INT
! 	  && SCALAR_INT_MODE_P (mode))
  	{
  	  src_related = try_const_anchors (src_const, mode);
  	  src_related_is_const_anchor = src_related != NULL_RTX;
2.39.3

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Richard Biener]

On Fri, 9 Jun 2023, Jiufu Guo wrote:

> Hi,
> 
> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P (mode))"
> in "try_const_anchors".
> This assert seems correct because the function try_const_anchors cares
> about integer values currently, and modes other than SCALAR_INT_MODE_P
> are not needed to support.
> 
> This patch makes sure SCALAR_INT_MODE_P when calling try_const_anchors.
> 
> This patch is raised when drafting below one.
> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
> try_const_anchors, and hits the assert/ice.
> 
> Boostrap and regtest pass on ppc64{,le} and x86_64.
> Is this ok for trunk?

Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
I suggest to instead fix try_const_anchors to change

  /* CONST_INT is used for CC modes, but we should leave those alone.  */
  if (GET_MODE_CLASS (mode) == MODE_CC)
    return NULL_RTX;

  gcc_assert (SCALAR_INT_MODE_P (mode));

to

  /* CONST_INT is used for CC modes, leave any non-scalar-int mode alone.  */
  if (!SCALAR_INT_MODE_P (mode))
    return NULL_RTX;

but as said I wonder how we arrive at a BLKmode CONST_INT and whether
we should have fended this off earlier.  Can you share more complete
RTL of that stack_tie?

> 
> BR,
> Jeff (Jiufu Guo)
> 
> gcc/ChangeLog:
> 
> 	* cse.cc (cse_insn): Add SCALAR_INT_MODE_P condition.
> 
> ---
>  gcc/cse.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index 2bb63ac4105..f213fa0faf7 100644
> *** a/gcc/cse.cc
> --- b/gcc/cse.cc
> ***************
> *** 5003,5009 ****
>         if (targetm.const_anchor
>   	  && !src_related
>   	  && src_const
> ! 	  && GET_CODE (src_const) == CONST_INT)
>   	{
>   	  src_related = try_const_anchors (src_const, mode);
>   	  src_related_is_const_anchor = src_related != NULL_RTX;
> - - 
> --- 5003,5010 ----
>         if (targetm.const_anchor
>   	  && !src_related
>   	  && src_const
> ! 	  && GET_CODE (src_const) == CONST_INT
> ! 	  && SCALAR_INT_MODE_P (mode))
>   	{
>   	  src_related = try_const_anchors (src_const, mode);
>   	  src_related_is_const_anchor = src_related != NULL_RTX;
> 2.39.3
> 
> 

-- 
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] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[guojiufu]

Hi,

On 2023-06-09 16:00, Richard Biener wrote:
> On Fri, 9 Jun 2023, Jiufu Guo wrote:
> 
>> Hi,
>> 
>> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P 
>> (mode))"
>> in "try_const_anchors".
>> This assert seems correct because the function try_const_anchors cares
>> about integer values currently, and modes other than SCALAR_INT_MODE_P
>> are not needed to support.
>> 
>> This patch makes sure SCALAR_INT_MODE_P when calling 
>> try_const_anchors.
>> 
>> This patch is raised when drafting below one.
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>> try_const_anchors, and hits the assert/ice.
>> 
>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>> Is this ok for trunk?
> 
> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
> I suggest to instead fix try_const_anchors to change
> 
>   /* CONST_INT is used for CC modes, but we should leave those alone.  
> */
>   if (GET_MODE_CLASS (mode) == MODE_CC)
>     return NULL_RTX;
> 
>   gcc_assert (SCALAR_INT_MODE_P (mode));
> 
> to
> 
>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
> alone.  */
>   if (!SCALAR_INT_MODE_P (mode))
>     return NULL_RTX;
> 

This is also able to fix this issue.  there is a "Punt on CC modes" 
patch
to return NULL_RTX in try_const_anchors.

> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
> we should have fended this off earlier.  Can you share more complete
> RTL of that stack_tie?


(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 is generated by:

rs6000.md
(define_expand "restore_stack_block"
   [(set (match_dup 2) (match_dup 3))
    (set (match_dup 4) (match_dup 2))
    (match_dup 5)
    (set (match_operand 0 "register_operand")
         (match_operand 1 "register_operand"))]
   ""
{
   rtvec p;

   operands[1] = force_reg (Pmode, operands[1]);
   operands[2] = gen_reg_rtx (Pmode);
   operands[3] = gen_frame_mem (Pmode, operands[0]);
   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);
   operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
})

This kind of case (like BLK with const0) is rare, but this would be an 
intended
RTL, and seems not invalid.

Thanks so much for your quick and very helpful comments!!

BR,
Jeff (Jiufu Guo)


> 
>> 
>> BR,
>> Jeff (Jiufu Guo)
>> 
>> gcc/ChangeLog:
>> 
>> 	* cse.cc (cse_insn): Add SCALAR_INT_MODE_P condition.
>> 
>> ---
>>  gcc/cse.cc | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> index 2bb63ac4105..f213fa0faf7 100644
>> *** a/gcc/cse.cc
>> --- b/gcc/cse.cc
>> ***************
>> *** 5003,5009 ****
>>         if (targetm.const_anchor
>>   	  && !src_related
>>   	  && src_const
>> ! 	  && GET_CODE (src_const) == CONST_INT)
>>   	{
>>   	  src_related = try_const_anchors (src_const, mode);
>>   	  src_related_is_const_anchor = src_related != NULL_RTX;
>> - -
>> --- 5003,5010 ----
>>         if (targetm.const_anchor
>>   	  && !src_related
>>   	  && src_const
>> ! 	  && GET_CODE (src_const) == CONST_INT
>> ! 	  && SCALAR_INT_MODE_P (mode))
>>   	{
>>   	  src_related = try_const_anchors (src_const, mode);
>>   	  src_related_is_const_anchor = src_related != NULL_RTX;
>> 2.39.3
>> 
>> 

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Richard Sandiford]

guojiufu <guojiufu@linux.ibm.com> writes:
> Hi,
>
> On 2023-06-09 16:00, Richard Biener wrote:
>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> 
>>> Hi,
>>> 
>>> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P 
>>> (mode))"
>>> in "try_const_anchors".
>>> This assert seems correct because the function try_const_anchors cares
>>> about integer values currently, and modes other than SCALAR_INT_MODE_P
>>> are not needed to support.
>>> 
>>> This patch makes sure SCALAR_INT_MODE_P when calling 
>>> try_const_anchors.
>>> 
>>> This patch is raised when drafting below one.
>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>>> try_const_anchors, and hits the assert/ice.
>>> 
>>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>>> Is this ok for trunk?
>> 
>> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>> I suggest to instead fix try_const_anchors to change
>> 
>>   /* CONST_INT is used for CC modes, but we should leave those alone.  
>> */
>>   if (GET_MODE_CLASS (mode) == MODE_CC)
>>     return NULL_RTX;
>> 
>>   gcc_assert (SCALAR_INT_MODE_P (mode));
>> 
>> to
>> 
>>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
>> alone.  */
>>   if (!SCALAR_INT_MODE_P (mode))
>>     return NULL_RTX;
>> 
>
> This is also able to fix this issue.  there is a "Punt on CC modes" 
> patch
> to return NULL_RTX in try_const_anchors.
>
>> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>> we should have fended this off earlier.  Can you share more complete
>> RTL of that stack_tie?
>
>
> (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])".

I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
would be though.  It's arguably more accurate too, since the effect
on the stack locations is unspecified rather than predictable.

Thanks,
Richard

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Richard Biener]

On Fri, 9 Jun 2023, Richard Sandiford wrote:

> guojiufu <guojiufu@linux.ibm.com> writes:
> > Hi,
> >
> > On 2023-06-09 16:00, Richard Biener wrote:
> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >> 
> >>> Hi,
> >>> 
> >>> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P 
> >>> (mode))"
> >>> in "try_const_anchors".
> >>> This assert seems correct because the function try_const_anchors cares
> >>> about integer values currently, and modes other than SCALAR_INT_MODE_P
> >>> are not needed to support.
> >>> 
> >>> This patch makes sure SCALAR_INT_MODE_P when calling 
> >>> try_const_anchors.
> >>> 
> >>> This patch is raised when drafting below one.
> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
> >>> try_const_anchors, and hits the assert/ice.
> >>> 
> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
> >>> Is this ok for trunk?
> >> 
> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
> >> I suggest to instead fix try_const_anchors to change
> >> 
> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
> >> */
> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
> >>     return NULL_RTX;
> >> 
> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
> >> 
> >> to
> >> 
> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
> >> alone.  */
> >>   if (!SCALAR_INT_MODE_P (mode))
> >>     return NULL_RTX;
> >> 
> >
> > This is also able to fix this issue.  there is a "Punt on CC modes" 
> > patch
> > to return NULL_RTX in try_const_anchors.
> >
> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
> >> we should have fended this off earlier.  Can you share more complete
> >> RTL of that stack_tie?
> >
> >
> > (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])".
> 
> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
> would be though.  It's arguably more accurate too, since the effect
> on the stack locations is unspecified rather than predictable.

powerpc seems to be the only port with a stack_tie that's not
using an UNSPEC RHS.

> Thanks,
> Richard

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jiufu Guo]

Hi,

Richard Biener <rguenther@suse.de> writes:

> On Fri, 9 Jun 2023, Richard Sandiford wrote:
>
>> guojiufu <guojiufu@linux.ibm.com> writes:
>> > Hi,
>> >
>> > On 2023-06-09 16:00, Richard Biener wrote:
>> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >> 
>> >>> Hi,
>> >>> 
>> >>> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P 
>> >>> (mode))"
>> >>> in "try_const_anchors".
>> >>> This assert seems correct because the function try_const_anchors cares
>> >>> about integer values currently, and modes other than SCALAR_INT_MODE_P
>> >>> are not needed to support.
>> >>> 
>> >>> This patch makes sure SCALAR_INT_MODE_P when calling 
>> >>> try_const_anchors.
>> >>> 
>> >>> This patch is raised when drafting below one.
>> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>> >>> try_const_anchors, and hits the assert/ice.
>> >>> 
>> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>> >>> Is this ok for trunk?
>> >> 
>> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>> >> I suggest to instead fix try_const_anchors to change
>> >> 
>> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
>> >> */
>> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
>> >>     return NULL_RTX;
>> >> 
>> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
>> >> 
>> >> to
>> >> 
>> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
>> >> alone.  */
>> >>   if (!SCALAR_INT_MODE_P (mode))
>> >>     return NULL_RTX;
>> >> 
>> >
>> > This is also able to fix this issue.  there is a "Punt on CC modes" 
>> > patch
>> > to return NULL_RTX in try_const_anchors.
>> >
>> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>> >> we should have fended this off earlier.  Can you share more complete
>> >> RTL of that stack_tie?
>> >
>> >
>> > (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])".
>> 
>> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
>> would be though.  It's arguably more accurate too, since the effect
>> on the stack locations is unspecified rather than predictable.
>
> powerpc seems to be the only port with a stack_tie that's not
> using an UNSPEC RHS.
In rs6000.md, it is

; 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"
  [(match_parallel 0 "tie_operand"
		   [(set (mem:BLK (reg 1)) (const_int 0))])]
  ""
  ""
  [(set_attr "length" "0")])

This would be just an placeholder insn, and acts as the comments.
UNSPEC_ would works like other targets.  While, I'm wondering
the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
MODEs between SET_DEST and SET_SRC?

Thanks for comments!

BR,
Jeff (Jiufu Guo)
>
>> Thanks,
>> Richard

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Richard Biener]

On Fri, 9 Jun 2023, Jiufu Guo wrote:

> 
> Hi,
> 
> Richard Biener <rguenther@suse.de> writes:
> 
> > On Fri, 9 Jun 2023, Richard Sandiford wrote:
> >
> >> guojiufu <guojiufu@linux.ibm.com> writes:
> >> > Hi,
> >> >
> >> > On 2023-06-09 16:00, Richard Biener wrote:
> >> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >> >> 
> >> >>> Hi,
> >> >>> 
> >> >>> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P 
> >> >>> (mode))"
> >> >>> in "try_const_anchors".
> >> >>> This assert seems correct because the function try_const_anchors cares
> >> >>> about integer values currently, and modes other than SCALAR_INT_MODE_P
> >> >>> are not needed to support.
> >> >>> 
> >> >>> This patch makes sure SCALAR_INT_MODE_P when calling 
> >> >>> try_const_anchors.
> >> >>> 
> >> >>> This patch is raised when drafting below one.
> >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
> >> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
> >> >>> try_const_anchors, and hits the assert/ice.
> >> >>> 
> >> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
> >> >>> Is this ok for trunk?
> >> >> 
> >> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
> >> >> I suggest to instead fix try_const_anchors to change
> >> >> 
> >> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
> >> >> */
> >> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
> >> >>     return NULL_RTX;
> >> >> 
> >> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
> >> >> 
> >> >> to
> >> >> 
> >> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
> >> >> alone.  */
> >> >>   if (!SCALAR_INT_MODE_P (mode))
> >> >>     return NULL_RTX;
> >> >> 
> >> >
> >> > This is also able to fix this issue.  there is a "Punt on CC modes" 
> >> > patch
> >> > to return NULL_RTX in try_const_anchors.
> >> >
> >> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
> >> >> we should have fended this off earlier.  Can you share more complete
> >> >> RTL of that stack_tie?
> >> >
> >> >
> >> > (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])".
> >> 
> >> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
> >> would be though.  It's arguably more accurate too, since the effect
> >> on the stack locations is unspecified rather than predictable.
> >
> > powerpc seems to be the only port with a stack_tie that's not
> > using an UNSPEC RHS.
> In rs6000.md, it is
> 
> ; 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"
>   [(match_parallel 0 "tie_operand"
> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
>   ""
>   ""
>   [(set_attr "length" "0")])
> 
> This would be just an placeholder insn, and acts as the comments.
> UNSPEC_ would works like other targets.  While, I'm wondering
> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
> MODEs between SET_DEST and SET_SRC?

I don't think the issue is the mode but the issue is that
the patter as-is says some memory is zeroed while that's not
actually true (not specifying a size means we can't really do
anything with this MEM, but still).  Using an UNSPEC avoids
implying anything for the stored value.

Of course I think a MEM SET_DEST without a specified size is bougs
as well, but there's larger precedent for this...

Richard.

> Thanks for comments!
> 
> BR,
> Jeff (Jiufu Guo)
> >
> >> 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] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jiufu Guo]

Hi,

Richard Biener <rguenther@suse.de> writes:

> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>
>> 
>> Hi,
>> 
>> Richard Biener <rguenther@suse.de> writes:
>> 
>> > On Fri, 9 Jun 2023, Richard Sandiford wrote:
>> >
>> >> guojiufu <guojiufu@linux.ibm.com> writes:
>> >> > Hi,
>> >> >
>> >> > On 2023-06-09 16:00, Richard Biener wrote:
>> >> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >> >> 
>> >> >>> Hi,
>> >> >>> 
...
>> >> >>> 
>> >> >>> This patch is raised when drafting below one.
>> >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>> >> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>> >> >>> try_const_anchors, and hits the assert/ice.
>> >> >>> 
>> >> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>> >> >>> Is this ok for trunk?
>> >> >> 
>> >> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>> >> >> I suggest to instead fix try_const_anchors to change
>> >> >> 
>> >> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
>> >> >> */
>> >> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
>> >> >>     return NULL_RTX;
>> >> >> 
>> >> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
>> >> >> 
>> >> >> to
>> >> >> 
>> >> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
>> >> >> alone.  */
>> >> >>   if (!SCALAR_INT_MODE_P (mode))
>> >> >>     return NULL_RTX;
>> >> >> 
>> >> >
>> >> > This is also able to fix this issue.  there is a "Punt on CC modes" 
>> >> > patch
>> >> > to return NULL_RTX in try_const_anchors.
>> >> >
>> >> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>> >> >> we should have fended this off earlier.  Can you share more complete
>> >> >> RTL of that stack_tie?
>> >> >
>> >> >
>> >> > (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])".
>> >> 
>> >> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
>> >> would be though.  It's arguably more accurate too, since the effect
>> >> on the stack locations is unspecified rather than predictable.
>> >
>> > powerpc seems to be the only port with a stack_tie that's not
>> > using an UNSPEC RHS.
>> In rs6000.md, it is
>> 
>> ; 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"
>>   [(match_parallel 0 "tie_operand"
>> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
>>   ""
>>   ""
>>   [(set_attr "length" "0")])
>> 
>> This would be just an placeholder insn, and acts as the comments.
>> UNSPEC_ would works like other targets.  While, I'm wondering
>> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
>> MODEs between SET_DEST and SET_SRC?
>
> I don't think the issue is the mode but the issue is that
> the patter as-is says some memory is zeroed while that's not
> actually true (not specifying a size means we can't really do
> anything with this MEM, but still).  Using an UNSPEC avoids
> implying anything for the stored value.
>
> Of course I think a MEM SET_DEST without a specified size is bougs
> as well, but there's larger precedent for this...

Thanks for your kindly comments!
Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
insn does not generate real thing (not a real store and no asm code),
may like barrier.

While I agree that, using UNSPEC may be more clear to avoid mis-reading.

BR,
Jeff (Jiufu Guo)

>
> Richard.
>
>> Thanks for comments!
>> 
>> BR,
>> Jeff (Jiufu Guo)
>> >
>> >> Thanks,
>> >> Richard
>> 

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Richard Biener]

On Fri, 9 Jun 2023, Jiufu Guo wrote:

> 
> Hi,
> 
> Richard Biener <rguenther@suse.de> writes:
> 
> > On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >
> >> 
> >> Hi,
> >> 
> >> Richard Biener <rguenther@suse.de> writes:
> >> 
> >> > On Fri, 9 Jun 2023, Richard Sandiford wrote:
> >> >
> >> >> guojiufu <guojiufu@linux.ibm.com> writes:
> >> >> > Hi,
> >> >> >
> >> >> > On 2023-06-09 16:00, Richard Biener wrote:
> >> >> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >> >> >> 
> >> >> >>> Hi,
> >> >> >>> 
> ...
> >> >> >>> 
> >> >> >>> This patch is raised when drafting below one.
> >> >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
> >> >> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
> >> >> >>> try_const_anchors, and hits the assert/ice.
> >> >> >>> 
> >> >> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
> >> >> >>> Is this ok for trunk?
> >> >> >> 
> >> >> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
> >> >> >> I suggest to instead fix try_const_anchors to change
> >> >> >> 
> >> >> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
> >> >> >> */
> >> >> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
> >> >> >>     return NULL_RTX;
> >> >> >> 
> >> >> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
> >> >> >> 
> >> >> >> to
> >> >> >> 
> >> >> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
> >> >> >> alone.  */
> >> >> >>   if (!SCALAR_INT_MODE_P (mode))
> >> >> >>     return NULL_RTX;
> >> >> >> 
> >> >> >
> >> >> > This is also able to fix this issue.  there is a "Punt on CC modes" 
> >> >> > patch
> >> >> > to return NULL_RTX in try_const_anchors.
> >> >> >
> >> >> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
> >> >> >> we should have fended this off earlier.  Can you share more complete
> >> >> >> RTL of that stack_tie?
> >> >> >
> >> >> >
> >> >> > (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])".
> >> >> 
> >> >> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
> >> >> would be though.  It's arguably more accurate too, since the effect
> >> >> on the stack locations is unspecified rather than predictable.
> >> >
> >> > powerpc seems to be the only port with a stack_tie that's not
> >> > using an UNSPEC RHS.
> >> In rs6000.md, it is
> >> 
> >> ; 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"
> >>   [(match_parallel 0 "tie_operand"
> >> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
> >>   ""
> >>   ""
> >>   [(set_attr "length" "0")])
> >> 
> >> This would be just an placeholder insn, and acts as the comments.
> >> UNSPEC_ would works like other targets.  While, I'm wondering
> >> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
> >> MODEs between SET_DEST and SET_SRC?
> >
> > I don't think the issue is the mode but the issue is that
> > the patter as-is says some memory is zeroed while that's not
> > actually true (not specifying a size means we can't really do
> > anything with this MEM, but still).  Using an UNSPEC avoids
> > implying anything for the stored value.
> >
> > Of course I think a MEM SET_DEST without a specified size is bougs
> > as well, but there's larger precedent for this...
> 
> Thanks for your kindly comments!
> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
> insn does not generate real thing (not a real store and no asm code),
> may like barrier.
> 
> While I agree that, using UNSPEC may be more clear to avoid mis-reading.

Btw, another way to avoid the issue in CSE is to make it not process
(aka record anything for optimization) for SET from MEMs with
!MEM_SIZE_KNOWN_P

Richard.

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jiufu Guo]

Richard Biener <rguenther@suse.de> writes:

> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>
>> 
>> Hi,
>> 
>> Richard Biener <rguenther@suse.de> writes:
>> 
>> > On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >
>> >> 
>> >> Hi,
>> >> 
>> >> Richard Biener <rguenther@suse.de> writes:
>> >> 
>> >> > On Fri, 9 Jun 2023, Richard Sandiford wrote:
>> >> >
>> >> >> guojiufu <guojiufu@linux.ibm.com> writes:
>> >> >> > Hi,
>> >> >> >
>> >> >> > On 2023-06-09 16:00, Richard Biener wrote:
>> >> >> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >> >> >> 
>> >> >> >>> Hi,
>> >> >> >>> 
>> ...
>> >> >> >>> 
>> >> >> >>> This patch is raised when drafting below one.
>> >> >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>> >> >> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>> >> >> >>> try_const_anchors, and hits the assert/ice.
>> >> >> >>> 
>> >> >> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>> >> >> >>> Is this ok for trunk?
>> >> >> >> 
>> >> >> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>> >> >> >> I suggest to instead fix try_const_anchors to change
>> >> >> >> 
>> >> >> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
>> >> >> >> */
>> >> >> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
>> >> >> >>     return NULL_RTX;
>> >> >> >> 
>> >> >> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
>> >> >> >> 
>> >> >> >> to
>> >> >> >> 
>> >> >> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
>> >> >> >> alone.  */
>> >> >> >>   if (!SCALAR_INT_MODE_P (mode))
>> >> >> >>     return NULL_RTX;
>> >> >> >> 
>> >> >> >
>> >> >> > This is also able to fix this issue.  there is a "Punt on CC modes" 
>> >> >> > patch
>> >> >> > to return NULL_RTX in try_const_anchors.
>> >> >> >
>> >> >> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>> >> >> >> we should have fended this off earlier.  Can you share more complete
>> >> >> >> RTL of that stack_tie?
>> >> >> >
>> >> >> >
>> >> >> > (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])".
>> >> >> 
>> >> >> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
>> >> >> would be though.  It's arguably more accurate too, since the effect
>> >> >> on the stack locations is unspecified rather than predictable.
>> >> >
>> >> > powerpc seems to be the only port with a stack_tie that's not
>> >> > using an UNSPEC RHS.
>> >> In rs6000.md, it is
>> >> 
>> >> ; 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"
>> >>   [(match_parallel 0 "tie_operand"
>> >> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
>> >>   ""
>> >>   ""
>> >>   [(set_attr "length" "0")])
>> >> 
>> >> This would be just an placeholder insn, and acts as the comments.
>> >> UNSPEC_ would works like other targets.  While, I'm wondering
>> >> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
>> >> MODEs between SET_DEST and SET_SRC?
>> >
>> > I don't think the issue is the mode but the issue is that
>> > the patter as-is says some memory is zeroed while that's not
>> > actually true (not specifying a size means we can't really do
>> > anything with this MEM, but still).  Using an UNSPEC avoids
>> > implying anything for the stored value.
>> >
>> > Of course I think a MEM SET_DEST without a specified size is bougs
>> > as well, but there's larger precedent for this...
>> 
>> Thanks for your kindly comments!
>> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
>> insn does not generate real thing (not a real store and no asm code),
>> may like barrier.
>> 
>> While I agree that, using UNSPEC may be more clear to avoid mis-reading.
>
> Btw, another way to avoid the issue in CSE is to make it not process
> (aka record anything for optimization) for SET from MEMs with
> !MEM_SIZE_KNOWN_P

Thanks! Yes, this would make sense.
Then, there are two ideas(patches) to handle this issue:
Which one would be preferable?  This one (from compiling time aspect)?

And maybe, the changes in rs6000 stack_tie through using unspec
can be a standalone enhancement besides cse patch.

Thanks for comments!

BR,
Jeff (Jiufu Guo)

-------------------- patch 1
diff --git a/gcc/cse.cc b/gcc/cse.cc
index 2bb63ac4105..06ecdadecbc 100644
--- a/gcc/cse.cc
+++ b/gcc/cse.cc
@@ -4271,6 +4271,8 @@ find_sets_in_insn (rtx_insn *insn, vec<struct set> *psets)
 	 someplace else, so it isn't worth cse'ing.  */
       else if (GET_CODE (SET_SRC (x)) == CALL)
 	;
+      else if (MEM_P (SET_DEST (x)) && !MEM_SIZE_KNOWN_P (SET_DEST (x)))
+	;
       else if (GET_CODE (SET_SRC (x)) == CONST_VECTOR
 	       && GET_MODE_CLASS (GET_MODE (SET_SRC (x))) != MODE_VECTOR_BOOL
 	       /* Prevent duplicates from being generated if the type is a V1
@@ -4314,6 +4316,8 @@ find_sets_in_insn (rtx_insn *insn, vec<struct set> *psets)
 		;
 	      else if (GET_CODE (SET_SRC (y)) == CALL)
 		;
+	      else if (MEM_P (SET_DEST (y)) && !MEM_SIZE_KNOWN_P (SET_DEST (y)))
+		;
 	      else
 		add_to_set (psets, y);
 	    }
-----------------------------
-------------------patch 2
diff --git a/gcc/cse.cc b/gcc/cse.cc
index 2bb63ac4105..ddb76fd281d 100644
--- a/gcc/cse.cc
+++ b/gcc/cse.cc
@@ -1312,11 +1312,10 @@ try_const_anchors (rtx src_const, machine_mode mode)
   rtx lower_exp = NULL_RTX, upper_exp = NULL_RTX;
   unsigned lower_old, upper_old;
 
-  /* CONST_INT is used for CC modes, but we should leave those alone.  */
-  if (GET_MODE_CLASS (mode) == MODE_CC)
+  /* CONST_INT is used for CC/BLK modes, leave any non-scalar-int mode alone. */
+  if (!SCALAR_INT_MODE_P (mode))
     return NULL_RTX;
 
-  gcc_assert (SCALAR_INT_MODE_P (mode));
   if (!compute_const_anchors (src_const, &lower_base, &lower_offs,
 			      &upper_base, &upper_offs))
     return NULL_RTX;
-------------


BR,
Jeff (Jiufu Guo)
>
> Richard.

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Richard Biener]

On Mon, 12 Jun 2023, Jiufu Guo wrote:

> Richard Biener <rguenther@suse.de> writes:
> 
> > On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >
> >> 
> >> Hi,
> >> 
> >> Richard Biener <rguenther@suse.de> writes:
> >> 
> >> > On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >> >
> >> >> 
> >> >> Hi,
> >> >> 
> >> >> Richard Biener <rguenther@suse.de> writes:
> >> >> 
> >> >> > On Fri, 9 Jun 2023, Richard Sandiford wrote:
> >> >> >
> >> >> >> guojiufu <guojiufu@linux.ibm.com> writes:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On 2023-06-09 16:00, Richard Biener wrote:
> >> >> >> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
> >> >> >> >> 
> >> >> >> >>> Hi,
> >> >> >> >>> 
> >> ...
> >> >> >> >>> 
> >> >> >> >>> This patch is raised when drafting below one.
> >> >> >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
> >> >> >> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
> >> >> >> >>> try_const_anchors, and hits the assert/ice.
> >> >> >> >>> 
> >> >> >> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
> >> >> >> >>> Is this ok for trunk?
> >> >> >> >> 
> >> >> >> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
> >> >> >> >> I suggest to instead fix try_const_anchors to change
> >> >> >> >> 
> >> >> >> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
> >> >> >> >> */
> >> >> >> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
> >> >> >> >>     return NULL_RTX;
> >> >> >> >> 
> >> >> >> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
> >> >> >> >> 
> >> >> >> >> to
> >> >> >> >> 
> >> >> >> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
> >> >> >> >> alone.  */
> >> >> >> >>   if (!SCALAR_INT_MODE_P (mode))
> >> >> >> >>     return NULL_RTX;
> >> >> >> >> 
> >> >> >> >
> >> >> >> > This is also able to fix this issue.  there is a "Punt on CC modes" 
> >> >> >> > patch
> >> >> >> > to return NULL_RTX in try_const_anchors.
> >> >> >> >
> >> >> >> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
> >> >> >> >> we should have fended this off earlier.  Can you share more complete
> >> >> >> >> RTL of that stack_tie?
> >> >> >> >
> >> >> >> >
> >> >> >> > (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])".
> >> >> >> 
> >> >> >> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
> >> >> >> would be though.  It's arguably more accurate too, since the effect
> >> >> >> on the stack locations is unspecified rather than predictable.
> >> >> >
> >> >> > powerpc seems to be the only port with a stack_tie that's not
> >> >> > using an UNSPEC RHS.
> >> >> In rs6000.md, it is
> >> >> 
> >> >> ; 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"
> >> >>   [(match_parallel 0 "tie_operand"
> >> >> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
> >> >>   ""
> >> >>   ""
> >> >>   [(set_attr "length" "0")])
> >> >> 
> >> >> This would be just an placeholder insn, and acts as the comments.
> >> >> UNSPEC_ would works like other targets.  While, I'm wondering
> >> >> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
> >> >> MODEs between SET_DEST and SET_SRC?
> >> >
> >> > I don't think the issue is the mode but the issue is that
> >> > the patter as-is says some memory is zeroed while that's not
> >> > actually true (not specifying a size means we can't really do
> >> > anything with this MEM, but still).  Using an UNSPEC avoids
> >> > implying anything for the stored value.
> >> >
> >> > Of course I think a MEM SET_DEST without a specified size is bougs
> >> > as well, but there's larger precedent for this...
> >> 
> >> Thanks for your kindly comments!
> >> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
> >> insn does not generate real thing (not a real store and no asm code),
> >> may like barrier.
> >> 
> >> While I agree that, using UNSPEC may be more clear to avoid mis-reading.
> >
> > Btw, another way to avoid the issue in CSE is to make it not process
> > (aka record anything for optimization) for SET from MEMs with
> > !MEM_SIZE_KNOWN_P
> 
> Thanks! Yes, this would make sense.
> Then, there are two ideas(patches) to handle this issue:
> Which one would be preferable?  This one (from compiling time aspect)?
> 
> And maybe, the changes in rs6000 stack_tie through using unspec
> can be a standalone enhancement besides cse patch.
> 
> Thanks for comments!
> 
> BR,
> Jeff (Jiufu Guo)
> 
> -------------------- patch 1
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index 2bb63ac4105..06ecdadecbc 100644
> --- a/gcc/cse.cc
> +++ b/gcc/cse.cc
> @@ -4271,6 +4271,8 @@ find_sets_in_insn (rtx_insn *insn, vec<struct set> *psets)
>  	 someplace else, so it isn't worth cse'ing.  */
>        else if (GET_CODE (SET_SRC (x)) == CALL)
>  	;
> +      else if (MEM_P (SET_DEST (x)) && !MEM_SIZE_KNOWN_P (SET_DEST (x)))
> +	;

I don't know CSE enough to decide if this is correct, it depends on
whether pset is only used to record new sets or if it is also used
to invalidate prior recorded sets (in which case these sets have to
be recorded still).  That is, the fix might better be in the caller
who interprets 'pset'

>        else if (GET_CODE (SET_SRC (x)) == CONST_VECTOR
>  	       && GET_MODE_CLASS (GET_MODE (SET_SRC (x))) != MODE_VECTOR_BOOL
>  	       /* Prevent duplicates from being generated if the type is a V1
> @@ -4314,6 +4316,8 @@ find_sets_in_insn (rtx_insn *insn, vec<struct set> *psets)
>  		;
>  	      else if (GET_CODE (SET_SRC (y)) == CALL)
>  		;
> +	      else if (MEM_P (SET_DEST (y)) && !MEM_SIZE_KNOWN_P (SET_DEST (y)))
> +		;
>  	      else
>  		add_to_set (psets, y);
>  	    }
> -----------------------------
> -------------------patch 2
> diff --git a/gcc/cse.cc b/gcc/cse.cc
> index 2bb63ac4105..ddb76fd281d 100644
> --- a/gcc/cse.cc
> +++ b/gcc/cse.cc
> @@ -1312,11 +1312,10 @@ try_const_anchors (rtx src_const, machine_mode mode)
>    rtx lower_exp = NULL_RTX, upper_exp = NULL_RTX;
>    unsigned lower_old, upper_old;
>  
> -  /* CONST_INT is used for CC modes, but we should leave those alone.  */
> -  if (GET_MODE_CLASS (mode) == MODE_CC)
> +  /* CONST_INT is used for CC/BLK modes, leave any non-scalar-int mode alone. */
> +  if (!SCALAR_INT_MODE_P (mode))
>      return NULL_RTX;
>  
> -  gcc_assert (SCALAR_INT_MODE_P (mode));
>    if (!compute_const_anchors (src_const, &lower_base, &lower_offs,
>  			      &upper_base, &upper_offs))
>      return NULL_RTX;
> -------------
> 
> 
> BR,
> Jeff (Jiufu Guo)
> >
> > 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] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jiufu Guo]

Hi,

Richard Biener <rguenther@suse.de> writes:

> On Mon, 12 Jun 2023, Jiufu Guo wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> 
>> > On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >
>> >> 
>> >> Hi,
>> >> 
>> >> Richard Biener <rguenther@suse.de> writes:
>> >> 
>> >> > On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >> >
>> >> >> 
>> >> >> Hi,
>> >> >> 
>> >> >> Richard Biener <rguenther@suse.de> writes:
>> >> >> 
>> >> >> > On Fri, 9 Jun 2023, Richard Sandiford wrote:
>> >> >> >
>> >> >> >> guojiufu <guojiufu@linux.ibm.com> writes:
>> >> >> >> > Hi,
>> >> >> >> >
>> >> >> >> > On 2023-06-09 16:00, Richard Biener wrote:
>> >> >> >> >> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>> >> >> >> >> 
>> >> >> >> >>> Hi,
>> >> >> >> >>> 
>> >> ...
>> >> >> >> >>> 
>> >> >> >> >>> This patch is raised when drafting below one.
>> >> >> >> >>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>> >> >> >> >>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>> >> >> >> >>> try_const_anchors, and hits the assert/ice.
>> >> >> >> >>> 
>> >> >> >> >>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>> >> >> >> >>> Is this ok for trunk?
>> >> >> >> >> 
>> >> >> >> >> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>> >> >> >> >> I suggest to instead fix try_const_anchors to change
>> >> >> >> >> 
>> >> >> >> >>   /* CONST_INT is used for CC modes, but we should leave those alone.  
>> >> >> >> >> */
>> >> >> >> >>   if (GET_MODE_CLASS (mode) == MODE_CC)
>> >> >> >> >>     return NULL_RTX;
>> >> >> >> >> 
>> >> >> >> >>   gcc_assert (SCALAR_INT_MODE_P (mode));
>> >> >> >> >> 
>> >> >> >> >> to
>> >> >> >> >> 
>> >> >> >> >>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
>> >> >> >> >> alone.  */
>> >> >> >> >>   if (!SCALAR_INT_MODE_P (mode))
>> >> >> >> >>     return NULL_RTX;
>> >> >> >> >> 
>> >> >> >> >
>> >> >> >> > This is also able to fix this issue.  there is a "Punt on CC modes" 
>> >> >> >> > patch
>> >> >> >> > to return NULL_RTX in try_const_anchors.
>> >> >> >> >
>> >> >> >> >> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>> >> >> >> >> we should have fended this off earlier.  Can you share more complete
>> >> >> >> >> RTL of that stack_tie?
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > (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])".
>> >> >> >> 
>> >> >> >> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
>> >> >> >> would be though.  It's arguably more accurate too, since the effect
>> >> >> >> on the stack locations is unspecified rather than predictable.
>> >> >> >
>> >> >> > powerpc seems to be the only port with a stack_tie that's not
>> >> >> > using an UNSPEC RHS.
>> >> >> In rs6000.md, it is
>> >> >> 
>> >> >> ; 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"
>> >> >>   [(match_parallel 0 "tie_operand"
>> >> >> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
>> >> >>   ""
>> >> >>   ""
>> >> >>   [(set_attr "length" "0")])
>> >> >> 
>> >> >> This would be just an placeholder insn, and acts as the comments.
>> >> >> UNSPEC_ would works like other targets.  While, I'm wondering
>> >> >> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
>> >> >> MODEs between SET_DEST and SET_SRC?
>> >> >
>> >> > I don't think the issue is the mode but the issue is that
>> >> > the patter as-is says some memory is zeroed while that's not
>> >> > actually true (not specifying a size means we can't really do
>> >> > anything with this MEM, but still).  Using an UNSPEC avoids
>> >> > implying anything for the stored value.
>> >> >
>> >> > Of course I think a MEM SET_DEST without a specified size is bougs
>> >> > as well, but there's larger precedent for this...
>> >> 
>> >> Thanks for your kindly comments!
>> >> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
>> >> insn does not generate real thing (not a real store and no asm code),
>> >> may like barrier.
>> >> 
>> >> While I agree that, using UNSPEC may be more clear to avoid mis-reading.
>> >
>> > Btw, another way to avoid the issue in CSE is to make it not process
>> > (aka record anything for optimization) for SET from MEMs with
>> > !MEM_SIZE_KNOWN_P
>> 
>> Thanks! Yes, this would make sense.
>> Then, there are two ideas(patches) to handle this issue:
>> Which one would be preferable?  This one (from compiling time aspect)?
>> 
>> And maybe, the changes in rs6000 stack_tie through using unspec
>> can be a standalone enhancement besides cse patch.
>> 
>> Thanks for comments!
>> 
>> BR,
>> Jeff (Jiufu Guo)
>> 
>> -------------------- patch 1
>> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> index 2bb63ac4105..06ecdadecbc 100644
>> --- a/gcc/cse.cc
>> +++ b/gcc/cse.cc
>> @@ -4271,6 +4271,8 @@ find_sets_in_insn (rtx_insn *insn, vec<struct set> *psets)
>>  	 someplace else, so it isn't worth cse'ing.  */
>>        else if (GET_CODE (SET_SRC (x)) == CALL)
>>  	;
>> +      else if (MEM_P (SET_DEST (x)) && !MEM_SIZE_KNOWN_P (SET_DEST (x)))
>> +	;
>
> I don't know CSE enough to decide if this is correct, it depends on
> whether pset is only used to record new sets or if it is also used
> to invalidate prior recorded sets (in which case these sets have to
> be recorded still).  That is, the fix might better be in the caller
> who interprets 'pset'

The 'sets' is a variable of cse_insn: "auto_vec<struct set, 8> sets;"

And the cse work in cse_insn is based on this 'sets'.  So if a 'set' is
not added to this 'sets' through 'find_sets_in_insn', it would be
skipped from the cse process.

So, this patch updates 'find_sets_in_insn' to avoid adding the 'set'
with 'unknown mem size' to the 'sets'.

I'm not get the meaning of "the fix might better be in the caller who
interprets 'pset'". 

Thanks a lot for your comments!


BR,
Jeff (Jiufu Guo)

>
>>        else if (GET_CODE (SET_SRC (x)) == CONST_VECTOR
>>  	       && GET_MODE_CLASS (GET_MODE (SET_SRC (x))) != MODE_VECTOR_BOOL
>>  	       /* Prevent duplicates from being generated if the type is a V1
>> @@ -4314,6 +4316,8 @@ find_sets_in_insn (rtx_insn *insn, vec<struct set> *psets)
>>  		;
>>  	      else if (GET_CODE (SET_SRC (y)) == CALL)
>>  		;
>> +	      else if (MEM_P (SET_DEST (y)) && !MEM_SIZE_KNOWN_P (SET_DEST (y)))
>> +		;
>>  	      else
>>  		add_to_set (psets, y);
>>  	    }
>> -----------------------------
>> -------------------patch 2
>> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> index 2bb63ac4105..ddb76fd281d 100644
>> --- a/gcc/cse.cc
>> +++ b/gcc/cse.cc
>> @@ -1312,11 +1312,10 @@ try_const_anchors (rtx src_const, machine_mode mode)
>>    rtx lower_exp = NULL_RTX, upper_exp = NULL_RTX;
>>    unsigned lower_old, upper_old;
>>  
>> -  /* CONST_INT is used for CC modes, but we should leave those alone.  */
>> -  if (GET_MODE_CLASS (mode) == MODE_CC)
>> +  /* CONST_INT is used for CC/BLK modes, leave any non-scalar-int mode alone. */
>> +  if (!SCALAR_INT_MODE_P (mode))
>>      return NULL_RTX;
>>  
>> -  gcc_assert (SCALAR_INT_MODE_P (mode));
>>    if (!compute_const_anchors (src_const, &lower_base, &lower_offs,
>>  			      &upper_base, &upper_offs))
>>      return NULL_RTX;
>> -------------
>> 
>> 
>> BR,
>> Jeff (Jiufu Guo)
>> >
>> > Richard.
>> 

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jeff Law]

On 6/11/23 23:44, Jiufu Guo wrote:
> Richard Biener <rguenther@suse.de> writes:
> 
>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>>
>>>
>>> Hi,
>>>
>>> Richard Biener <rguenther@suse.de> writes:
>>>
>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Richard Biener <rguenther@suse.de> writes:
>>>>>
>>>>>> On Fri, 9 Jun 2023, Richard Sandiford wrote:
>>>>>>
>>>>>>> guojiufu <guojiufu@linux.ibm.com> writes:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2023-06-09 16:00, Richard Biener wrote:
>>>>>>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>> ...
>>>>>>>>>>
>>>>>>>>>> This patch is raised when drafting below one.
>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>>>>>>>>>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>>>>>>>>>> try_const_anchors, and hits the assert/ice.
>>>>>>>>>>
>>>>>>>>>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>>>>>>>>>> Is this ok for trunk?
>>>>>>>>>
>>>>>>>>> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>>>>>>>>> I suggest to instead fix try_const_anchors to change
>>>>>>>>>
>>>>>>>>>    /* CONST_INT is used for CC modes, but we should leave those alone.
>>>>>>>>> */
>>>>>>>>>    if (GET_MODE_CLASS (mode) == MODE_CC)
>>>>>>>>>      return NULL_RTX;
>>>>>>>>>
>>>>>>>>>    gcc_assert (SCALAR_INT_MODE_P (mode));
>>>>>>>>>
>>>>>>>>> to
>>>>>>>>>
>>>>>>>>>    /* CONST_INT is used for CC modes, leave any non-scalar-int mode
>>>>>>>>> alone.  */
>>>>>>>>>    if (!SCALAR_INT_MODE_P (mode))
>>>>>>>>>      return NULL_RTX;
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is also able to fix this issue.  there is a "Punt on CC modes"
>>>>>>>> patch
>>>>>>>> to return NULL_RTX in try_const_anchors.
>>>>>>>>
>>>>>>>>> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>>>>>>>>> we should have fended this off earlier.  Can you share more complete
>>>>>>>>> RTL of that stack_tie?
>>>>>>>>
>>>>>>>>
>>>>>>>> (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])".
>>>>>>>
>>>>>>> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
>>>>>>> would be though.  It's arguably more accurate too, since the effect
>>>>>>> on the stack locations is unspecified rather than predictable.
>>>>>>
>>>>>> powerpc seems to be the only port with a stack_tie that's not
>>>>>> using an UNSPEC RHS.
>>>>> In rs6000.md, it is
>>>>>
>>>>> ; 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"
>>>>>    [(match_parallel 0 "tie_operand"
>>>>> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
>>>>>    ""
>>>>>    ""
>>>>>    [(set_attr "length" "0")])
>>>>>
>>>>> This would be just an placeholder insn, and acts as the comments.
>>>>> UNSPEC_ would works like other targets.  While, I'm wondering
>>>>> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
>>>>> MODEs between SET_DEST and SET_SRC?
>>>>
>>>> I don't think the issue is the mode but the issue is that
>>>> the patter as-is says some memory is zeroed while that's not
>>>> actually true (not specifying a size means we can't really do
>>>> anything with this MEM, but still).  Using an UNSPEC avoids
>>>> implying anything for the stored value.
>>>>
>>>> Of course I think a MEM SET_DEST without a specified size is bougs
>>>> as well, but there's larger precedent for this...
>>>
>>> Thanks for your kindly comments!
>>> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
>>> insn does not generate real thing (not a real store and no asm code),
>>> may like barrier.
>>>
>>> While I agree that, using UNSPEC may be more clear to avoid mis-reading.
>>
>> Btw, another way to avoid the issue in CSE is to make it not process
>> (aka record anything for optimization) for SET from MEMs with
>> !MEM_SIZE_KNOWN_P
> 
> Thanks! Yes, this would make sense.
> Then, there are two ideas(patches) to handle this issue:
> Which one would be preferable?  This one (from compiling time aspect)?
> 
> And maybe, the changes in rs6000 stack_tie through using unspec
> can be a standalone enhancement besides cse patch.
I'd tend to lean more towards fixing the rs6000 backend.  It's basically 
lying to the rest of the compiler and when it presents passes with 
something like

(set (mem:BLK) (const_int 0))

It's largely inviting the generic bits to treat it like a memory store, 
when in fact it's something significantly different.

I don't think the CSE patch is wrong or a bad idea, more that it's just 
papering over a problem caused by an odd chunk of RTL created by the PPC 
backend.

jeff

Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[Jiufu Guo]

Hi,

Jeff Law <jeffreyalaw@gmail.com> writes:

> On 6/11/23 23:44, Jiufu Guo wrote:
>> Richard Biener <rguenther@suse.de> writes:
>>
>>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>>>
>>>>
>>>> Hi,
>>>>
>>>> Richard Biener <rguenther@suse.de> writes:
>>>>
>>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Richard Biener <rguenther@suse.de> writes:
>>>>>>
>>>>>>> On Fri, 9 Jun 2023, Richard Sandiford wrote:
>>>>>>>
>>>>>>>> guojiufu <guojiufu@linux.ibm.com> writes:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 2023-06-09 16:00, Richard Biener wrote:
>>>>>>>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>> ...
>>>>>>>>>>>
>>>>>>>>>>> This patch is raised when drafting below one.
>>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>>>>>>>>>>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>>>>>>>>>>> try_const_anchors, and hits the assert/ice.
>>>>>>>>>>>
>>>>>>>>>>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>>>>>>>>>>> Is this ok for trunk?
>>>>>>>>>>
>>>>>>>>>> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
>>>>>>>>>> I suggest to instead fix try_const_anchors to change
>>>>>>>>>>
>>>>>>>>>>    /* CONST_INT is used for CC modes, but we should leave those alone.
>>>>>>>>>> */
>>>>>>>>>>    if (GET_MODE_CLASS (mode) == MODE_CC)
>>>>>>>>>>      return NULL_RTX;
>>>>>>>>>>
>>>>>>>>>>    gcc_assert (SCALAR_INT_MODE_P (mode));
>>>>>>>>>>
>>>>>>>>>> to
>>>>>>>>>>
>>>>>>>>>>    /* CONST_INT is used for CC modes, leave any non-scalar-int mode
>>>>>>>>>> alone.  */
>>>>>>>>>>    if (!SCALAR_INT_MODE_P (mode))
>>>>>>>>>>      return NULL_RTX;
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is also able to fix this issue.  there is a "Punt on CC modes"
>>>>>>>>> patch
>>>>>>>>> to return NULL_RTX in try_const_anchors.
>>>>>>>>>
>>>>>>>>>> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
>>>>>>>>>> we should have fended this off earlier.  Can you share more complete
>>>>>>>>>> RTL of that stack_tie?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> (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])".
>>>>>>>>
>>>>>>>> I'm not convinced this is correct RTL.  (unspec:BLK [(const_int 0)] ...)
>>>>>>>> would be though.  It's arguably more accurate too, since the effect
>>>>>>>> on the stack locations is unspecified rather than predictable.
>>>>>>>
>>>>>>> powerpc seems to be the only port with a stack_tie that's not
>>>>>>> using an UNSPEC RHS.
>>>>>> In rs6000.md, it is
>>>>>>
>>>>>> ; 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"
>>>>>>    [(match_parallel 0 "tie_operand"
>>>>>> 		   [(set (mem:BLK (reg 1)) (const_int 0))])]
>>>>>>    ""
>>>>>>    ""
>>>>>>    [(set_attr "length" "0")])
>>>>>>
>>>>>> This would be just an placeholder insn, and acts as the comments.
>>>>>> UNSPEC_ would works like other targets.  While, I'm wondering
>>>>>> the concerns on "set (mem:BLK (reg 1)) (const_int 0)".
>>>>>> MODEs between SET_DEST and SET_SRC?
>>>>>
>>>>> I don't think the issue is the mode but the issue is that
>>>>> the patter as-is says some memory is zeroed while that's not
>>>>> actually true (not specifying a size means we can't really do
>>>>> anything with this MEM, but still).  Using an UNSPEC avoids
>>>>> implying anything for the stored value.
>>>>>
>>>>> Of course I think a MEM SET_DEST without a specified size is bougs
>>>>> as well, but there's larger precedent for this...
>>>>
>>>> Thanks for your kindly comments!
>>>> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this
>>>> insn does not generate real thing (not a real store and no asm code),
>>>> may like barrier.
>>>>
>>>> While I agree that, using UNSPEC may be more clear to avoid mis-reading.
>>>
>>> Btw, another way to avoid the issue in CSE is to make it not process
>>> (aka record anything for optimization) for SET from MEMs with
>>> !MEM_SIZE_KNOWN_P
>>
>> Thanks! Yes, this would make sense.
>> Then, there are two ideas(patches) to handle this issue:
>> Which one would be preferable?  This one (from compiling time aspect)?
>>
>> And maybe, the changes in rs6000 stack_tie through using unspec
>> can be a standalone enhancement besides cse patch.
> I'd tend to lean more towards fixing the rs6000 backend.  It's basically lying to the rest of the compiler and when it presents passes with something like
>
> (set (mem:BLK) (const_int 0))
>
> It's largely inviting the generic bits to treat it like a memory store, when in fact it's something significantly different.
>
> I don't think the CSE patch is wrong or a bad idea, more that it's
> just papering over a problem caused by an odd chunk of RTL created by
> the PPC backend.

Thanks a lot for your very kindly and helpful review!
Agree with you comments! A patch in rs6000 was prepared to handle this.

BR,
Jeff (Jiufu Guo)

>
> jeff