Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix of ToUInt32 (ecma_number_to_uint32) and ToInt32 (ecma_number_to_int32) conversion routines #226

Merged
merged 2 commits into from
Jun 23, 2015
+235 −26
Merged

Fix of ToUInt32 (ecma_number_to_uint32) and ToInt32 (ecma_number_to_int32) conversion routines #226

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1efaefa
Extract part of ecma_op_number_remainder (ECMA-262 v5, 11.5.3), corre…
ruben-ayrapetyan Jun 22, 2015
913519d
Fix of ToUInt32 (ecma_number_to_uint32) and ToInt32 (ecma_number_to_i…
ruben-ayrapetyan Jun 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 97 additions & 14 deletions jerry-core/ecma/base/ecma-helpers-conversion.cpp
View file
Original file line number Diff line number Diff line change
@@ -823,24 +823,80 @@ ecma_int32_to_number (int32_t value) /**< signed 32-bit integer value */
} /* ecma_int32_to_number */

/**
* ECMA-defined conversion of Number value to Uint32 value
* ECMA-defined conversion of Number value to UInt32 value
*
* See also:
* ECMA-262 v5, 9.6
*
* @return number - result of conversion.
* @return 32-bit unsigned integer - result of conversion.
*/
uint32_t
ecma_number_to_uint32 (ecma_number_t value) /**< unsigned 32-bit integer value */
ecma_number_to_uint32 (ecma_number_t num) /**< ecma-number */
{
if (ecma_number_is_nan (value)
|| ecma_number_is_zero (value)
|| ecma_number_is_infinity (value))
if (ecma_number_is_nan (num)
|| ecma_number_is_zero (num)
|| ecma_number_is_infinity (num))
{
return 0;
}

return (uint32_t) value;
bool sign = ecma_number_is_negative (num);
ecma_number_t abs_num;

if (sign)
{
abs_num = ecma_number_negate (num);
}
else
{
abs_num = num;
}

// 2 ^ 32
const uint64_t uint64_2_pow_32 = (1ull << 32);

const ecma_number_t num_2_pow_32 = (float) uint64_2_pow_32;

ecma_number_t num_in_uint32_range;

if (abs_num >= num_2_pow_32)
{
num_in_uint32_range = ecma_number_calc_remainder (abs_num,
num_2_pow_32);
}
else
{
num_in_uint32_range = abs_num;
}

// Check that the floating point value can be represented with uint32_t
JERRY_ASSERT (num_in_uint32_range < uint64_2_pow_32);
uint32_t uint32_num = (uint32_t) num_in_uint32_range;

uint32_t ret;

if (sign)
{
ret = -uint32_num;
}
else
{
ret = uint32_num;
}

#ifndef JERRY_NDEBUG
if (sign
&& uint32_num != 0)
{
JERRY_ASSERT (ret == uint64_2_pow_32 - uint32_num);
}
else
{
JERRY_ASSERT (ret == uint32_num);
}
#endif /* !JERRY_NDEBUG */

return ret;
} /* ecma_number_to_uint32 */

