Re: [Tarantool-patches] [PATCH luajit 5/5] Revert to trival pow() optimizations to prevent inaccuracies.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
LGTM now, except for a single comment below.
Also, see my answers too.
On Mon, Aug 21, 2023 at 11:15:22AM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
> See my answers below.
> 
> On 20.08.23, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> > On Tue, Aug 15, 2023 at 12:36:31PM +0300, Sergey Kaplun wrote:
> > > From: Mike Pall <mike>
> > > 
> > > (cherry-picked from commit 96d6d5032098ea9f0002165394a8774dcaa0c0ce)
> > > 
> > > This patch fixes different misbehaviour between JIT-compiled code and
> > Typo: s/misbehaviour/misbehaviours/
> 
> Fixed.
> 
> > > the interpreter for power operator with the following ways:
> > Typo: s/with/in/
> 
> Fixed.
> 
> > > * Drop folding optimizations for base ^ n => base * base ..., as far as
> > >   pow(base, n) isn't interchangeable with just multiplicity of numbers
> > >   and depends on the <math.h> implementation.
> > > * Since the internal power function is inaccurate for very big or small
> > >   powers, it is dropped, and `pow()` from the standard library is used
> > >   instead. To save consistency between JIT behaviour and the VM
> > Typo: s/VM/VM,/
> 
> Fixed.
> 
> > >   narrowing optimization is dropped, and only trivial folding
> > >   optimizations are used. Also, `math_extern2` version with two
> > >   parameters is dropped, since it's no more used.
> > Typo: s/more/longer/
> 
> Fixed.
> 
> > > 
> > > Also, this fixes failures of the [220/502] lib/string/format/num.lua
> > > test [1] from LuaJIT-test suite.
> > Typo: s/from/from the/
> 
> Fixed.
> 
> > > 
> > > [1]: https://www.exploringbinary.com/incorrect-floating-point-to-decimal-conversions/
> > > 
> > > Sergey Kaplun:
> > > * added the description and the test for the problem
> > > 
> > > Part of tarantool/tarantool#8825
> > > ---
> 
> <snipped>
> 
> > >  
> > > +-- -948388 ^ 3 = -0x1.7ad0e8ad7439dp+59.
> > Same as in the previous patch, we need some additinal commentary for
> > those magic numbers.
> 
> See my answer in the previous reply.
I think the same commentary, as one I suggested in reply, will do.
> 
> > > +res = {}
> > > +-- XXX: use local variable to prevent folding via parser.
> > > +-- XXX: use stack slot out of trace to prevent constant folding.
> > > +local corner_case_3 = -948388
> > Naming is misleading, it seems like it is the test case number,
> > which it is not. Please also fix this in the previous patch for `corner_case_5`.
> 
> Renamed as the following:
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua
> index 003fe957..418a1557 100644
> --- a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua
> +++ b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua
> @@ -52,10 +52,10 @@ test:samevalues(res, ('consistent results for folding (-inf) ^ 0.5'))
>  res = {}
>  -- XXX: use local variable to prevent folding via parser.
>  -- XXX: use stack slot out of trace to prevent constant folding.
> -local corner_case_05 = 2921
> +local corner_case_pow_05 = 2921
>  jit.on()
>  for i = 1, 4 do
> -  res[i] = corner_case_05 ^ 0.5
> +  res[i] = corner_case_pow_05 ^ 0.5
>  end
>  
>  -- XXX: Prevent hotcount side effects.
> ===================================================================
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua
> index 23dd44e8..57685a72 100644
> --- a/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua
> +++ b/test/tarantool-tests/lj-684-pow-inconsistencies.test.lua
> @@ -69,10 +69,10 @@ test:samevalues(res, ('consistent results for folding 2921 ^ 0.5'))
>  res = {}
>  -- XXX: use local variable to prevent folding via parser.
>  -- XXX: use stack slot out of trace to prevent constant folding.
> -local corner_case_3 = -948388
> +local corner_case_pow_3 = -948388
>  jit.on()
>  for i = 1, 4 do
> -  res[i] = corner_case_3 ^ 3
> +  res[i] = corner_case_pow_3 ^ 3
>  end
>  
>  -- XXX: Prevent hotcount side effects.
> ===================================================================
> 
> Branch is force-pushed.
> 
> > > +jit.on()
> > > +for i = 1, 4 do
> > > +  res[i] = corner_case_3 ^ 3
> > > +end
> > > +
> > > +-- XXX: Prevent hotcount side effects.
> > > +jit.off()
> > > +jit.flush()
> > 
> > If you'll succeed making that dedicated function for those test cases in the
> > previous patch fix ups, this one should be rewritten too.
> 
> See my answer in the previous reply.
As I've already said in the previous reply, I've checked the proposed change
by myself and it doesn't seem to work great, although it would increase readability
by a great margin. So yep, ignore it.
> 
> > > +
> > > +test:samevalues(res, ('consistent results for int pow (-948388) ^ 3'))
> > > +
> > >  -- Narrowing for non-constant base of power operation.
> > >  local function pow(base, power)
> > >    return base ^ power
> > > -- 
> > > 2.41.0
> > > 
> 
> -- 
> Best regards,
> Sergey Kaplun