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

[Jiufu Guo <guojiufu@linux.ibm.com>]

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
>>