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

[Richard Biener <rguenther@suse.de>]

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)