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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: adrake, Assigned: adrake)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 548563 [details] [diff] [review]
Patch v0

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)
(Assignee)

Comment 1

7 years ago
Created attachment 548564 [details] [diff] [review]
Patch v1

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+
(Assignee)

Comment 3

7 years ago
-        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.
(Assignee)

Comment 5

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