Last Comment Bug 710130 - IonMonkey: Implement callVM on ARM
: IonMonkey: Implement callVM on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 701984 (view as bug list)
Depends on:
Blocks: 710948
  Show dependency treegraph
 
Reported: 2011-12-12 23:03 PST by David Anderson [:dvander]
Modified: 2011-12-28 12:12 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
callVM ARM (46.25 KB, patch)
2011-12-21 18:22 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-12-12 23:03:54 PST
This is necessary to move NewArray to CodeGenerator.cpp, as well as other incoming opcode implementations like GETPROP and bhackett's GETPROP ICs.
Comment 1 Nicolas B. Pierron [:nbp] 2011-12-21 18:22:24 PST
Created attachment 583700 [details] [diff] [review]
callVM ARM

Update generateVMWrapper and factor callVM inside CGShared.
Comment 2 David Anderson [:dvander] 2011-12-22 11:24:45 PST
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?
Comment 3 David Anderson [:dvander] 2011-12-22 11:53:00 PST
*** Bug 701984 has been marked as a duplicate of this bug. ***
Comment 4 Nicolas B. Pierron [:nbp] 2011-12-22 12:32:42 PST
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.
Comment 5 David Anderson [:dvander] 2011-12-22 12:36:24 PST
> ::: 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.
Comment 6 Nicolas B. Pierron [:nbp] 2011-12-28 12:12:33 PST
https://hg.mozilla.org/projects/ionmonkey/rev/60766fb64a78

Note You need to log in before you can comment on or make changes to this bug.