Closed Bug 1213750 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS64: Import Trampoline-mips64

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(1 file)

Bug 1140954, Part 11: Trampoline-mips64.
Summary: IonMonkey: MIPS64: Trampoline-mips64 → IonMonkey: MIPS64: Import Trampoline-mips64
Thanks!
Attachment #8672511 - Flags: review?(nicolas.b.pierron)
Attachment #8672511 - Flags: review?(branislav.rankov)
Comment on attachment 8672511 [details] [diff] [review]
Part 11: Import Trampoline-mips64.

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

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +87,5 @@
> +GeneratePrologue(MacroAssembler& masm)
> +{
> +    // Save non-volatile registers. These must be saved by the trampoline,
> +    // rather than the JIT'd code, because they are scanned by the conservative
> +    // scanner.

nit: We no longer have a conservative scanner, so this comment is out-dated.

@@ +116,5 @@
> +/*
> + * This method generates a trampoline for a c++ function with the following
> + * signature:
> + *   void enter(void* code, int argc, Value* argv, InterpreterFrame* fp,
> + *              CalleeToken calleeToken, JSObject* scopeChain, size_t numStackValues, Value* vp)

nit: Mention the EnterJitCode typedef.

@@ +167,5 @@
> +    masm.ma_dsubu(s1, StackPointer, Imm32(sizeof(Value)));
> +    masm.as_movn(StackPointer, s1, s0);
> +
> +    masm.as_dsll(s0, reg_argc, 3); // s0 = argc * 8
> +    masm.addPtr(reg_argv, s0); // s0 = argv + argc * 8

nit: I think  // s0 = &argv[argc * 8]

would be a bit more readable based on the context.

@@ +357,5 @@
> +    // Save floating point registers
> +    // We can use as_sd because stack is alligned.
> +    for (uint32_t i = 0; i < FloatRegisters::TotalPhys; i ++)
> +        masm.as_sd(FloatRegister::FromCode(i), StackPointer,
> +                   InvalidationBailoutStack::offsetOfFpRegs() + i * sizeof(double));

nit: Would it be possible to do that same as on x64?
   https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x64/Trampoline-x64.cpp#353
   https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x64/Trampoline-x64.cpp#22-26

@@ +623,5 @@
> +    // Save floating point registers
> +    // We can use as_sd because stack is alligned.
> +    for (uint32_t i = 0; i < FloatRegisters::TotalPhys; i++)
> +        masm.as_sd(FloatRegister::FromCode(i), StackPointer,
> +                   BailoutStack::offsetOfFpRegs() + i * sizeof(double));

nit: Same thing, use PushRegsInMask if possible.

@@ +631,5 @@
> +    // See: CodeGeneratorMIPS64::generateOutOfLineCode()
> +    masm.storePtr(ra, Address(StackPointer, BailoutStack::offsetOfFrameSize()));
> +
> +    // Put frame class to stack
> +    masm.storePtr(ImmWord(frameClass), Address(StackPointer, BailoutStack::offsetOfFrameClass()));

I am not sure why you want to have this extra field knowing that the FrameSizeClass:ClassLimit is 0.
Attachment #8672511 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 8672511 [details] [diff] [review]
> Part 11: Import Trampoline-mips64.
> 
> Review of attachment 8672511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/Trampoline-mips64.cpp
> @@ +87,5 @@
> > +GeneratePrologue(MacroAssembler& masm)
> > +{
> > +    // Save non-volatile registers. These must be saved by the trampoline,
> > +    // rather than the JIT'd code, because they are scanned by the conservative
> > +    // scanner.
> 
> nit: We no longer have a conservative scanner, so this comment is out-dated.
> 
> @@ +116,5 @@
> > +/*
> > + * This method generates a trampoline for a c++ function with the following
> > + * signature:
> > + *   void enter(void* code, int argc, Value* argv, InterpreterFrame* fp,
> > + *              CalleeToken calleeToken, JSObject* scopeChain, size_t numStackValues, Value* vp)
> 
> nit: Mention the EnterJitCode typedef.
> 
> @@ +167,5 @@
> > +    masm.ma_dsubu(s1, StackPointer, Imm32(sizeof(Value)));
> > +    masm.as_movn(StackPointer, s1, s0);
> > +
> > +    masm.as_dsll(s0, reg_argc, 3); // s0 = argc * 8
> > +    masm.addPtr(reg_argv, s0); // s0 = argv + argc * 8
> 
> nit: I think  // s0 = &argv[argc * 8]
> 
> would be a bit more readable based on the context.
> 
> @@ +357,5 @@
> > +    // Save floating point registers
> > +    // We can use as_sd because stack is alligned.
> > +    for (uint32_t i = 0; i < FloatRegisters::TotalPhys; i ++)
> > +        masm.as_sd(FloatRegister::FromCode(i), StackPointer,
> > +                   InvalidationBailoutStack::offsetOfFpRegs() + i * sizeof(double));
> 
> nit: Would it be possible to do that same as on x64?
>   
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x64/Trampoline-x64.
> cpp#353
>   
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x64/Trampoline-x64.
> cpp#22-26
> 
> @@ +623,5 @@
> > +    // Save floating point registers
> > +    // We can use as_sd because stack is alligned.
> > +    for (uint32_t i = 0; i < FloatRegisters::TotalPhys; i++)
> > +        masm.as_sd(FloatRegister::FromCode(i), StackPointer,
> > +                   BailoutStack::offsetOfFpRegs() + i * sizeof(double));
> 
> nit: Same thing, use PushRegsInMask if possible.
> 
> @@ +631,5 @@
> > +    // See: CodeGeneratorMIPS64::generateOutOfLineCode()
> > +    masm.storePtr(ra, Address(StackPointer, BailoutStack::offsetOfFrameSize()));
> > +
> > +    // Put frame class to stack
> > +    masm.storePtr(ImmWord(frameClass), Address(StackPointer, BailoutStack::offsetOfFrameClass()));
> 
> I am not sure why you want to have this extra field knowing that the
> FrameSizeClass:ClassLimit is 0.

Hi,

I will fix all your mentions except last one and do that in another bug (1217873).

Thank you!
https://hg.mozilla.org/mozilla-central/rev/ebd106bca18d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.