Re: [Tarantool-patches] [PATCH luajit 5/5] Fix IR_RENAME snapshot number. Follow-up fix for a32aeadc.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the patch!

Please consider my comments below.

On 15.02.23, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Victor Bombi, analyzed by XmiliaH. Thanks!
> 
> (cherry-picked from commit bf51d3535109c4745bfbbe19a5587a9eac00259a)
> 
> If the `snapalloc` flag is set, then the allocation hasn't
> occurred yet, meaning that rename is applied to the next
> snapshot. Otherwise, refs are already allocated and rename
> is applied to current applied to current snapshot.

Typo: s<applied to current applied to current snapshot>
       <applied to the current snapshot>

> 
> Maxim Kokryashkin:
> * added the description for the problem
> 
> Part of tarantool/tarantool#7745

I suggest to add the following:
| Part of tarantool/tarantool#8069

> ---
>  src/lj_asm.c | 9 ++++++++-

It will be nice to see the testcase, anyway.

>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index adfaf286..929a6da6 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c
> @@ -682,7 +682,14 @@ static void ra_rename(ASMState *as, Reg down, Reg up)
>    RA_DBGX((as, "rename    $f $r $r", regcost_ref(as->cost[up]), down, up));
>    emit_movrr(as, ir, down, up);  /* Backwards codegen needs inverse move. */
>    if (!ra_hasspill(IR(ref)->s)) {  /* Add the rename to the IR. */
> -    ra_addrename(as, down, ref, as->snapno);
> +    /*
> +    ** The rename is effective at the subsequent (already emitted) exit
> +    ** branch. This is for the current snapshot (as->snapno). Except if we
> +    ** haven't yet allocated any refs for the snapshot (as->snapalloc == 1),
> +    ** then it belongs to the next snapshot.
> +    ** See also the discussion at asm_snap_checkrename().
> +    */
> +    ra_addrename(as, down, ref, as->snapno + as->snapalloc);
>    }
>  }
>  
> -- 
> 2.39.0
> 

-- 
Best regards,
Sergey Kaplun