Closed Bug 854400 Opened 12 years ago Closed 12 years ago

UnsafeSetElement does not handle double conversion

Categories

(Core :: JavaScript Engine, defect)

22 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: nmatsakis, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

I believe that the inlining for UnsafeSetElement() is not properly handling double conversion. On IRC, jandem writes: [09:59:31] <jandem> nmatsakis: yes you have to ensure int32 values written to the array are converted to double if needed [10:00:32] <jandem> nmatsakis: something like TypeOracle::elementWriteNeedsDoubleConversion, if that's |true| then insert an MToDouble instruction, like IonBuilder::jsop_setelem_dense One tricky part is that creating a simple testcase for intrinsics is difficult since they cannot be directly invoked.
Blocks: PJS
It turns out intrinsics can be accessed from within the shell, so it shouldn't be too hard to devise a test.
Attached patch patch (obsolete) — Splinter Review
Not sure how to write a testcase for this, since we can't test type tags.
Carrying over r+ from bhackett, which was given on attachment 746701 [details] [diff] [review] (accidentally added to bug 833898)
Attachment #729359 - Attachment is obsolete: true
Attachment #746961 - Flags: review+
Attachment #746963 - Flags: review?(bhackett1024) → review+
I am not 100% sure how to create a targeted test case for this, but I do know that double conversion was blocking the test that I will be adding in bug 865931.
Attachment #747231 - Flags: review?(jdemooij)
Blocks: 865931
Try run for these patches (also including patch from bug 865931) looks green (at least for SM tests, didn't run the rest): https://tbpl.mozilla.org/?tree=Try&rev=ec8d7228222b
Attachment #747231 - Flags: review?(jdemooij) → review+
Comment on attachment 747231 [details] [diff] [review] Disable double conversion during parallel exec Review of attachment 747231 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.cpp @@ +6226,5 @@ > // If we can load the element as a definite double, make sure to check that > // the array has been converted to homogenous doubles first. > + // > + // NB: We disable this optimization in parallel execution mode > + // because it inherently not threadsafe (how do you convert the Nit: s/it/it's
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: