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