Fix of ToUInt32 (ecma_number_to_uint32) and ToInt32 (ecma_number_to_int32) conversion routines #226
Conversation
ruben-ayrapetyan
added
bug
|
Good to me. |
| const ecma_number_t num_2_pow_32 = (float) uint64_2_pow_32; | ||
|
|
||
| ecma_number_t num_in_uint32_range = ecma_number_calc_remainder (abs_num, | ||
| num_2_pow_32); |
There was a problem hiding this comment.
Personally I think this is a costly operation, and would add a fast path which simply checks that abs_num < num_2_pow_32.
| // Check that the value can be represented with uint32_t | ||
| JERRY_ASSERT (ret < (1ull << 32)); | ||
|
|
||
| return (uint32_t) ret; |
There was a problem hiding this comment.
uint64 computation on 32 bit machines is also costly, and I think it is not necessary here. We could achive the same result with:
uint32_t uint32_num = (uint32_t) num_in_uint32_range;
if (sign)
{
uint32_num = -uint32_num;
}
return uint32_num;
ruben-ayrapetyan
force-pushed
the
Ruben-fix-issue-160
branch
from
d852a3c to
48d7d04
Compare
|
Vera checks failed. |
ruben-ayrapetyan
force-pushed
the
Ruben-fix-issue-160
branch
from
48d7d04 to
3293c6e
Compare
|
Pull request is updated. |
ruben-ayrapetyan
force-pushed
the
Ruben-fix-issue-160
branch
from
3293c6e to
6b71cf3
Compare
|
Good to me. |
| { | ||
| return 0; | ||
| ret = (int32_t) (uint32_num - int64_2_pow_32); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Looks much better, +1 from me. |
|
|
ruben-ayrapetyan
force-pushed
the
Ruben-fix-issue-160
branch
from
6b71cf3 to
38fd674
Compare
…sponding to item 6 (actual remainder calculation, without handling of NaN. Infinity and zero values), as ecma_number_calc_remainder. JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan [email protected]
…nt32) conversion routines. Related issue: #160 JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan [email protected]
ruben-ayrapetyan
force-pushed
the
Ruben-fix-issue-160
branch
from
38fd674 to
913519d
Compare
Related issue: #160