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

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

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.

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

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