IonMonkey: Implement callVM on ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dvander, Assigned: nbp)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

This is necessary to move NewArray to CodeGenerator.cpp, as well as other incoming opcode implementations like GETPROP and bhackett's GETPROP ICs.
(Assignee)

Updated

6 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED

Updated

6 years ago
Blocks: 710948
(Assignee)

Comment 1

6 years ago
Created attachment 583700 [details] [diff] [review]
callVM ARM

Update generateVMWrapper and factor callVM inside CGShared.
Attachment #583700 - Flags: review?(dvander)
Comment on attachment 583700 [details] [diff] [review]
callVM ARM

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

r=me with nits picked

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +501,3 @@
>      MacroAssembler masm;
> +    GeneralRegisterSet regs =
> +        GeneralRegisterSet::Not(GeneralRegisterSet(Register::Codes::VolatileMask));

What's the significance of choosing ~Volatile here?

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +290,5 @@
> +    return true;
> +}
> +
> +bool
> +CodeGeneratorShared::visitNewArray(LNewArray *ins)

visitNewArray and visitLoadPropertyGeneric should be in CodeGenerator, rather than CodeGenerator-shared. CG-shared is intended for helper functions that are usable across all architectures, whereas CG is the "frontend".

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +66,5 @@
>      LIRGraph &graph;
>      LBlock *current;
>      SnapshotWriter snapshots_;
>      IonCode *deoptTable_;
> +#ifdef DEBUG

Newline above DEBUG (though I think it looks nicer without the DEBUG).

@@ +183,4 @@
>      template <typename T>
>      void pushArg(const T &t) {
>          masm.Push(t);
> +#ifdef DEBUG

Ditto.

::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +141,5 @@
>      }
>      void load32(const Address &address, Register dest) {
>          movl(Operand(address), dest);
>      }
> +    void callExitIon(IonCode *target) {

Please rename this to "callWithDescriptor"

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +480,5 @@
>  
>      // Initialize and set the context parameter.
>      Register cxreg = regs.takeAny();
> +    masm.movePtr(ImmWord(JS_THREAD_DATA(cx)), cxreg);
> +    masm.loadPtr(Address(cxreg, offsetof(ThreadData, ionJSContext)), cxreg);

What was the reasoning behind this change?
Attachment #583700 - Flags: review?(dvander) → review+
Duplicate of this bug: 701984
(Assignee)

Comment 4

6 years ago
Comment on attachment 583700 [details] [diff] [review]
callVM ARM

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

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +501,3 @@
>      MacroAssembler masm;
> +    GeneralRegisterSet regs =
> +        GeneralRegisterSet::Not(GeneralRegisterSet(Register::Codes::VolatileMask));

On ARM, Volatile registers are registers used for arguments using the same set of registers as setABIArg cause issues in the MoveEmitter.

I would have prefered to use Volatile registers here to later be able to keep non-GC values into registers across calls, but we will see when we implement such optimization.

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +66,5 @@
>      LIRGraph &graph;
>      LBlock *current;
>      SnapshotWriter snapshots_;
>      IonCode *deoptTable_;
> +#ifdef DEBUG

pushedArgs_ is only used in a JS_ASSERT in callVM function.  So I don't see any need to keep it updating it if it is never used in optimized builds.

::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +141,5 @@
>      }
>      void load32(const Address &address, Register dest) {
>          movl(Operand(address), dest);
>      }
> +    void callExitIon(IonCode *target) {

Unfortunately not.  The reason is that ARM may end-up having different frame layout for the exit frame and other JS frames.

What I can do is callIonFrame(FrameType current, FrameType called)

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +480,5 @@
>  
>      // Initialize and set the context parameter.
>      Register cxreg = regs.takeAny();
> +    masm.movePtr(ImmWord(JS_THREAD_DATA(cx)), cxreg);
> +    masm.loadPtr(Address(cxreg, offsetof(ThreadData, ionJSContext)), cxreg);

to be consistent with the ARM implementation which does not have an equivalent of

masm.movl(Operand(…), reg).

and we don't have Address(offset) constructor.
> ::: js/src/ion/shared/MacroAssembler-x86-shared.h
> @@ +141,5 @@
> >      }
> >      void load32(const Address &address, Register dest) {
> >          movl(Operand(address), dest);
> >      }
> > +    void callExitIon(IonCode *target) {
> 
> Unfortunately not.  The reason is that ARM may end-up having different frame
> layout for the exit frame and other JS frames.
> 
> What I can do is callIonFrame(FrameType current, FrameType called)

Okay, "callWithExitFrame" then.
(Assignee)

Comment 6

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