Hi,
Richard Biener <rguent...@suse.de> writes: > 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. 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 >>
