On Wed, 14 Jun 2023, Jiufu Guo wrote: > > Hi, > > Segher Boessenkool <seg...@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 <rguent...@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)
