Closed Bug 1213743 Opened 7 years ago Closed 7 years ago

IonMonkey: MIPS64: Import CodeGenerator-mips64

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1140954, Part 6: CodeGenerator-mips64.
Thanks!
Attachment #8672518 - Flags: review?(nicolas.b.pierron)
Attachment #8672518 - Flags: review?(branislav.rankov)
Thanks!
Attachment #8672519 - Flags: review?(nicolas.b.pierron)
Attachment #8672519 - Flags: review?(branislav.rankov)
Attachment #8672518 - Flags: review?(arai.unmht)
Attachment #8672519 - Flags: review?(lhansen)
Comment on attachment 8672518 [details] [diff] [review]
Part 6.1: Import MIPS64 support into CodGenerator-mips-shared.

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

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ +110,5 @@
>  
>  void
>  CodeGeneratorMIPSShared::visitCompare(LCompare* comp)
>  {
> +    MCompare* mir = comp->mir();

This should be inside #ifdef, as it's used only there and warning will be shown for mips32 build.

@@ +137,5 @@
>  
>  void
>  CodeGeneratorMIPSShared::visitCompareAndBranch(LCompareAndBranch* comp)
>  {
> +    MCompare* mir = comp->cmpMir();

This should also be inside #ifdef.

@@ +1551,5 @@
>      // patching to occur. Otherwise, we could overwrite the invalidation
>      // epilogue.
> +    size_t existSize = masm.currentOffset() - returnLabel_.offset();
> +    if (Assembler::PatchWrite_NearCallSize() > existSize) {
> +        size_t padSize = Assembler::PatchWrite_NearCallSize() - existSize;

Can you tell me what's going on above 3 lines?
Blocks: 1217873
Comment on attachment 8672519 [details] [diff] [review]
Part 6.2: Import CodeGeneraotr-mips64.

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

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +83,5 @@
> +        masm.subPtr(Imm32(mir->low()), index);
> +
> +    // Jump to default case if input is out of range
> +    int32_t cases = mir->numCases();
> +    masm.branchPtr(Assembler::AboveOrEqual, index, ImmWord(cases), defaultcase);

branch32?

@@ +93,5 @@
> +    addOutOfLineCode(ool, mir);
> +
> +    // Compute the position where a pointer to the right case stands.
> +    masm.ma_li(address, ool->jumpLabel()->dest());
> +    masm.lshiftPtr(Imm32(5), index);

The "5" needs a comment, at a minimum, and the location in the code that corresponds to the 5 needs another comment to match.

@@ +134,5 @@
> +CodeGeneratorMIPS64::ToTempValue(LInstruction* ins, size_t pos)
> +{
> +    return ValueOperand(ToRegister(ins->getTemp(pos)));
> +}
> +

Nit: blank line.
Attachment #8672519 - Flags: review?(lhansen) → review+
(In reply to Tooru Fujisawa [:arai] from comment #3)
> Comment on attachment 8672518 [details] [diff] [review]
> Part 6.1: Import MIPS64 support into CodGenerator-mips-shared.
> 
> Review of attachment 8672518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
> @@ +110,5 @@
> >  
> >  void
> >  CodeGeneratorMIPSShared::visitCompare(LCompare* comp)
> >  {
> > +    MCompare* mir = comp->mir();
> 
> This should be inside #ifdef, as it's used only there and warning will be
> shown for mips32 build.
> 
> @@ +137,5 @@
> >  
> >  void
> >  CodeGeneratorMIPSShared::visitCompareAndBranch(LCompareAndBranch* comp)
> >  {
> > +    MCompare* mir = comp->cmpMir();
> 
> This should also be inside #ifdef.
OK

> 
> @@ +1551,5 @@
> >      // patching to occur. Otherwise, we could overwrite the invalidation
> >      // epilogue.
> > +    size_t existSize = masm.currentOffset() - returnLabel_.offset();
> > +    if (Assembler::PatchWrite_NearCallSize() > existSize) {
> > +        size_t padSize = Assembler::PatchWrite_NearCallSize() - existSize;
> 
> Can you tell me what's going on above 3 lines?
That's fix a bug, bug i forgot which one. I remove it at this point.
Attachment #8672518 - Attachment is obsolete: true
Attachment #8672518 - Flags: review?(nicolas.b.pierron)
Attachment #8672518 - Flags: review?(branislav.rankov)
Attachment #8672518 - Flags: review?(arai.unmht)
Attachment #8680511 - Flags: review?(arai.unmht)
Attachment #8680511 - Flags: review?(arai.unmht) → review+
Comment on attachment 8672519 [details] [diff] [review]
Part 6.2: Import CodeGeneraotr-mips64.

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

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +65,5 @@
> +        CodeLabel cl;
> +        masm.ma_li(ScratchRegister, cl.dest());
> +        masm.branch(ScratchRegister);
> +        masm.as_nop();
> +        masm.as_nop();

Any reason to have explicit nops here instead of making a pacthable branch?
Attachment #8672519 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 8672519 [details] [diff] [review]
> Part 6.2: Import CodeGeneraotr-mips64.
> 
> Review of attachment 8672519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/CodeGenerator-mips64.cpp
> @@ +65,5 @@
> > +        CodeLabel cl;
> > +        masm.ma_li(ScratchRegister, cl.dest());
> > +        masm.branch(ScratchRegister);
> > +        masm.as_nop();
> > +        masm.as_nop();
> 
> Any reason to have explicit nops here instead of making a pacthable branch?

The two padding nops to making size of table line is pow of 2. Thus we can get target offset based on table by left shift 'index' by 5 in emitTableSwitchDispatch.

Please see: https://hg.mozilla.org/integration/mozilla-inbound/file/be3d699655ec/js/src/jit/mips64/CodeGenerator-mips64.cpp#l72
and https://hg.mozilla.org/integration/mozilla-inbound/file/be3d699655ec/js/src/jit/mips64/CodeGenerator-mips64.cpp#l106
https://hg.mozilla.org/mozilla-central/rev/5351b1322c90
https://hg.mozilla.org/mozilla-central/rev/92e7f5b6c849
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.