Closed Bug 837639 Opened 10 years ago Closed 10 years ago

BaselineCompiler: Convert int32 to double when storing to double arrays

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Not doing this is causing correctness issues with bitops-nsieve-bits.
Attached patch PatchSplinter Review
If ObjectElements::convertDoubleElements is set, convert int32 values to double. The extra branch is a bit unfortunate, but convertDoubleElements is unrelated to the shape or TypeObject, so it's hard to avoid.

If it ever becomes a problem we can make the jump patchable and patch it the first time we set convertDoubleElements, or make the shape determine this flag. The patch does not seem to affect SS (much) though so I think it's fine for now.
Attachment #709691 - Flags: review?(kvijayan)
Comment on attachment 709691 [details] [diff] [review]
Patch

Review of attachment 709691 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.

::: js/src/ion/BaselineIC.cpp
@@ +2096,5 @@
>      masm.patchableCallPreBarrier(element, MIRType_Value);
>      masm.storeValue(R0, element);
>      EmitReturnFromIC(masm);
>  
> +    masm.bind(&convertDoubles);

Nit: Comment for this section would be nice.  Eg "// Convert to double and jump back."

@@ +2210,4 @@
>      masm.storeValue(R0, element);
>      EmitReturnFromIC(masm);
>  
> +    masm.bind(&convertDoubles);

Nit: Comment, as bove

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +601,5 @@
>          JS_ASSERT(cond == Equal || cond == NotEqual);
>          cmpl(ToUpper32(operand), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_INT32))));
>          j(cond, label);
>      }
> +    void branchTestInt32(Condition cond, const Address &address, Label *label) {

Nit: assert cond == Equal or NotEqual, as in the ARM code.

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +302,5 @@
>          JS_ASSERT(cond == Equal || cond == NotEqual);
>          cmpl(ToType(operand), ImmTag(JSVAL_TAG_INT32));
>          return cond;
>      }
> +    Condition testInt32(Condition cond, const Address &address) {

Nit: assert cond == Equal or NotEqual, as in the ARM code.
Attachment #709691 - Flags: review?(kvijayan) → review+
Pushed with nits addressed:

https://hg.mozilla.org/projects/ionmonkey/rev/386a8cc4fbe9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.