Closed
Bug 1213743
Opened 9 years ago
Closed 9 years ago
IonMonkey: MIPS64: Import CodeGenerator-mips64
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: hev, Assigned: hev)
References
Details
Attachments
(2 files, 1 obsolete file)
12.84 KB,
patch
|
nbp
:
review+
lth
:
review+
|
Details | Diff | Splinter Review |
3.17 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
Bug 1140954, Part 6: CodeGenerator-mips64.
Assignee | ||
Comment 1•9 years ago
|
||
Thanks!
Attachment #8672518 -
Flags: review?(nicolas.b.pierron)
Attachment #8672518 -
Flags: review?(branislav.rankov)
Assignee | ||
Comment 2•9 years ago
|
||
Thanks!
Attachment #8672519 -
Flags: review?(nicolas.b.pierron)
Attachment #8672519 -
Flags: review?(branislav.rankov)
Assignee | ||
Updated•9 years ago
|
Attachment #8672518 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•9 years ago
|
Attachment #8672519 -
Flags: review?(lhansen)
Comment 3•9 years ago
|
||
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?
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8680511 -
Flags: review?(arai.unmht) → review+
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
(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
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5351b1322c90
https://hg.mozilla.org/mozilla-central/rev/92e7f5b6c849
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 11•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5351b1322c90
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/92e7f5b6c849
status-b2g-v2.5:
--- → fixed
Comment 12•9 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•