Closed
Bug 705351
Opened 14 years ago
Closed 14 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•14 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•14 years ago
|
||
Adds an out-of-line path to MUnbox.
Attachment #577680 -
Attachment is obsolete: true
Attachment #578650 -
Flags: review?(dvander)
Assignee | ||
Comment 4•14 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•14 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•14 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•14 years ago
|
||
Pushed with OutOfLineUnboxDouble moved to CodeGenerator.cpp:
http://hg.mozilla.org/projects/ionmonkey/rev/351e94bf59cb
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•