Clean-up: Move MacroAssemblerSpecific::framePushed to the generic MacroAssembler.

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

Each MacroAssembler redefine the same framePushed logic.  We should move the frame logic into the generic MacroAssembler.
Assignee: nobody → nicolas.b.pierron
Created attachment 8611241 [details] [diff] [review]
Move MacroAssemblerSpecific::framePushed_ fields to the generic MacroAssembler.

Sorry, this is quite a big patch.

The goal of this patch is to move all framePushed_ references to the generic
MacroAssembler.  This implies creating a jit/MacroAssembler-inl.h file, as
these functions are used quite frequently.

Also, as we cannot include "-inl.h" in any ".h" header, and reference any
asMasm() implementation, we have to either move some functions to the
respective "-inl.h" file or to the ".cpp" file.  This last constraint make
this patch quite verbose compared to the expected modification.

I tested this patch locally against x64, x86, arm, mips(*), and with Ion disabled.

(*) Mips backend does not build, but I removed all errors related to the
current modifications.
Attachment #8611241 - Flags: review?(jdemooij)
Comment on attachment 8611241 [details] [diff] [review]
Move MacroAssemblerSpecific::framePushed_ fields to the generic MacroAssembler.

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

Nice!

::: js/src/jit/MacroAssembler.h
@@ +281,5 @@
> +    // This field is used to statically (at compilation time) emulate a frame
> +    // pointer by keeping track of stack manipulations.
> +    //
> +    // It is maintained by all stack manipulation functions below.
> +    uint32_t framePushed_;

This field should be private or protected.

::: js/src/jit/x86/MacroAssembler-x86.cpp
@@ +599,5 @@
> +
> +void
> +MacroAssembler::freeStack(Register amount)
> +{
> +    addl(amount, StackPointer);

I wonder if this could be |addPtr(amount, StackPointer);| and shared between platforms. Doesn't matter too much though.
Attachment #8611241 - Flags: review?(jdemooij) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ade1289c095a

Delta:
 - Moved CodeGeneratorSpecific::ToOperand to CodeGeneratorShared::ToOperand (somehow, I did not noticed that before)
 - framePushed_ is now private.
 - MacroAssembler::freeStack is no longer per-arch.
https://hg.mozilla.org/mozilla-central/rev/06ca9c794fd0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1176633
You need to log in before you can comment on or make changes to this bug.