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

Reply via email to