BaselineCompiler: Mark baseline frames

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 680631 [details] [diff] [review]
Patch

Stores the frame size without VMFunction arguments in the frame, then uses that to mark all locals and stack values.
Attachment #680631 - Flags: review?(kvijayan)
(Assignee)

Comment 1

5 years ago
Created attachment 680633 [details] [diff] [review]
Patch v2

Fix a typo.
Attachment #680631 - Attachment is obsolete: true
Attachment #680631 - Flags: review?(kvijayan)
Attachment #680633 - Flags: review?(kvijayan)
Comment on attachment 680633 [details] [diff] [review]
Patch v2

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

::: js/src/ion/shared/IonFrames-x86-shared.h
@@ +450,5 @@
> +    static const uint32_t FramePointerOffset = sizeof(void *);
> +
> +    static inline size_t offsetOfFrameSize() {
> +        return -offsetof(BaselineFrame, frameSize) - sizeof(uint32_t);
> +    }

These offsets are intended to be used with reference to a pointer immediately above the BaselineFrame.  At least that's what my reading of the marking code seems to indicate.

This expression may be clearer as:
-(sizeof(BaselineFrame) - offsetOf(BaselineFrame, frameSize))

And maybe we should call these method backwardsOffsetOf... or reverseOffsetOf... ?  The offsetOf naming convention seems pretty solidly consistent to refer to the offset of things from a pointer to the start of a structure, not the end.

@@ +456,5 @@
> +        return -(sizeof(BaselineFrame) + index * sizeof(js::Value)) - sizeof(js::Value);
> +    }
> +    static inline size_t offsetOfArg(size_t index) {
> +        return FramePointerOffset + js::ion::IonJSFrameLayout::offsetOfActualArg(index);
> +    }

The above statement about renaming the offsetOf... methods wouldn't apply to offsetOfArg since it does compute a forward offset.
Attachment #680633 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 681014 [details] [diff] [review]
Dump baseline frames
Attachment #681014 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

5 years ago
Pushed with the reverse* names + a comment.

https://hg.mozilla.org/projects/ionmonkey/rev/7c9cfaafa4a1
Comment on attachment 681014 [details] [diff] [review]
Dump baseline frames

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

Sounds good. I am happy to see that adding the dumpBaseline function help clarify the bunch of JS_ASSERT of the Mark function.
Attachment #681014 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 6

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