Re: [Tarantool-patches] [PATCH luajit 06/19] PPC: Add soft-float support to JIT compiler backend.

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

Hi, Maxim!
Thanks for the review!
See my answers below.

On 15.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few typos and a single question below.
> On Wed, Aug 09, 2023 at 06:35:55PM +0300, Sergey Kaplun via Tarantool-patches wrote:
> > From: Mike Pall <mike>
> > 
> > Contributed by Djordje Kovacevic and Stefan Pejic from RT-RK.com.
> > Sponsored by Cisco Systems, Inc.
> > 
> > (cherry-picked from commit 71b7bc88341945f13f3951e2bb5fd247b639ff7a)
> > 
> > The software floating point library is used on machines which do not
> > have hardware support for floating point [1]. This patch enables
> > support for such machines in the JIT compiler for powerpc.
> Typo: s/powerpc/PowerPC/

Fixed, thanks!

> > This includes:
> > * All fp-depending paths are instrumented with `LJ_SOFTFP` macro.
> Typo: s/fp-depending/fp-dependent/

Fixed.

> > * `asm_sfpmin_max()` is introduced for min/max operations on soft-float
> >   point.
> > * `asm_sfpcomp()` is introduced for soft-float point comparisons.
> > 
> > [1]: https://gcc.gnu.org/onlinedocs/gccint/Soft-float-library-routines.html
> > 
> > Sergey Kaplun:
> > * added the description for the feature
> > 
> > Part of tarantool/tarantool#8825
> > ---

<snipped>

> > +#if LJ_SOFTFP
> > +  case IR_SLOAD: case IR_ALOAD: case IR_HLOAD: case IR_ULOAD: case IR_VLOAD:
> > +  case IR_STRTO:
> Why are those fp-dependent? Should we write an explanation?

I supposed, that is used lo for possible half of a fp value.
Also, there is no need to use it on hard-float machines.
I suppose, that the comment as is is OK.
Same for the stores.

> > +    if (!uselo)
> > +      ra_allocref(as, ir->op1, RSET_GPR);  /* Mark lo op as used. */
> > +    break;
> > +#endif
> >    case IR_CALLN:
> > +  case IR_CALLS:
> >    case IR_CALLXS:
> >      if (!uselo)
> >        ra_allocref(as, ir->op1, RID2RSET(RID_RETLO));  /* Mark lo op as used. */
> >      break;
> > +#if LJ_SOFTFP
> > +  case IR_ASTORE: case IR_HSTORE: case IR_USTORE: case IR_TOSTR:
> > +#endif
> >    case IR_CNEWI:
> >      /* Nothing to do here. Handled by lo op itself. */
> >      break;
> > @@ -1800,8 +2019,19 @@ static void asm_stack_restore(ASMState *as, SnapShot *snap)
> >      if ((sn & SNAP_NORESTORE))
> >        continue;
> >      if (irt_isnum(ir->t)) {

<snipped>

> > 

-- 
Best regards,
Sergey Kaplun