/**
@@ -849,19 +905,46 @@ ecma_number_to_uint32 (ecma_number_t value) /**< unsigned 32-bit integer value *
* See also:
* ECMA-262 v5, 9.5
*
* @return number - result of conversion.
* @return 32-bit signed integer - result of conversion.
*/
int32_t
ecma_number_to_int32 (ecma_number_t value) /**< unsigned 32-bit integer value */
ecma_number_to_int32 (ecma_number_t num) /**< ecma-number */
{
if (ecma_number_is_nan (value)
|| ecma_number_is_zero (value)
|| ecma_number_is_infinity (value))
uint32_t uint32_num = ecma_number_to_uint32 (num);

// 2 ^ 32
const int64_t int64_2_pow_32 = (1ll << 32);

// 2 ^ 31
const uint32_t uint32_2_pow_31 = (1ull << 31);

int32_t ret;

if (uint32_num >= uint32_2_pow_31)
{
return 0;
ret = (int32_t) (uint32_num - int64_2_pow_32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not fully sure, but isn't the is the same as the original uint32_num? You substract 32 0s, and only keep the lower 32 bit. Should be the same as ret = (int32_t) uint32_num; If this is true, we could remove the whole check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, it is true for most cases, but according to C++ standard (4.7. Integral conversions):
"If the destination type is signed, the value is unchanged if it can be represented in the destination type (and bit-field width); otherwise, the value is implementation-defined."

From other view point, for cases, where it is true, optimizer should remove unnecessary operations. I'll check this on armv7-hf and x86_32 targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If noinline attribute is put on both functions, g++ 4.8.2 produces the following code for ecma_number_to_int32 (-Os optimization level):

  • x86:
0804fdab <_Z20ecma_number_to_int32d>:
 804fdab:       55                      push   %ebp
 804fdac:       89 e5                   mov    %esp,%ebp
 804fdae:       5d                      pop    %ebp
 804fdaf:       e9 47 ff ff ff          jmp    804fcfb <_Z21ecma_number_to_uint32d>
  • armv7-hf:
0000d0c0 <_Z20ecma_number_to_int32d>:
    d0c0:       e7c1            b.n     d046 <_Z21ecma_number_to_uint32d>

So, all conversion operations in ecma_number_to_int32, are removed for the targets, and the function, actually, just returns the value, received from ecma_number_to_uint32.

}
else
{
ret = (int32_t) uint32_num;
}

#ifndef JERRY_NDEBUG
int64_t int64_num = uint32_num;

JERRY_ASSERT (int64_num >= 0);

if (int64_num >= uint32_2_pow_31)
{
JERRY_ASSERT (ret == int64_num - int64_2_pow_32);
}
else
{
JERRY_ASSERT (ret == int64_num);
}
#endif /* !JERRY_NDEBUG */

return (int32_t) (uint32_t) value;
return ret;
} /* ecma_number_to_int32 */

#if CONFIG_ECMA_NUMBER_TYPE == CONFIG_ECMA_NUMBER_FLOAT64
27 changes: 27 additions & 0 deletions jerry-core/ecma/base/ecma-helpers-number.cpp
View file
Original file line number Diff line number Diff line change
@@ -707,6 +707,33 @@ ecma_number_trunc (ecma_number_t num) /**< ecma-number */
}
} /* ecma_number_trunc */

/**
* Calculate remainder of division of two numbers,
* as specified in ECMA-262 v5, 11.5.3, item 6.
*
* Note:
* operands shouldn't contain NaN, Infinity, or zero.
*
* @return number - calculated remainder.
*/
ecma_number_t
ecma_number_calc_remainder (ecma_number_t left_num, /**< left operand */
ecma_number_t right_num) /**< right operand */
{
ecma_number_t n = left_num, d = right_num;

JERRY_ASSERT (!ecma_number_is_nan (n)
&& !ecma_number_is_zero (n)
&& !ecma_number_is_infinity (n));
JERRY_ASSERT (!ecma_number_is_nan (d)
&& !ecma_number_is_zero (d)
&& !ecma_number_is_infinity (d));

ecma_number_t q = ecma_number_trunc (ecma_number_divide (n, d));

return ecma_number_substract (n, ecma_number_multiply (d, q));
} /* ecma_number_calc_remainder */

