Closed Bug 674334 Opened 13 years ago Closed 13 years ago

IonMonkey: Failures on optimized builds and on large-stack scripts

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: adrake)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v0 (obsolete) — Splinter Review
This fixes:

1) Off by one in linear scan reifier where the "to" half of a move would be included in the freeRegs set.
2) Inactive intervals were removed from the active list (I don't think this actually breaks anything due to the way InlineList is implemented).
3) The 'handled' list was #ifdef DEBUG, and is now needed for the algorithm proper.
4) The code generator did not properly take into account fixed frame sizes when computing stack slot offsets.
5) The code generator swapped Move::GENERAL and Move::DOUBLE at move creation time.
6) MoveEmitterX86::toOperand did not handle register operands, thus register to register moves could not be emitted.
7) The code generator called emitDoubleMove for integer register moves, and vice-versa.
Attachment #548563 - Flags: review?(dvander)
Attached patch Patch v1Splinter Review
Removes spurious whitespace insertion that didn't get qrefreshed before I hit submit on the initial report.
Assignee: general → adrake
Attachment #548563 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #548563 - Flags: review?(dvander)
Attachment #548564 - Flags: review?(dvander)
Comment on attachment 548564 [details] [diff] [review]
Patch v1

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

Thanks for finding and fixing these!

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +79,5 @@
>      }
>  
>      inline int32 SlotToStackOffset(int32 slot) {
>          JS_ASSERT(slot >= 0 && slot <= int32(graph.localSlotCount()));
> +        int32 offset = masm.framePushed() - frameStaticSize_ + slot * STACK_SLOT_SIZE;

Let's put the slots at the top of the frame instead, clearly this was totally wrong before though.
Attachment #548564 - Flags: review?(dvander) → review+
-        JS_ASSERT(slot >= 0 && slot <= int32(graph.localSlotCount()));
-        int32 offset = masm.framePushed() - frameStaticSize_ + slot * STACK_SLOT_SIZE;
+        JS_ASSERT(slot > 0 && slot <= int32(graph.localSlotCount()));
+        int32 offset = masm.framePushed() - slot * STACK_SLOT_SIZE;

How about that?
(In reply to comment #3)
> -        JS_ASSERT(slot >= 0 && slot <= int32(graph.localSlotCount()));
> -        int32 offset = masm.framePushed() - frameStaticSize_ + slot *
> STACK_SLOT_SIZE;
> +        JS_ASSERT(slot > 0 && slot <= int32(graph.localSlotCount()));
> +        int32 offset = masm.framePushed() - slot * STACK_SLOT_SIZE;
> 
> How about that?

Looks good.
http://hg.mozilla.org/projects/ionmonkey/rev/07a3818d8d24
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.