Closed
Bug 705351
Opened 13 years ago
Closed 12 years ago
IM+TI: check for int32 when loading doubles from property/array slots
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file, 4 obsolete files)
26.78 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
When JM loads a double from a property or element slot, it checks whether it's a double or an int. If it's an int, it's converted to a double. Getelem/getgname in Ion currently don't do this check and this causes correctness issues in some sunspider tests. Here's an example: -- var a = 1.1; function f(arr) { return a + 0.2; } function test() { for (var i=0; i<100; i++) print(f()); a = 20; print(f()); } test(); -- With --ion -n, this prints 20 instead of 20.2
Assignee | ||
Comment 1•13 years ago
|
||
- Adds loadInt32OrDouble, it's used by the LLoad*T instructions. It checks whether the value is an int32 and then either loads as double or converts it to double. - In pushTypeBarrier, use MToDouble instead of MUnbox for doubles. This ensures we don't bailout when the value is an integer. This handles the LLoad*V instructions. Note that LValueToDouble is not very efficient right now, but it could take a TypeSet* in the future and only emit the int32/double paths. - Fixes a small bug in pushTypeBarrier (related to null/undefined) and added a test for it.
Attachment #577680 -
Flags: review?(dvander)
Comment on attachment 577680 [details] [diff] [review] Fix Review of attachment 577680 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.cpp @@ +2515,5 @@ > + break; > + > + case JSVAL_TYPE_DOUBLE: > + // Use MToDouble to handle Int32Value -> double too. > + barrier = MToDouble::New(ins); This will generate LValueToDouble which could incorrectly coerce something like "null" to 0.0. Instead, I would make MUnbox have an out-of-line path for int32->double conversion. The MToDouble earlier is okay since it's infallible, but it would be better to use MUnbox there as well to generate less code.
Attachment #577680 -
Flags: review?(dvander)
Assignee | ||
Comment 3•13 years ago
|
||
Adds an out-of-line path to MUnbox.
Attachment #577680 -
Attachment is obsolete: true
Attachment #578650 -
Flags: review?(dvander)
Assignee | ||
Comment 4•13 years ago
|
||
Moves the bailout to the OOL path. One problem was that masm.framePushed() was always 0 for out-of-line code (since it's generated after the function epilogue) and bailout() expects it to be valid. I think two OOL paths can have different framePushed values so I decided to store the framePushed value in the OOL class and restore it when generating code for the OOL path.
Attachment #578650 -
Attachment is obsolete: true
Attachment #578650 -
Flags: review?(dvander)
Attachment #578820 -
Flags: review?(dvander)
Comment on attachment 578820 [details] [diff] [review] Patch v3 Review of attachment 578820 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing the out-of-line variant. Make sure to have a follow-up patch for ARM - if OutOfLineUnboxDouble could even be shared across all platforms (at the very least, its header), even better. ::: js/src/ion/x86/CodeGenerator-x86.cpp @@ +250,5 @@ > +{ > + LUnboxDouble *ins = ool->unboxDouble(); > + const ValueOperand value = ToValue(ins, LUnboxDouble::Input); > + > + masm.bind(ool->entry()); Sort of unrelated, is it possible to hoist entry-binding to the OOL visitor?
Attachment #578820 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 6•13 years ago
|
||
If we add LUnboxDouble for x64, we can generate code for it in CodeGenerator.cpp and get rid of the 3 platform-dependent implementations. This also makes it easier to add the OOL path to CodeGenerator.cpp Asking re-review mostly for the LUnboxDouble on x64.
Attachment #578820 -
Attachment is obsolete: true
Attachment #580032 -
Flags: review?(dvander)
Assignee | ||
Comment 7•13 years ago
|
||
Now also moves entry-binding to the OOL visitor.
Attachment #580032 -
Attachment is obsolete: true
Attachment #580032 -
Flags: review?(dvander)
Attachment #580047 -
Flags: review?(dvander)
Comment on attachment 580047 [details] [diff] [review] Patch Review of attachment 580047 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.h @@ +88,5 @@ > + virtual bool visitOutOfLineUnboxDouble(OutOfLineUnboxDouble *ool); > +}; > + > +// An out-of-line path to convert a boxed int32 to a double. > +class OutOfLineUnboxDouble : public OutOfLineCodeBase<CodeGenerator> Nit: You could move this into CodeGenerator.cpp to avoid .h clutter.
Attachment #580047 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Pushed with OutOfLineUnboxDouble moved to CodeGenerator.cpp: http://hg.mozilla.org/projects/ionmonkey/rev/351e94bf59cb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•