/**
* ECMA-number addition.
*
1 change: 1 addition & 0 deletions jerry-core/ecma/base/ecma-helpers.h
View file
Original file line number Diff line number Diff line change
@@ -185,6 +185,7 @@ extern ecma_number_t ecma_number_get_prev (ecma_number_t num);
extern ecma_number_t ecma_number_get_next (ecma_number_t num);
extern ecma_number_t ecma_number_negate (ecma_number_t num);
extern ecma_number_t ecma_number_trunc (ecma_number_t num);
extern ecma_number_t ecma_number_calc_remainder (ecma_number_t left_num, ecma_number_t right_num);
extern ecma_number_t ecma_number_add (ecma_number_t left_num, ecma_number_t right_num);
extern ecma_number_t ecma_number_substract (ecma_number_t left_num, ecma_number_t right_num);
extern ecma_number_t ecma_number_multiply (ecma_number_t left_num, ecma_number_t right_num);
13 changes: 1 addition & 12 deletions jerry-core/ecma/operations/ecma-number-arithmetic.cpp
View file
Original file line number Diff line number Diff line change
@@ -38,8 +38,6 @@ ecma_number_t
ecma_op_number_remainder (ecma_number_t left_num, /**< left operand */
ecma_number_t right_num) /**< right operand */
{
TODO (Check precision);

ecma_number_t n = left_num, d = right_num;

if (ecma_number_is_nan (n)
@@ -56,16 +54,7 @@ ecma_op_number_remainder (ecma_number_t left_num, /**< left operand */
return n;
}

JERRY_ASSERT (!ecma_number_is_nan (n)
&& !ecma_number_is_zero (n)
&& !ecma_number_is_infinity (n));
JERRY_ASSERT (!ecma_number_is_nan (d)
&& !ecma_number_is_zero (d)
&& !ecma_number_is_infinity (d));

ecma_number_t q = ecma_number_trunc (ecma_number_divide (n, d));

return ecma_number_substract (n, ecma_number_multiply (d, q));
return ecma_number_calc_remainder (n, d);
} /* ecma_op_number_remainder */

/**
109 changes: 109 additions & 0 deletions tests/unit/test-number-to-integer.cpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/* Copyright 2015 Samsung Electronics Co., Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "ecma-globals.h"
#include "ecma-helpers.h"

#include "test-common.h"

typedef struct
{
ecma_number_t num;
uint32_t uint32_num;
} uint32_test_case_t;

typedef struct
{
ecma_number_t num;
int32_t int32_num;
} int32_test_case_t;

/**
* Unit test's main function.
*/
int
main (int __attr_unused___ argc,
char __attr_unused___ **argv)
{
TEST_INIT ();

const uint32_test_case_t test_cases_uint32[] =
{
#define TEST_CASE(num, uint32) { num, uint32 }
TEST_CASE (1.0, 1),
TEST_CASE (0.0, 0),
TEST_CASE (ecma_number_negate (0.0), 0),
TEST_CASE (NAN, 0),
TEST_CASE (-NAN, 0),
TEST_CASE (INFINITY, 0),
TEST_CASE (-INFINITY, 0),
TEST_CASE (0.1, 0),
TEST_CASE (-0.1, 0),
TEST_CASE (1.1, 1),
TEST_CASE (-1.1, 4294967295),
TEST_CASE (4294967295, 4294967295),
TEST_CASE (-4294967295, 1),
TEST_CASE (4294967296, 0),
TEST_CASE (-4294967296, 0),
TEST_CASE (4294967297, 1),
TEST_CASE (-4294967297, 4294967295)
#undef TEST_CASE
};

for (uint32_t i = 0;
i < sizeof (test_cases_uint32) / sizeof (test_cases_uint32[0]);
i++)
{
JERRY_ASSERT (ecma_number_to_uint32 (test_cases_uint32[i].num) == test_cases_uint32[i].uint32_num);
}

int32_test_case_t test_cases_int32[] =
{
#define TEST_CASE(num, int32) { num, int32 }
TEST_CASE (1.0, 1),
TEST_CASE (0.0, 0),
TEST_CASE (ecma_number_negate (0.0), 0),
TEST_CASE (NAN, 0),
TEST_CASE (-NAN, 0),
TEST_CASE (INFINITY, 0),
TEST_CASE (-INFINITY, 0),
TEST_CASE (0.1, 0),
TEST_CASE (-0.1, 0),
TEST_CASE (1.1, 1),
TEST_CASE (-1.1, -1),
TEST_CASE (4294967295, -1),
TEST_CASE (-4294967295, 1),
TEST_CASE (4294967296, 0),
TEST_CASE (-4294967296, 0),
TEST_CASE (4294967297, 1),
TEST_CASE (-4294967297, -1),
TEST_CASE (2147483648, -2147483648),
TEST_CASE (-2147483648, -2147483648),
TEST_CASE (2147483647, 2147483647),
TEST_CASE (-2147483647, -2147483647),
TEST_CASE (-2147483649, 2147483647),
TEST_CASE (2147483649, -2147483647)
#undef TEST_CASE
};

for (uint32_t i = 0;
i < sizeof (test_cases_int32) / sizeof (test_cases_int32[0]);
i++)
{
JERRY_ASSERT (ecma_number_to_int32 (test_cases_int32[i].num) == test_cases_int32[i].int32_num);
}

return 0;
} /* main */