Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10026017:
Fix incorrect Math.pow() calculations with MinGW-w64 (Closed)
|
Created:
8 years, 8 months ago by Net147 Modified:
8 years, 8 months ago CC:
v8-dev Visibility:
Public. |
DescriptionFix incorrect Math.pow() calculations with MinGW-w64
Contributed by net147@gmail.com
BUGS=
TEST=mjsunit/math-pow,mjsunit/math-sqrt
Patch Set 1 : #
Total comments: 1
Messages
Total messages: 10 (0 generated)
On 2012/04/09 07:36:46, Net147 wrote: Please explain why this is necessary. Is the implementation of pow in MinGW-w64 somehow faulty? It seems to me that you are just handling a few special cases differently. Are you sure you caught all special cases where pow would return the wrong result?
On 2012/04/17 13:44:56, Yang wrote: > On 2012/04/09 07:36:46, Net147 wrote: > > Please explain why this is necessary. Is the implementation of pow in MinGW-w64 > somehow faulty? It seems to me that you are just handling a few special cases > differently. Are you sure you caught all special cases where pow would return > the wrong result? MinGW-w64 has a custom implementation of pow that behaves slightly differently to MinGW, MSVC and GCC on Linux in these special cases. I've handled all the cases that are tested in the V8 unit tests. With this change and previous changes, all unit tests pass with MinGW/MinGW-w64 4.5.2 including https://code.google.com/p/v8/issues/detail?id=1063.
On 2012/04/17 13:54:26, Net147 wrote: > On 2012/04/17 13:44:56, Yang wrote: > > On 2012/04/09 07:36:46, Net147 wrote: > > > > Please explain why this is necessary. Is the implementation of pow in > MinGW-w64 > > somehow faulty? It seems to me that you are just handling a few special cases > > differently. Are you sure you caught all special cases where pow would return > > the wrong result? > > MinGW-w64 has a custom implementation of pow that behaves slightly differently > to MinGW, MSVC and GCC on Linux in these special cases. I've handled all the > cases that are tested in the V8 unit tests. With this change and previous > changes, all unit tests pass with MinGW/MinGW-w64 4.5.2 including > https://code.google.com/p/v8/issues/detail?id=1063. I see. I think it's more important to make pow compliant with the ECMA spec, so it might be better to test against test262 as well. Also, there is one more place where pow is used (Runtime_Math_pow in runtime.cc), which should be fixed as well?
On 2012/04/17 14:02:51, Yang wrote: > On 2012/04/17 13:54:26, Net147 wrote: > > On 2012/04/17 13:44:56, Yang wrote: > > > On 2012/04/09 07:36:46, Net147 wrote: > > > > > > Please explain why this is necessary. Is the implementation of pow in > > MinGW-w64 > > > somehow faulty? It seems to me that you are just handling a few special > cases > > > differently. Are you sure you caught all special cases where pow would > return > > > the wrong result? > > > > MinGW-w64 has a custom implementation of pow that behaves slightly differently > > to MinGW, MSVC and GCC on Linux in these special cases. I've handled all the > > cases that are tested in the V8 unit tests. With this change and previous > > changes, all unit tests pass with MinGW/MinGW-w64 4.5.2 including > > https://code.google.com/p/v8/issues/detail?id=1063. > > I see. I think it's more important to make pow compliant with the ECMA spec, so > it might be better to test against test262 as well. Also, there is one more > place where pow is used (Runtime_Math_pow in runtime.cc), which should be fixed > as well? Runtime_Math_pow just calls power_double_double in assembler.cc which is handled already. How do I run test262?
On 2012/04/17 14:17:48, Net147 wrote: > On 2012/04/17 14:02:51, Yang wrote: > > On 2012/04/17 13:54:26, Net147 wrote: > > > On 2012/04/17 13:44:56, Yang wrote: > > > > On 2012/04/09 07:36:46, Net147 wrote: > > > > > > > > Please explain why this is necessary. Is the implementation of pow in > > > MinGW-w64 > > > > somehow faulty? It seems to me that you are just handling a few special > > cases > > > > differently. Are you sure you caught all special cases where pow would > > return > > > > the wrong result? > > > > > > MinGW-w64 has a custom implementation of pow that behaves slightly > differently > > > to MinGW, MSVC and GCC on Linux in these special cases. I've handled all the > > > cases that are tested in the V8 unit tests. With this change and previous > > > changes, all unit tests pass with MinGW/MinGW-w64 4.5.2 including > > > https://code.google.com/p/v8/issues/detail?id=1063. > > > > I see. I think it's more important to make pow compliant with the ECMA spec, > so > > it might be better to test against test262 as well. Also, there is one more > > place where pow is used (Runtime_Math_pow in runtime.cc), which should be > fixed > > as well? > > Runtime_Math_pow just calls power_double_double in assembler.cc which is handled > already. How do I run test262? My bad. You can find instructions for test262 in test/test262/README.
On 2012/04/17 14:26:23, Yang wrote: > On 2012/04/17 14:17:48, Net147 wrote: > > On 2012/04/17 14:02:51, Yang wrote: > > > On 2012/04/17 13:54:26, Net147 wrote: > > > > On 2012/04/17 13:44:56, Yang wrote: > > > > > On 2012/04/09 07:36:46, Net147 wrote: > > > > > > > > > > Please explain why this is necessary. Is the implementation of pow in > > > > MinGW-w64 > > > > > somehow faulty? It seems to me that you are just handling a few special > > > cases > > > > > differently. Are you sure you caught all special cases where pow would > > > return > > > > > the wrong result? > > > > > > > > MinGW-w64 has a custom implementation of pow that behaves slightly > > differently > > > > to MinGW, MSVC and GCC on Linux in these special cases. I've handled all > the > > > > cases that are tested in the V8 unit tests. With this change and previous > > > > changes, all unit tests pass with MinGW/MinGW-w64 4.5.2 including > > > > https://code.google.com/p/v8/issues/detail?id=1063. > > > > > > I see. I think it's more important to make pow compliant with the ECMA spec, > > so > > > it might be better to test against test262 as well. Also, there is one more > > > place where pow is used (Runtime_Math_pow in runtime.cc), which should be > > fixed > > > as well? > > > > Runtime_Math_pow just calls power_double_double in assembler.cc which is > handled > > already. How do I run test262? > > My bad. You can find instructions for test262 in test/test262/README. All 34752 unit tests from test262 pass. Quite a lot of tests. I ran the tests using 8 shards/threads to speed it up testing.
On 2012/04/17 14:54:46, Net147 wrote: > On 2012/04/17 14:26:23, Yang wrote: > > On 2012/04/17 14:17:48, Net147 wrote: > > > On 2012/04/17 14:02:51, Yang wrote: > > > > On 2012/04/17 13:54:26, Net147 wrote: > > > > > On 2012/04/17 13:44:56, Yang wrote: > > > > > > On 2012/04/09 07:36:46, Net147 wrote: > > > > > > > > > > > > Please explain why this is necessary. Is the implementation of pow in > > > > > MinGW-w64 > > > > > > somehow faulty? It seems to me that you are just handling a few > special > > > > cases > > > > > > differently. Are you sure you caught all special cases where pow would > > > > return > > > > > > the wrong result? > > > > > > > > > > MinGW-w64 has a custom implementation of pow that behaves slightly > > > differently > > > > > to MinGW, MSVC and GCC on Linux in these special cases. I've handled all > > the > > > > > cases that are tested in the V8 unit tests. With this change and > previous > > > > > changes, all unit tests pass with MinGW/MinGW-w64 4.5.2 including > > > > > https://code.google.com/p/v8/issues/detail?id=1063. > > > > > > > > I see. I think it's more important to make pow compliant with the ECMA > spec, > > > so > > > > it might be better to test against test262 as well. Also, there is one > more > > > > place where pow is used (Runtime_Math_pow in runtime.cc), which should be > > > fixed > > > > as well? > > > > > > Runtime_Math_pow just calls power_double_double in assembler.cc which is > > handled > > > already. How do I run test262? > > > > My bad. You can find instructions for test262 in test/test262/README. > > All 34752 unit tests from test262 pass. Quite a lot of tests. I ran the tests > using 8 shards/threads to speed it up testing. Landed with small changes as r11362.
https://chromiumcodereview.appspot.com/10026017/diff/5002/src/assembler.cc File src/assembler.cc (right): https://chromiumcodereview.appspot.com/10026017/diff/5002/src/assembler.cc#ne... src/assembler.cc:1168: return ldexp(1.0, y); nit: y_int
On 2012/04/17 16:40:13, alexeif wrote: > https://chromiumcodereview.appspot.com/10026017/diff/5002/src/assembler.cc > File src/assembler.cc (right): > > https://chromiumcodereview.appspot.com/10026017/diff/5002/src/assembler.cc#ne... > src/assembler.cc:1168: return ldexp(1.0, y); > nit: y_int Fix in https://chromiumcodereview.appspot.com/10116001/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
