IonMonkey: Implement callVM on ARM

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: nbp)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
This is necessary to move NewArray to CodeGenerator.cpp, as well as other incoming opcode implementations like GETPROP and bhackett's GETPROP ICs.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED

Updated

5 years ago
Blocks: 710948
Created attachment 583700 [details] [diff] [review]
callVM ARM

Update generateVMWrapper and factor callVM inside CGShared.
Attachment #583700 - Flags: review?(dvander)
(Reporter)

Comment 2

5 years ago
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+
(Reporter)

Updated

5 years ago
Duplicate of this bug: 701984
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.
(Reporter)

Comment 5

5 years ago
> ::: 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.
https://hg.mozilla.org/projects/ionmonkey/rev/60766fb64a78
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.